Skip to content
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): put back the missing insertion of initial frame rate #3987

Merged

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented May 15, 2024

In #3932, this line of code got accidentally removed. I'm not sure if this codepath is ever actually taken. It's not currently covered in tests; to be honest the mocking around GPU timestamps is a mess, and I haven't been able to untangle it to get this particular codepath covered. I hope to make more progress on that in #3985.

But I would like to get this merged before any release goes out, in case something would actually break.

#skip-changelog, for #3555

@armcknight armcknight changed the title fix: put back the missing insertion of initial frame rate fix(profiling): put back the missing insertion of initial frame rate May 15, 2024
Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.884%. Comparing base (d914696) to head (2127882).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3987       +/-   ##
=============================================
+ Coverage   90.802%   90.884%   +0.081%     
=============================================
  Files          594       594               
  Lines        46351     46351               
  Branches     16619     16617        -2     
=============================================
+ Hits         42088     42126       +38     
+ Misses        4083      4044       -39     
- Partials       180       181        +1     
Files Coverage Δ
Sources/Sentry/SentryProfileTimeseries.mm 53.535% <0.000%> (-0.547%) ⬇️

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d914696...2127882. Read the comment docs.

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1220.96 ms 1235.23 ms 14.27 ms
Size 21.58 KiB 629.85 KiB 608.27 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dd0557f 1242.04 ms 1250.86 ms 8.82 ms
742d4b6 1217.04 ms 1229.38 ms 12.33 ms
313b1d9 1240.18 ms 1258.44 ms 18.26 ms
533859f 1237.78 ms 1249.76 ms 11.98 ms
67460f4 1244.56 ms 1255.96 ms 11.40 ms
f0ce81c 1231.06 ms 1244.79 ms 13.73 ms
d3abae0 1200.36 ms 1224.22 ms 23.87 ms
56ec5d0 1236.65 ms 1261.90 ms 25.25 ms
e324230 1254.92 ms 1262.92 ms 8.00 ms
324dc7b 1210.33 ms 1244.92 ms 34.59 ms

App size

Revision Plain With Sentry Diff
dd0557f 22.85 KiB 411.75 KiB 388.90 KiB
742d4b6 21.58 KiB 546.19 KiB 524.61 KiB
313b1d9 22.85 KiB 414.11 KiB 391.26 KiB
533859f 22.85 KiB 408.84 KiB 386.00 KiB
67460f4 20.76 KiB 426.15 KiB 405.39 KiB
f0ce81c 21.58 KiB 418.33 KiB 396.75 KiB
d3abae0 20.76 KiB 434.92 KiB 414.16 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.69 KiB
e324230 22.85 KiB 408.88 KiB 386.03 KiB
324dc7b 22.85 KiB 408.84 KiB 385.99 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@armcknight armcknight merged commit b2c9166 into main May 15, 2024
71 of 72 checks passed
@armcknight armcknight deleted the armcknight/fix/gpu-slicing-first-frame-rate-addition branch May 15, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants