-
-
Notifications
You must be signed in to change notification settings - Fork 324
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: profiling memory growth fix #3145
fix: profiling memory growth fix #3145
Conversation
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, but the tests are failing.
The test is supposed to fail in this PR. We'll merge #3135 which is the fix into this branch before merging this to main. |
- always ensure the profiler's timeout timer is scheduled on the main thread - keep references to any aborted profiler until its associated transaction is finished
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3145 +/- ##
=============================================
+ Coverage 89.108% 89.129% +0.020%
=============================================
Files 501 501
Lines 53867 53951 +84
Branches 19298 19327 +29
=============================================
+ Hits 48000 48086 +86
+ Misses 5011 4898 -113
- Partials 856 967 +111
... and 24 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
43aa39d | 1239.16 ms | 1270.42 ms | 31.26 ms |
7bc3c0d | 1259.74 ms | 1268.45 ms | 8.71 ms |
6f4d20c | 1228.67 ms | 1238.88 ms | 10.21 ms |
984eb2d | 1220.62 ms | 1235.24 ms | 14.62 ms |
03d9eb5 | 1227.35 ms | 1231.24 ms | 3.90 ms |
596ccc1 | 1221.57 ms | 1236.82 ms | 15.25 ms |
1d11695 | 1219.57 ms | 1243.52 ms | 23.95 ms |
6e342ac | 1202.98 ms | 1228.74 ms | 25.76 ms |
1bbcb9c | 1189.61 ms | 1223.50 ms | 33.89 ms |
06548c0 | 1225.58 ms | 1244.70 ms | 19.12 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
43aa39d | 20.76 KiB | 432.82 KiB | 412.06 KiB |
7bc3c0d | 20.76 KiB | 427.35 KiB | 406.59 KiB |
6f4d20c | 20.76 KiB | 431.71 KiB | 410.95 KiB |
984eb2d | 20.76 KiB | 425.77 KiB | 405.01 KiB |
03d9eb5 | 20.76 KiB | 436.33 KiB | 415.57 KiB |
596ccc1 | 22.84 KiB | 401.44 KiB | 378.60 KiB |
1d11695 | 20.76 KiB | 401.60 KiB | 380.84 KiB |
6e342ac | 20.76 KiB | 436.66 KiB | 415.90 KiB |
1bbcb9c | 20.76 KiB | 426.10 KiB | 405.34 KiB |
06548c0 | 20.76 KiB | 427.35 KiB | 406.59 KiB |
Following on from #3133, write a test that exercises the situation that is causing the memory growth bug. We actually already had a test that sets up the scenario, but prior to being able to mock backtraces, which that linked PR provides the ability to do, we were not able to distinguish which profile data comes from which profiler–and therefore tell that one had been lost. Now that we can disambiguate them with different mock data, we can ensure both are preserved. This test should fail in this PR and then pass in #3135 with the fixes.
#skip-changelog