-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
impr: Reduce memory usage of storing envelopes #4219
impr: Reduce memory usage of storing envelopes #4219
Conversation
Remove the error parameter for dataWithEnvelope which the method doesn't use.
Reduce the memory footprint when storing envelopes to disk by writing the envelopes in chunks by using an NSFileHandle. This addresses one part of GH-3630.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4219 +/- ##
=============================================
- Coverage 91.434% 91.376% -0.058%
=============================================
Files 611 610 -1
Lines 49103 49029 -74
Branches 17732 17669 -63
=============================================
- Hits 44897 44801 -96
- Misses 4113 4135 +22
Partials 93 93
... and 20 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 this.
This will help a little bit.
For a next step we should serialize the items, headers and envelope direct to file instead of data and then write the data to file.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a2af9fa | 1236.62 ms | 1253.12 ms | 16.50 ms |
7cd187e | 1239.02 ms | 1261.42 ms | 22.40 ms |
3297d6e | 1231.56 ms | 1254.12 ms | 22.56 ms |
d0deebd | 1235.17 ms | 1255.54 ms | 20.37 ms |
3cb68af | 1221.15 ms | 1238.40 ms | 17.25 ms |
0589699 | 1226.90 ms | 1243.96 ms | 17.06 ms |
efd8ddb | 1208.88 ms | 1234.12 ms | 25.24 ms |
8b39743 | 1258.51 ms | 1263.26 ms | 4.75 ms |
9fa5d27 | 1219.86 ms | 1221.71 ms | 1.85 ms |
c8dbe73 | 1230.98 ms | 1253.37 ms | 22.39 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a2af9fa | 20.76 KiB | 432.87 KiB | 412.11 KiB |
7cd187e | 20.76 KiB | 401.65 KiB | 380.89 KiB |
3297d6e | 21.58 KiB | 418.44 KiB | 396.86 KiB |
d0deebd | 21.58 KiB | 542.28 KiB | 520.69 KiB |
3cb68af | 20.76 KiB | 401.60 KiB | 380.84 KiB |
0589699 | 21.58 KiB | 656.60 KiB | 635.02 KiB |
efd8ddb | 21.58 KiB | 678.19 KiB | 656.61 KiB |
8b39743 | 22.85 KiB | 413.98 KiB | 391.13 KiB |
9fa5d27 | 20.76 KiB | 393.37 KiB | 372.61 KiB |
c8dbe73 | 21.58 KiB | 615.91 KiB | 594.33 KiB |
Previous results on branch: impr/reduce-memory-footprint-of-storing-envelope
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
871e525 | 1221.31 ms | 1237.57 ms | 16.26 ms |
f825c91 | 1210.26 ms | 1227.81 ms | 17.55 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
871e525 | 21.58 KiB | 694.83 KiB | 673.25 KiB |
f825c91 | 21.58 KiB | 694.83 KiB | 673.25 KiB |
Yes, exactly. |
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.
Great change! One question.
This reverts commit 804fd04.
This reverts commit 804fd04.
Reverting #4219 to further investigation
📜 Description
Reduce the memory footprint when storing envelopes to disk by writing
the envelopes in chunks by using an NSFileHandle. This addresses one
part of #3630.
💡 Motivation and Context
This addresses one
part of #3630.
💚 How did you test it?
I tested the functionality with unit tests and for the improved memory footprint by storing an envelope with two attachments, each around 10MB, and checked the physical memory peak with XCTest.
The improved version shows a peak of
42794.662 kB
and the existing version67832.691 kB
. The improved version has around 20 MB smaller physical memory peak, which is roughly the size of the two added attachments. This is because the improved version doesn't duplicate the NSData of the attachment before writing it to disk, but instead directly writes it to disk.You can check the full test case here, which I didn't commit because CI can't validate the memory peaks as it requires a baseline profile which doesn't work in CI.
Full Test Case
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps