-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
base: main
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
... and 677 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
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 |
There was a problem hiding this 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.
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:
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. |
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. |
📜 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:
sendDefaultPII
is enabled.