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): extract methods related to profile serialization #3876

Conversation

armcknight
Copy link
Member

Following on #3875, for #3555, #skip-changelog

Extract the logic that dealt with serializing profile data and creating envelope items to SentrySerialization et al. There was a linear chain of methods/functions that would be called in this process; they have been flattened into as few functions as possible.

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

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

Project coverage is 90.529%. Comparing base (135dec6) to head (0b3796f).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3876       +/-   ##
=============================================
- Coverage   90.632%   90.529%   -0.103%     
=============================================
  Files          580       580               
  Lines        45294     45203       -91     
  Branches     16113     16059       -54     
=============================================
- Hits         41051     40922      -129     
- Misses        4173      4211       +38     
  Partials        70        70               
Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 86.792% <100.000%> (-0.364%) ⬇️
Sources/Sentry/SentryProfiler.mm 89.743% <100.000%> (+0.469%) ⬆️
Sources/Sentry/SentryTracer.m 96.650% <100.000%> (-0.247%) ⬇️
Tests/SentryProfilerTests/SentryProfilerTests.mm 98.773% <100.000%> (ø)
...es/Sentry/Profiling/SentryProfilerSerialization.mm 88.414% <88.414%> (ø)

... and 23 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 135dec6...0b3796f. Read the comment docs.

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

github-actions bot commented Apr 23, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.91 ms 1248.76 ms 22.84 ms
Size 21.58 KiB 615.29 KiB 593.71 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
be51b56 1220.84 ms 1249.36 ms 28.52 ms
ddb4778 1245.36 ms 1254.16 ms 8.80 ms
216bdf9 1202.57 ms 1215.45 ms 12.88 ms
596ccc1 1233.51 ms 1244.24 ms 10.73 ms
2f9c5f9 1218.78 ms 1239.58 ms 20.80 ms
56baaef 1241.60 ms 1260.51 ms 18.91 ms
45d3ca5 1248.27 ms 1255.48 ms 7.21 ms
626b91b 1231.67 ms 1247.13 ms 15.46 ms
b9b0f0a 1220.20 ms 1229.27 ms 9.06 ms
d3630c3 1226.55 ms 1245.23 ms 18.68 ms

App size

Revision Plain With Sentry Diff
be51b56 20.76 KiB 432.20 KiB 411.44 KiB
ddb4778 21.58 KiB 414.92 KiB 393.34 KiB
216bdf9 21.58 KiB 418.13 KiB 396.54 KiB
596ccc1 22.84 KiB 401.44 KiB 378.60 KiB
2f9c5f9 21.58 KiB 418.82 KiB 397.24 KiB
56baaef 21.58 KiB 571.91 KiB 550.33 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
626b91b 21.58 KiB 546.20 KiB 524.61 KiB
b9b0f0a 20.76 KiB 434.94 KiB 414.18 KiB
d3630c3 22.84 KiB 403.19 KiB 380.34 KiB

Previous results on branch: armcknight/feat/3555-continuous-profiling/4-refactoring/5-profile-serialization

Startup times

Revision Plain With Sentry Diff
9e3a640 1222.90 ms 1238.35 ms 15.45 ms
b813174 1231.20 ms 1251.61 ms 20.41 ms

App size

Revision Plain With Sentry Diff
9e3a640 21.58 KiB 615.29 KiB 593.71 KiB
b813174 21.58 KiB 615.38 KiB 593.80 KiB

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM!

Just one suggestion.

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.

Thanks for splitting this up. LGTM

…continuous-profiling/4-refactoring/3-renames
…/3-renames' into armcknight/feat/3555-continuous-profiling/4-refactoring/4-extract-sdk-start-profiling-tasks
…/4-extract-sdk-start-profiling-tasks' into armcknight/feat/3555-continuous-profiling/4-refactoring/5-profile-serialization
Base automatically changed from armcknight/feat/3555-continuous-profiling/4-refactoring/4-extract-sdk-start-profiling-tasks to main April 23, 2024 21:15
@armcknight armcknight merged commit 07b120c into main Apr 25, 2024
68 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/4-refactoring/5-profile-serialization branch April 25, 2024 00:19
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.

3 participants