-
-
Notifications
You must be signed in to change notification settings - Fork 372
fix: Remove symbolication that causes deadlocks #6562
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
Conversation
b87555b to
38f322d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6562 +/- ##
=============================================
+ Coverage 84.779% 84.898% +0.119%
=============================================
Files 452 451 -1
Lines 27686 27521 -165
Branches 12156 12061 -95
=============================================
- Hits 23472 23365 -107
+ Misses 4169 4113 -56
+ Partials 45 43 -2
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7273bf4 | 1202.42 ms | 1227.62 ms | 25.21 ms |
| 8d944ac | 1236.92 ms | 1254.91 ms | 18.00 ms |
| c6c1cb7 | 1235.71 ms | 1263.80 ms | 28.08 ms |
| a4c5ddc | 1239.61 ms | 1266.41 ms | 26.80 ms |
| 1bf44b4 | 1238.14 ms | 1275.45 ms | 37.31 ms |
| e0e5391 | 1191.21 ms | 1243.14 ms | 51.93 ms |
| 7dabfb9 | 1227.55 ms | 1243.94 ms | 16.39 ms |
| cbbc82c | 1246.43 ms | 1266.13 ms | 19.70 ms |
| aa96485 | 1215.37 ms | 1234.04 ms | 18.67 ms |
| 4f36ea5 | 1216.87 ms | 1246.34 ms | 29.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7273bf4 | 23.75 KiB | 908.01 KiB | 884.26 KiB |
| 8d944ac | 23.75 KiB | 919.69 KiB | 895.94 KiB |
| c6c1cb7 | 23.75 KiB | 928.15 KiB | 904.40 KiB |
| a4c5ddc | 23.75 KiB | 977.30 KiB | 953.55 KiB |
| 1bf44b4 | 23.75 KiB | 1021.20 KiB | 997.45 KiB |
| e0e5391 | 23.74 KiB | 1022.21 KiB | 998.46 KiB |
| 7dabfb9 | 23.75 KiB | 980.80 KiB | 957.05 KiB |
| cbbc82c | 23.75 KiB | 995.22 KiB | 971.47 KiB |
| aa96485 | 23.75 KiB | 874.46 KiB | 850.71 KiB |
| 4f36ea5 | 23.75 KiB | 995.08 KiB | 971.33 KiB |
Previous results on branch: removeSymbolication
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0e8838d | 1229.56 ms | 1262.41 ms | 32.85 ms |
| b97c496 | 1228.33 ms | 1259.09 ms | 30.77 ms |
| 92024ac | 1208.29 ms | 1227.73 ms | 19.45 ms |
| 2a37909 | 1215.14 ms | 1252.82 ms | 37.68 ms |
| 3c5e97a | 1226.33 ms | 1256.65 ms | 30.32 ms |
| 57eb364 | 1217.94 ms | 1254.89 ms | 36.95 ms |
| 580eec7 | 1210.89 ms | 1245.78 ms | 34.89 ms |
| c31160d | 1230.77 ms | 1264.46 ms | 33.69 ms |
| d683042 | 1220.33 ms | 1257.30 ms | 36.97 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0e8838d | 23.75 KiB | 1021.68 KiB | 997.93 KiB |
| b97c496 | 23.75 KiB | 1.00 MiB | 1001.63 KiB |
| 92024ac | 23.75 KiB | 1021.67 KiB | 997.92 KiB |
| 2a37909 | 23.75 KiB | 1021.67 KiB | 997.92 KiB |
| 3c5e97a | 24.14 KiB | 1.01 MiB | 1012.15 KiB |
| 57eb364 | 24.15 KiB | 1.01 MiB | 1012.97 KiB |
| 580eec7 | 24.14 KiB | 1.01 MiB | 1012.15 KiB |
| c31160d | 23.74 KiB | 1023.38 KiB | 999.64 KiB |
| d683042 | 24.15 KiB | 1.01 MiB | 1013.05 KiB |
1392a12 to
91847a3
Compare
b07a319 to
3d9e95a
Compare
dc9beab to
827f677
Compare
Tests/SentryTests/Integrations/ANR/SentryANRTrackingIntegrationTests.swift
Outdated
Show resolved
Hide resolved
76f0817 to
e3b3ad9
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.
We definitely need a clear changelog entry to explain that local symbolication is gone now, and we should add a decision log entry here https://github.com/getsentry/sentry-cocoa/blob/main/develop-docs/DECISIONS.md to explain why we removed local symbolication.
Also, we must check our onboarding here https://docs.sentry.io/platforms/apple/guides/ios/ and the wizard if we shouldn't mention that you need to upload dSYMs to guarantee a great experience. Otherwise, some users might be frustrated during the onboarding, because it usuallally sets options.debug = true.
And huge thanks for doing this @noahsmartin
|
Thanks @philipphofmann just made those changes? |
b2cbb17 to
778f2fe
Compare
philipphofmann
left a comment
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 would still double check if we can ditch the 0x0000000000000000. I don't think it's required. Thanks
778f2fe to
ee44c0e
Compare
ee44c0e to
29ff851
Compare
philprime
left a comment
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
Resolves #6560 (and all the other related issues)
This also has the benefit of unifying the debug and non-debug codepaths to reduce the chances that a bug affects one environment but not the other. For example, the debug behavior of buildStacktraceForCurrentThread would remove the frame for that function itself, but this wouldn't happen in non-debug (since it relied on symbolication). This PR preserves the non-debug behavior, now in all configurations
#skip-changelog
Closes #6698