-
-
Notifications
You must be signed in to change notification settings - Fork 340
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: avoid inserting a nil into the gpu data dictionary literal #3092
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3092 +/- ##
=============================================
- Coverage 88.958% 88.906% -0.053%
=============================================
Files 495 494 -1
Lines 53464 53164 -300
Branches 19139 18833 -306
=============================================
- Hits 47561 47266 -295
- Misses 4944 5049 +105
+ Partials 959 849 -110
... and 88 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2405ba5 | 1248.37 ms | 1259.30 ms | 10.93 ms |
7f691b5 | 1233.94 ms | 1243.80 ms | 9.86 ms |
dacf894 | 1223.96 ms | 1250.41 ms | 26.45 ms |
6943de0 | 1215.83 ms | 1253.22 ms | 37.39 ms |
60dd0f5 | 1244.96 ms | 1268.98 ms | 24.02 ms |
2b4a663 | 1220.55 ms | 1249.98 ms | 29.43 ms |
2af280d | 1246.22 ms | 1253.10 ms | 6.88 ms |
efb2222 | 1256.44 ms | 1278.90 ms | 22.46 ms |
e2abb0d | 1235.08 ms | 1257.00 ms | 21.92 ms |
85b619d | 1252.51 ms | 1275.33 ms | 22.82 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2405ba5 | 20.76 KiB | 435.23 KiB | 414.47 KiB |
7f691b5 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
dacf894 | 20.76 KiB | 426.34 KiB | 405.58 KiB |
6943de0 | 20.76 KiB | 393.33 KiB | 372.57 KiB |
60dd0f5 | 20.76 KiB | 393.36 KiB | 372.60 KiB |
2b4a663 | 20.76 KiB | 436.65 KiB | 415.89 KiB |
2af280d | 20.76 KiB | 435.22 KiB | 414.46 KiB |
efb2222 | 20.76 KiB | 424.45 KiB | 403.69 KiB |
e2abb0d | 20.76 KiB | 434.72 KiB | 413.96 KiB |
85b619d | 20.76 KiB | 426.11 KiB | 405.35 KiB |
Previous results on branch: armcknight/fix/slice-gpu-crash-nil-check
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
68c4d5c | 1201.82 ms | 1215.96 ms | 14.14 ms |
950841b | 1222.39 ms | 1247.10 ms | 24.71 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
68c4d5c | 20.76 KiB | 393.43 KiB | 372.67 KiB |
950841b | 20.76 KiB | 393.43 KiB | 372.67 KiB |
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.
It looks good, but I have one question below.
@@ -143,7 +143,7 @@ | |||
@"value" : obj[@"value"], | |||
}]; | |||
}]; | |||
if (useMostRecentRecording && slicedGPUEntries.count == 0) { | |||
if (useMostRecentRecording && slicedGPUEntries.count == 0 && nearestPredecessorValue != nil) { |
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: Does it make sense to have a test to avoid regression?
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 don't think so for several reasons:
- the only way this will regress is if someone removes the nil check
- the testing is disproportionately difficult to implement
- i don't even have a repro on this yet
- i'm working on a more holistic solution in fix: data race collecting GPU slow/frozen frames and framerates #3090
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.
+1 to fixing the immediate crash right now and working on the correct fix + tests in the other PR @armcknight linked
I updated this PR to only perform the nil check, without the refactor. I will make that other change in a separate PR. |
📜 Description
Stops the crash from occurring that was reported in #3082
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
- [ ] I added tests to verify the changes.sendDefaultPII
is enabled.- [ ] I updated the docs if needed.- [ ] Review from the native team if needed.🔮 Next steps