Skip to content

feat(session-replay): add preferred frame rate to display link wrapper #5258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

philprime
Copy link
Contributor

@philprime philprime commented May 20, 2025

📜 Description

Configures the CADisplayLink used by session replay to use the target frame rate

💡 Motivation and Context

If the display link is not configured with a target frame rate, it can cause apps to run at 60 FPS even if not necessary.

💚 How did you test it?

Ran the sample app and checked created frames

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

@philprime philprime self-assigned this May 20, 2025
@philprime philprime changed the title feat: add preferred frame rate to display link wrapper feat(session-replay): add preferred frame rate to display link wrapper May 20, 2025
Copy link
Contributor

github-actions bot commented May 20, 2025

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- add preferred frame rate to display link wrapper ([#5258](https://github.com/getsentry/sentry-cocoa/pull/5258))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against b94ef04

@philprime philprime marked this pull request as ready for review May 20, 2025 09:15
Copy link

codecov bot commented May 20, 2025

Codecov Report

Attention: Patch coverage is 0% with 25 lines in your changes missing coverage. Please review.

Project coverage is 9.172%. Comparing base (e70c3aa) to head (b94ef04).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
...tegrations/SessionReplay/SentrySessionReplay.swift 0.000% 16 Missing ⚠️
Sources/Sentry/include/SentryDisplayLinkWrapper.m 0.000% 6 Missing ⚠️
Sources/Sentry/SentryFramesTracker.m 0.000% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (e70c3aa) and HEAD (b94ef04). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (e70c3aa) HEAD (b94ef04)
4 1
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main    #5258        +/-   ##
=============================================
- Coverage   92.893%   9.172%   -83.721%     
=============================================
  Files          689      363       -326     
  Lines        86259    26535     -59724     
  Branches     31221      121     -31100     
=============================================
- Hits         80129     2434     -77695     
- Misses        6028    24101     +18073     
+ Partials       102        0       -102     
Files with missing lines Coverage Δ
Sources/Sentry/SentryFramesTracker.m 17.142% <0.000%> (-82.375%) ⬇️
Sources/Sentry/include/SentryDisplayLinkWrapper.m 0.000% <0.000%> (-100.000%) ⬇️
...tegrations/SessionReplay/SentrySessionReplay.swift 0.000% <0.000%> (-90.554%) ⬇️

... and 677 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e70c3aa...b94ef04. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1232.82 ms 1259.96 ms 27.14 ms
Size 23.76 KiB 820.87 KiB 797.11 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2f8b3a8 1233.76 ms 1260.24 ms 26.48 ms
5e66a38 1209.10 ms 1233.90 ms 24.79 ms
94e1968 1238.85 ms 1252.02 ms 13.17 ms
d011484 1220.86 ms 1237.18 ms 16.33 ms
f0737f6 1220.43 ms 1236.44 ms 16.01 ms
324dc7b 1210.33 ms 1244.92 ms 34.59 ms
79239ff 1223.53 ms 1250.98 ms 27.45 ms
f278bab 1236.50 ms 1251.75 ms 15.25 ms
84fb4d9 1212.45 ms 1223.39 ms 10.94 ms
f938d24 1223.26 ms 1242.12 ms 18.86 ms

App size

Revision Plain With Sentry Diff
2f8b3a8 20.76 KiB 434.72 KiB 413.96 KiB
5e66a38 22.85 KiB 408.88 KiB 386.03 KiB
94e1968 21.58 KiB 614.74 KiB 593.16 KiB
d011484 21.58 KiB 616.14 KiB 594.56 KiB
f0737f6 20.76 KiB 437.12 KiB 416.36 KiB
324dc7b 22.85 KiB 408.84 KiB 385.99 KiB
79239ff 22.31 KiB 765.78 KiB 743.46 KiB
f278bab 22.31 KiB 760.65 KiB 738.33 KiB
84fb4d9 22.84 KiB 402.56 KiB 379.72 KiB
f938d24 20.76 KiB 434.88 KiB 414.12 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the display link is not configured with a target frame rate, it can cause apps to run at 60 FPS even if not necessary.

Can you prove this? How can we be sure that's the case? According to a recent investigation of @noahsmartin we should actually swap out the CADisplayLink and use a RunLoopObserver instead; see internal Notion doc. I think it makes sense to decide on our future approach before touching this right now.

@philprime
Copy link
Contributor Author

Can you prove this?
I observed this behavior during testing of #4940 that the sample app starts off with less than 60 FPS frame rate, then when SR starts it spins up to 60 FPS. But it could also be caused from unoptimized

I would have to create a reproducible project to proof this with numbers, but I believe the documentation [1] states enough reason for this change:

Use lower refresh rates whenever possible to save power, because higher refresh rates can result in significant power consumption.

According to a recent investigation of @noahsmartin we should actually swap out the CADisplayLink and use a RunLoopObserver instead; see internal Notion doc. I think it makes sense to decide on our future approach before touching this right now.

I agree with swapping it out for performance tracing, I am not convinced yet that it is the right approach for visual timers and per-frame updates. We need to ensure that we do get the target frame rate, as we are trying to achieve constant FPS. This is out-of-scope for this PR.

@philipphofmann
Copy link
Member

As discussed in an internal meeting, I think I wouldn't change the preferred frame rate for SR right now. I think we first should investigate #5263 before tackling this PR.

@philprime philprime added the dontmerge A branch that absolutely should not be merged while this label is applied. label May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dontmerge A branch that absolutely should not be merged while this label is applied.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants