fix: Profiling memory leaks #3055
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Passing the std::function (which captures the `SentryProfilingState`) to the thread constructor creates a copy of the std::function that appears to not be destructed when the thread exits - unclear if this is expected behavior, but passing it as a const ref instead of a copy fixes the leak.
This is the same fix as the other leak fix, for the same reason - std::thread leaks the parameters even after the thread exits
Closed
indragiek
commented
May 24, 2023
| }; | ||
| NSString *const labelNSStr = | ||
| [NSString stringWithUTF8String:backtrace.queueMetadata.label->c_str()]; | ||
| // -[NSString stringWithUTF8String:] can return `nil` for malformed string data |
Member
Author
There was a problem hiding this comment.
Here's a repro of this happening:
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
=============================================
+ Coverage 88.844% 88.865% +0.021%
=============================================
Files 492 492
Lines 52976 53017 +41
Branches 18970 18992 +22
=============================================
+ Hits 47066 47114 +48
+ Misses 4953 4948 -5
+ Partials 957 955 -2
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2af280d | 1219.33 ms | 1239.98 ms | 20.65 ms |
| 1bbcb9c | 1189.61 ms | 1223.50 ms | 33.89 ms |
| 1db04d8 | 1250.20 ms | 1258.12 ms | 7.92 ms |
| 0001a09 | 1223.90 ms | 1249.56 ms | 25.66 ms |
| 8526e93 | 1228.24 ms | 1239.14 ms | 10.90 ms |
| c0b4b71 | 1218.16 ms | 1251.28 ms | 33.12 ms |
| 8f397a7 | 1196.55 ms | 1226.82 ms | 30.27 ms |
| bd2afa6 | 1192.31 ms | 1210.37 ms | 18.05 ms |
| ecd9ecd | 1191.76 ms | 1216.92 ms | 25.16 ms |
| 1bf8571 | 1215.31 ms | 1232.48 ms | 17.17 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2af280d | 20.76 KiB | 435.22 KiB | 414.46 KiB |
| 1bbcb9c | 20.76 KiB | 426.10 KiB | 405.34 KiB |
| 1db04d8 | 20.76 KiB | 435.50 KiB | 414.74 KiB |
| 0001a09 | 20.76 KiB | 433.19 KiB | 412.43 KiB |
| 8526e93 | 20.76 KiB | 420.22 KiB | 399.47 KiB |
| c0b4b71 | 20.76 KiB | 430.98 KiB | 410.22 KiB |
| 8f397a7 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
| bd2afa6 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
| ecd9ecd | 20.76 KiB | 420.23 KiB | 399.47 KiB |
| 1bf8571 | 20.76 KiB | 437.12 KiB | 416.36 KiB |
philipphofmann
approved these changes
May 24, 2023
Member
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks, @indragiek. LGTM.
armcknight
approved these changes
May 24, 2023
| numSamples_ = 0; | ||
| thread_ = std::thread(samplingThreadMain, port_, clock_, delaySpec_, cache_, callback_, | ||
| std::ref(numSamples_), onThreadStart); | ||
| thread_ = std::thread(samplingThreadMain, port_, clock_, delaySpec_, std::cref(cache_), |
Member
There was a problem hiding this comment.
TIL std::ref and std::cref, nice! I was trying to do this with & which wouldn't compile 😭
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Description
This fixes most of the memory leaks reported in #2980. I checked before/after the fixes using Instruments.
Before:
After:
There is 1 memory leak left to fix that I couldn't find a solution for yet, but the other leaks are fixed.
The root of the problem seems to be that
std::threaddoes not correctly deallocate parameters that are passed to the newly spawned thread. It's unclear if this is expected behavior or not, but the fix was simple: pass parameters that were being copied (and leaked) by reference.There's also one more unrelated fix to a crash that I found while debugging the leaks - it turns out
-[NSString stringWithUTF8String:]can returnnilfor malformed data and we weren't checking for nil.💚 How did you test it?
Check before/after the fix in Instruments, run all existing unit tests.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.🔮 Next steps