-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
impr: Capture profiles on a BG Thread #4377
base: main
Are you sure you want to change the base?
Conversation
Serializing and capturing a profile can run on the main thread and block it for a couple of milliseconds. Therefore, we move this to a background thread to avoid potentially blocking the main thread.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4377 +/- ##
=============================================
+ Coverage 91.450% 91.454% +0.003%
=============================================
Files 629 630 +1
Lines 50625 50658 +33
Branches 18346 18358 +12
=============================================
+ Hits 46297 46329 +32
Misses 4235 4235
- Partials 93 94 +1
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1bf8571 | 1250.96 ms | 1255.36 ms | 4.40 ms |
2ccbd03 | 1225.13 ms | 1247.51 ms | 22.39 ms |
d011484 | 1220.86 ms | 1237.18 ms | 16.33 ms |
6001822 | 1220.82 ms | 1245.02 ms | 24.20 ms |
3723833 | 1205.22 ms | 1216.94 ms | 11.71 ms |
ff09c7e | 1240.94 ms | 1262.66 ms | 21.72 ms |
b9d59f7 | 1250.71 ms | 1257.78 ms | 7.06 ms |
94d8eb3 | 1234.02 ms | 1249.63 ms | 15.60 ms |
e773cad | 1219.86 ms | 1238.26 ms | 18.40 ms |
90d17d3 | 1261.18 ms | 1278.18 ms | 17.00 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1bf8571 | 20.76 KiB | 437.12 KiB | 416.36 KiB |
2ccbd03 | 21.58 KiB | 546.20 KiB | 524.62 KiB |
d011484 | 21.58 KiB | 616.14 KiB | 594.56 KiB |
6001822 | 22.85 KiB | 410.98 KiB | 388.13 KiB |
3723833 | 21.58 KiB | 424.34 KiB | 402.76 KiB |
ff09c7e | 20.76 KiB | 427.76 KiB | 407.00 KiB |
b9d59f7 | 22.85 KiB | 405.77 KiB | 382.93 KiB |
94d8eb3 | 21.58 KiB | 417.86 KiB | 396.28 KiB |
e773cad | 21.58 KiB | 681.75 KiB | 660.17 KiB |
90d17d3 | 20.76 KiB | 432.17 KiB | 411.41 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.
Thanks for this. There is one thing to discuss.
Co-authored-by: Dhiogo Brustolin <[email protected]>
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.
LGTM!
I'm still waiting for @armcknight to approve this so we can avoid shooting ourselves in the foot. |
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.
Could you be more specific about how you tested this? I just tried looking at a manual transaction with trace-based profiling on main vs this branch and the profiles look different. I can't get a launch profile to show up when running from this branch.
I tried comparing the differences between main and this branch with continuous profiling but I'm not even seeing what I expect on main so i need to ask the team what's going on there.
At a minimum for anything profiling right now you have to check what it looks like on main
against your changes for:
- trace-based
- regular transaction
- launch
- continuous
- regular transaction
- launch
Trace-based regular txn on main (https://sentry-sdks.sentry.io/profiling/profile/sentry-cocoa/cdc0737ef92245a0b14dcdb40c5d68c3/flamegraph/?colorCoding=by%20system%20vs%20application%20frame&fov=0%2C0%2C3822472960%2C19&query=&sorting=call%20order&tid=259&view=top%20down):
Hmm, weird, I didn't test it in great detail as I didn't expect that when all the tests are green, such a small change can have such a big impact. @armcknight, do you have a clue about what could cause this? I will look into this tomorrow. |
@philipphofmann my guess is that you aren't dispatching the right call to the bg queue, and so it captures a set of profiling data much earlier than it marks the end of the transaction. You may need to look for a more granular point in that process, or create one by refactoring. |
I found the problem. This PR only impacts trace based profiles, as only the SentryTracer uses I tested this for manual transactions, UIViewController transactions and app start transactions.
|
return; | ||
} | ||
|
||
const auto profilingData = [profiler.state copyProfilingData]; |
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.
@armcknight, when I move this into the async block the profiles the SDK shortens the profiles uploaded to Sentry, but I can't replicate this behavior in the testCaptureTransactionWithProfile_SetsCorrectAmountOfSamples
. I would like to have a failing test for this to ensure we don't break this by mistake. Maybe the tests with the mocks behave differently than the actual profiling code, or I just don't get it.
📜 Description
Serializing and capturing a profile can run on the main thread and block it for a couple of milliseconds. Therefore, we move this to a background thread to avoid potentially blocking the main thread.
💡 Motivation and Context
This came up while investigating profiles for potentially slowing down UIViewControllers.
💚 How did you test it?
Unit test and checking if the profile is still correctly attached to UIViewController transactions with our iOS-Swift sample app.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps