-
-
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
fix: Don't use main thread for app hang screenshot and view hierarchy #3547
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1437c68 | 1244.86 ms | 1254.18 ms | 9.32 ms |
7c5d161 | 1224.38 ms | 1249.66 ms | 25.28 ms |
6001822 | 1202.61 ms | 1213.50 ms | 10.89 ms |
257c2a9 | 1239.52 ms | 1251.08 ms | 11.56 ms |
2af280d | 1246.22 ms | 1253.10 ms | 6.88 ms |
ecd9ecd | 1191.76 ms | 1216.92 ms | 25.16 ms |
ad7cec6 | 1203.22 ms | 1224.74 ms | 21.52 ms |
01a28a9 | 1237.78 ms | 1259.04 ms | 21.27 ms |
a176fc4 | 1226.24 ms | 1247.50 ms | 21.26 ms |
98752f3 | 1240.61 ms | 1259.80 ms | 19.18 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1437c68 | 22.85 KiB | 410.96 KiB | 388.11 KiB |
7c5d161 | 20.76 KiB | 414.44 KiB | 393.68 KiB |
6001822 | 22.85 KiB | 410.98 KiB | 388.13 KiB |
257c2a9 | 20.76 KiB | 401.36 KiB | 380.60 KiB |
2af280d | 20.76 KiB | 435.22 KiB | 414.46 KiB |
ecd9ecd | 20.76 KiB | 420.23 KiB | 399.47 KiB |
ad7cec6 | 20.76 KiB | 427.32 KiB | 406.55 KiB |
01a28a9 | 22.85 KiB | 405.39 KiB | 382.54 KiB |
a176fc4 | 22.84 KiB | 403.24 KiB | 380.39 KiB |
98752f3 | 20.76 KiB | 435.09 KiB | 414.33 KiB |
Previous results on branch: fix/screenshot-anr
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0a8b53c | 1204.04 ms | 1214.56 ms | 10.52 ms |
5d91312 | 1199.92 ms | 1211.47 ms | 11.55 ms |
0f7cf35 | 1222.74 ms | 1244.75 ms | 22.01 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0a8b53c | 21.58 KiB | 418.49 KiB | 396.91 KiB |
5d91312 | 21.58 KiB | 419.20 KiB | 397.61 KiB |
0f7cf35 | 21.58 KiB | 419.16 KiB | 397.57 KiB |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3547 +/- ##
=============================================
+ Coverage 89.289% 89.327% +0.037%
=============================================
Files 529 529
Lines 57737 57979 +242
Branches 20412 20740 +328
=============================================
+ Hits 51553 51791 +238
Misses 5276 5276
- Partials 908 912 +4
... and 73 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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 doing this. A couple of comments.
Tests/SentryTests/Integrations/Screenshot/SentryScreenshotIntegrationTests.swift
Show resolved
Hide resolved
Tests/SentryTests/Integrations/Screenshot/SentryScreenshotIntegrationTests.swift
Outdated
Show resolved
Hide resolved
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.
Almost, LGTM.
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, thanks @brustolin 💪
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.
Again, LGTM.
📜 Description
During an app hang event, we attempt to capture a screenshot and view hierarchy in the main thread. However, the main thread is blocked, so the event must wait for the main thread to be freed, a situation that may never occur.
💡 Motivation and Context
Close #3220
💚 How did you test it?
Unit Test
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps