-
-
Notifications
You must be signed in to change notification settings - Fork 329
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
perf: gather profiler metrics gathering on a low-pri queue #2956
perf: gather profiler metrics gathering on a low-pri queue #2956
Conversation
@implementation SentryDispatchQueueWrapper { | ||
// Don't use a normal property because on RN a user got a warning "Property with 'retain (or | ||
// strong)' attribute must be of object type". A dispatch queue is since iOS 6.0 an NSObject so | ||
// it should work with strong, but nevertheless, we use an instance variable to fix this | ||
// warning. | ||
dispatch_queue_t queue; | ||
} |
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.
This is no longer true since we bumped our min supported version. I believe we can also change this:
sentry-cocoa/Sources/Sentry/SentryDispatchQueueWrapper.m
Lines 16 to 17 in d257eb9
// DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL is requires iOS 10. Since we are targeting | |
// iOS 9 we need to manually add the autoreleasepool. |
This is now exposed as a property in the public interface so it can be accessed when setting up a new dispatch_source_t
in SentryDispatchSourceWrapper
[
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3e7aa41 | 1214.02 ms | 1238.84 ms | 24.82 ms |
d253cdf | 1231.61 ms | 1259.52 ms | 27.91 ms |
d60f70a | 1219.63 ms | 1228.54 ms | 8.91 ms |
cf724da | 1243.14 ms | 1261.44 ms | 18.30 ms |
d1d5c2d | 1239.20 ms | 1251.92 ms | 12.72 ms |
cb2eefe | 1222.04 ms | 1243.04 ms | 21.00 ms |
ecd9ecd | 1204.67 ms | 1226.46 ms | 21.79 ms |
2cf1b84 | 6265.25 ms | 6277.98 ms | 12.73 ms |
25bcc50 | 1240.47 ms | 1254.70 ms | 14.23 ms |
10ee2ce | 1250.90 ms | 1258.57 ms | 7.67 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
3e7aa41 | 20.76 KiB | 432.31 KiB | 411.55 KiB |
d253cdf | 20.76 KiB | 427.66 KiB | 406.90 KiB |
d60f70a | 20.76 KiB | 430.97 KiB | 410.21 KiB |
cf724da | 20.76 KiB | 430.52 KiB | 409.76 KiB |
d1d5c2d | 20.76 KiB | 427.83 KiB | 407.07 KiB |
cb2eefe | 20.76 KiB | 419.62 KiB | 398.86 KiB |
ecd9ecd | 20.76 KiB | 420.23 KiB | 399.47 KiB |
2cf1b84 | 20.76 KiB | 431.91 KiB | 411.15 KiB |
25bcc50 | 20.76 KiB | 427.22 KiB | 406.46 KiB |
10ee2ce | 20.76 KiB | 427.77 KiB | 407.00 KiB |
Previous results on branch: armcknight/perf/metric-profiler-nonmain-thread
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
fcf88c3 | 1244.68 ms | 1253.80 ms | 9.12 ms |
9584752 | 1218.84 ms | 1233.46 ms | 14.62 ms |
3c724c4 | 1246.64 ms | 1250.74 ms | 4.10 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
fcf88c3 | 20.76 KiB | 433.87 KiB | 413.11 KiB |
9584752 | 20.76 KiB | 435.54 KiB | 414.78 KiB |
3c724c4 | 20.76 KiB | 435.56 KiB | 414.80 KiB |
[weakSelf recordCPUPercentagePerCore]; | ||
[weakSelf recordMemoryFootprint]; |
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.
recordCPUPercentagePerCore
and recordMemoryFootprint
were previously called on the main thread, are they thread safe?
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.
Good question. Their calls to task_info
and host_processor_info
are not synchronized, but their modifications of the data structure those readings are stored in are inside @synchronized
blocks. Is this safe, or do you think we should move the task_info
/host_processor_info
calls inside the synchronization as well?
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.
i improved the method that reads from the data structure as well in adef4bc
Sorry, @armcknight, I didn't get to this today. I will try to have a look tomorrow. In the meantime maybe you have time to fix CI 😃? |
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.
A few suggestions to think about.
Not reviewing this, as @brustolin already LGTM. |
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- gather profiler metrics gathering on a low-pri queue ([#2956](https://github.com/getsentry/sentry-cocoa/pull/2956)) If none of the above apply, you can opt out of this check by adding |
📜 Description
Change the metric profiler from using an NSTimer, which dispatches blocks to the main thread, to a dispatch source that dispatches blocks to a queue with a utility quality of service.
💡 Motivation and Context
We noticed some customer profiles that showed our own symbols doing lots of work on main threads. This is undesirable from a customer POV and isn't even really necessary for these readings.
💚 How did you test it?
Created mocking for the dispatch source and verified the preexisting tests still work.
This also introduces the aforementioned dispatch factory that vends our wrappers for dispatch queues and the new source wrapper, which we should eventually use in all the places in the SDK that internally alloc/init queue wrappers, for easier and more consistent mockability in tests.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.- [ ] I updated the docs if needed.- [ ] Review from the native team if needed.🔮 Next steps