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

ref(profiling): move sliceGPUData to timeseries code unit; prefix C functions #3872

Conversation

armcknight
Copy link
Member

Taking some time to clean up the implementation of SentryProfiler and organize it a little better before moving forward with adding the continuous profiling implementation.

This moves the sliceGPU function out of SentryProfiler.mm and into SentryProfilerTimeseries.mm. Also prefix the C function names with _sentry as in #3862

for #3555 #skip-changelog

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

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

Project coverage is 90.635%. Comparing base (94e1968) to head (390a859).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3872       +/-   ##
=============================================
- Coverage   90.639%   90.635%   -0.005%     
=============================================
  Files          579       579               
  Lines        45298     45297        -1     
  Branches     16121     16124        +3     
=============================================
- Hits         41058     41055        -3     
- Misses        4170      4172        +2     
  Partials        70        70               
Files Coverage Δ
Sources/Sentry/SentryProfiler.mm 87.209% <100.000%> (+1.875%) ⬆️
Tests/SentryProfilerTests/SentryProfilerTests.mm 98.773% <100.000%> (ø)
Sources/Sentry/SentryProfileTimeseries.mm 57.831% <64.516%> (+3.985%) ⬆️

... 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 94e1968...390a859. Read the comment docs.

@armcknight armcknight changed the title ref: move sliceGPUData to timeseries code unit; prefix C functions ref(profiling): move sliceGPUData to timeseries code unit; prefix C functions Apr 23, 2024
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1215.45 ms 1231.07 ms 15.62 ms
Size 21.58 KiB 614.71 KiB 593.13 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
becc941 1221.90 ms 1240.37 ms 18.47 ms
f8833c4 1229.69 ms 1236.45 ms 6.76 ms
8f397a7 1219.12 ms 1236.67 ms 17.55 ms
7c37d8e 1256.00 ms 1259.36 ms 3.36 ms
7fb7afb 1227.73 ms 1243.16 ms 15.43 ms
6ad07ae 1240.76 ms 1242.98 ms 2.22 ms
5b6694b 1221.71 ms 1259.06 ms 37.35 ms
1437c68 1244.86 ms 1254.18 ms 9.32 ms
154f795 1250.38 ms 1274.54 ms 24.16 ms
7bb0873 1226.18 ms 1247.30 ms 21.12 ms

App size

Revision Plain With Sentry Diff
becc941 21.58 KiB 419.82 KiB 398.24 KiB
f8833c4 21.58 KiB 422.66 KiB 401.08 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
7c37d8e 20.76 KiB 426.86 KiB 406.09 KiB
7fb7afb 20.76 KiB 419.70 KiB 398.94 KiB
6ad07ae 20.76 KiB 424.69 KiB 403.93 KiB
5b6694b 20.76 KiB 426.11 KiB 405.34 KiB
1437c68 22.85 KiB 410.96 KiB 388.11 KiB
154f795 20.76 KiB 435.25 KiB 414.49 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 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 6e31b7c into main Apr 23, 2024
69 of 71 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/4-refactoring/1-timeseries-functions branch April 23, 2024 18:09
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
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