-
-
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
ref: Session replay performance for SwiftUI #4419
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1529984 | 1229.36 ms | 1247.45 ms | 18.09 ms |
814655e | 1230.51 ms | 1243.13 ms | 12.61 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
1529984 | 21.58 KiB | 730.64 KiB | 709.06 KiB |
814655e | 21.58 KiB | 705.77 KiB | 684.19 KiB |
Previous results on branch: test/SR-Performance
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0e4d5e9 | 1210.56 ms | 1224.63 ms | 14.07 ms |
e2d4ed4 | 1237.67 ms | 1246.39 ms | 8.72 ms |
13f95b7 | 1232.81 ms | 1251.57 ms | 18.76 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0e4d5e9 | 21.58 KiB | 706.83 KiB | 685.25 KiB |
e2d4ed4 | 21.58 KiB | 706.83 KiB | 685.25 KiB |
13f95b7 | 21.58 KiB | 706.39 KiB | 684.81 KiB |
@@ -39,6 +39,10 @@ struct RedactRegion { | |||
self.type = type | |||
self.color = color | |||
} | |||
|
|||
static func == (lhs: RedactRegion, rhs: RedactRegion) -> Bool { | |||
lhs.size == rhs.size && lhs.transform == rhs.transform && lhs.type == rhs.type |
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.
h
: I guess you added this so you can compare the regions easily here:
guard region != latestRegion && imageRect.contains(path.boundingBoxOfPath) else { continue } |
I'm not a fan of this cause the equals here doesn't include the color. This can be very confusing. I would prefer adding a helper method or something different if you only want to check if to redact regions are the same without considering the color.
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.
Yeah, we can do that.
for region in redact { | ||
let rect = CGRect(origin: CGPoint.zero, size: region.size) | ||
var transform = region.transform | ||
let path = CGPath(rect: rect, transform: &transform) | ||
|
||
defer { latestRegion = region } | ||
|
||
guard region != latestRegion && imageRect.contains(path.boundingBoxOfPath) else { continue } |
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.
m
: It seems like this speeds up the performance significantly. Maybe it's worth adding a comment for this, cause we don't have a test to validate that we do this.
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 do have a test to validate this.
Its the only test added to this PR
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/SR-Start-Stop #4419 +/- ##
========================================================
+ Coverage 90.104% 91.301% +1.196%
========================================================
Files 607 610 +3
Lines 49378 49696 +318
Branches 17443 17903 +460
========================================================
+ Hits 44492 45373 +881
+ Misses 4793 4231 -562
+ Partials 93 92 -1
... and 100 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
📜 Description
Improved Performance for SwiftUI session replay
💚 How did you test it?
Samples
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.