-
-
Notifications
You must be signed in to change notification settings - Fork 339
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: optimize some conversions of unsigned ints to NSString #2863
perf: optimize some conversions of unsigned ints to NSString #2863
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c00eafe | 1198.26 ms | 1227.62 ms | 29.36 ms |
ecd9ecd | 1241.28 ms | 1260.35 ms | 19.07 ms |
8526e93 | 1228.24 ms | 1239.14 ms | 10.90 ms |
2cf1b84 | 6265.25 ms | 6277.98 ms | 12.73 ms |
efb2222 | 1256.44 ms | 1278.90 ms | 22.46 ms |
aa27ac0 | 1214.02 ms | 1227.42 ms | 13.40 ms |
d60f70a | 1219.63 ms | 1228.54 ms | 8.91 ms |
ea73af6 | 1241.69 ms | 1252.96 ms | 11.27 ms |
9ef729b | 1228.79 ms | 1245.36 ms | 16.57 ms |
e79ead7 | 1233.39 ms | 1255.52 ms | 22.13 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c00eafe | 20.76 KiB | 432.87 KiB | 412.11 KiB |
ecd9ecd | 20.76 KiB | 420.23 KiB | 399.47 KiB |
8526e93 | 20.76 KiB | 420.22 KiB | 399.47 KiB |
2cf1b84 | 20.76 KiB | 431.91 KiB | 411.15 KiB |
efb2222 | 20.76 KiB | 424.45 KiB | 403.69 KiB |
aa27ac0 | 20.76 KiB | 432.21 KiB | 411.45 KiB |
d60f70a | 20.76 KiB | 430.97 KiB | 410.21 KiB |
ea73af6 | 20.76 KiB | 425.76 KiB | 404.99 KiB |
9ef729b | 20.76 KiB | 432.88 KiB | 412.12 KiB |
e79ead7 | 20.76 KiB | 426.11 KiB | 405.35 KiB |
Previous results on branch: armcknight/perf/speed-up-profiler-hex-address-formatting
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
69f3d5b | 1243.90 ms | 1250.70 ms | 6.80 ms |
b506df4 | 1271.62 ms | 1273.61 ms | 1.99 ms |
5854c18 | 1245.02 ms | 1256.52 ms | 11.50 ms |
8e69b81 | 1203.33 ms | 1229.88 ms | 26.55 ms |
8b72bae | 1220.41 ms | 1257.62 ms | 37.21 ms |
f5bcb8f | 1219.49 ms | 1240.94 ms | 21.45 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
69f3d5b | 20.76 KiB | 430.69 KiB | 409.93 KiB |
b506df4 | 20.76 KiB | 433.00 KiB | 412.24 KiB |
5854c18 | 20.76 KiB | 433.07 KiB | 412.31 KiB |
8e69b81 | 20.76 KiB | 431.87 KiB | 411.11 KiB |
8b72bae | 20.76 KiB | 432.08 KiB | 411.32 KiB |
f5bcb8f | 20.76 KiB | 431.87 KiB | 411.11 KiB |
33cbb19
to
9781a6b
Compare
…thFormat with sentry_format
- don't use macro or vargs - separate into hex address and uint64 functions - use hardcoded buffer size for hex address version
9781a6b
to
22d47ab
Compare
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'm surprised that snprintf
is much faster than sentry_formatHexAddress
. Thanks for the improvement.
I think it comes down to all the extra allocations around NSStrings (even for the literal format string) and conversion of NSNumber back to an unsigned long long, both of which are avoided in the new version (and avoiding boxing the ull's to NSNumbers in the first place solely to pass them to this function is an extra benefit). |
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, what's stopping us from making this ready for review?
…-up-profiler-hex-address-formatting
@philipphofmann this one's ready for you now! |
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, @armcknight, for this performance improvement 👏 😃 . The changelog needs some fixes, I believe. LGTM.
@@ -5,6 +5,7 @@ | |||
### Fixes | |||
|
|||
- Ensure the current GPU frame rate is always reported for concurrent transaction profiling metrics (#2929) | |||
- Improved performance serializing profiling data (#2863) |
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.
m
: I think that needs to move up a bit.
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.
Are you looking at ordering by PR number? I haven't really cared about it too much, but am happy to start.
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.
No, I thought the entry was in an already-released section, but I maybe was mistaken; sorry.
The only CI failures were codecov, but only because the test coverage was not properly assessed for the header implementations. There is indeed test coverage for them. |
Some work done in a tight loop on the profiler's sampling thread used inefficient methods to convert integers to strings. We sped this up by using
snprintf
.Targets to optimize:
-[NSNumber stringValue]
sentry_formatHexAddress
which called+[NSString stringWithFormat:]
[NSString stringWithFormat:@"%llu"
Observed a 75% reduction in cost of doing the conversions using Instruments, just in the profiler's sampling thread: https://www.notion.so/sentry/formatHexAddress-perf-optimization-6ce9e406e53b4d99aed2de591428599b?pvs=4
Converted a couple other usages in the SDK to also use the new function, so we get some other perf wins for free. There are still numerous usages of the older
sentry_formatHexAddress(NSNumber *)
that are using values from JSON-decodedNSDictionary
instances in SentryCrashReportConverter, which could be a possible target for future optimizations by the Cocoa team.TODO
main
: https://sentry-sdks.sentry.io/profiling/profile/sentry-cocoa/d112e213125c483280ba883f15dd72b0/flamechart/?colorCoding=by+system+vs+application+frame&fov=0%2C0%2C14142140416%2C48&query=&sorting=call+order&tid=259&view=top+down#skip-changelog