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: cpu usage readings for profiling #3214

Merged
merged 8 commits into from
Aug 15, 2023

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Aug 10, 2023

We noticed as we were iterating that we weren't calculating the CPU usage correctly. After fixing it, we realized that this reading doesn't report CPU usage per core only for the process using the API; it shows usage across all processes. We decided to use the API that reports a total CPU usage per thread, to guarantee process confinement, and sum these into a single number. This has a benefit of matching how Android reports this metric.

💚 How did you test it?

Visual inspection of the Xcode debugger and matching it to what we see in the flamechart viewer.

Xcode debugger:
image

From the profile:
image

A bit hard to compare due to limitations sizing them to match scale, but the profiler's version basically matches the same trends, except for the one spike shown in the Xcode debugger. Also watched the Xcode "speedometer" with the single CPU % and numbers logged from the profiling code throughout the test, and they basically matched while I changed the amount of CPU work to do in the test app:
image

@armcknight armcknight force-pushed the armcknight/fix/cpu-usage-readings branch from ee3a859 to 63a498f Compare August 10, 2023 05:42
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #3214 (6e9ef71) into main (b066509) will decrease coverage by 0.018%.
The diff coverage is 69.811%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3214       +/-   ##
=============================================
- Coverage   89.199%   89.182%   -0.018%     
=============================================
  Files          502       502               
  Lines        54081     54041       -40     
  Branches     19413     19394       -19     
=============================================
- Hits         48240     48195       -45     
- Misses        4988      4991        +3     
- Partials       853       855        +2     
Files Changed Coverage Δ
...SentryProfilerTests/SentrySystemWrapperTests.swift 85.714% <0.000%> (+2.380%) ⬆️
Sources/Sentry/SentrySystemWrapper.mm 60.655% <64.705%> (-13.258%) ⬇️
Sources/Sentry/SentryMetricProfiler.mm 92.380% <85.714%> (-2.578%) ⬇️
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.517% <100.000%> (-0.014%) ⬇️

... and 16 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 b066509...6e9ef71. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Aug 10, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.27 ms 1254.71 ms 12.44 ms
Size 22.84 KiB 402.63 KiB 379.79 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b385962 1195.85 ms 1221.63 ms 25.78 ms
1bf8571 1215.31 ms 1232.48 ms 17.17 ms
60dd0f5 1238.98 ms 1254.48 ms 15.50 ms
6ad07ae 1240.76 ms 1242.98 ms 2.22 ms
c319795 1205.12 ms 1231.20 ms 26.08 ms
8f397a7 1251.82 ms 1268.34 ms 16.52 ms
46f5eb8 1212.27 ms 1231.42 ms 19.15 ms
dc0db9e 1246.06 ms 1260.46 ms 14.40 ms
25e65a5 1249.08 ms 1277.78 ms 28.70 ms
b8dd0fc 1235.57 ms 1253.12 ms 17.55 ms

App size

Revision Plain With Sentry Diff
b385962 20.76 KiB 399.69 KiB 378.93 KiB
1bf8571 20.76 KiB 437.12 KiB 416.36 KiB
60dd0f5 20.76 KiB 393.37 KiB 372.61 KiB
6ad07ae 20.76 KiB 424.69 KiB 403.93 KiB
c319795 20.76 KiB 431.99 KiB 411.22 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
46f5eb8 20.76 KiB 432.37 KiB 411.61 KiB
dc0db9e 20.76 KiB 419.62 KiB 398.86 KiB
25e65a5 22.85 KiB 403.19 KiB 380.34 KiB
b8dd0fc 20.76 KiB 401.39 KiB 380.63 KiB

Previous results on branch: armcknight/fix/cpu-usage-readings

Startup times

Revision Plain With Sentry Diff
9ce25d7 1239.69 ms 1251.94 ms 12.25 ms
ed8a1f8 1240.08 ms 1261.12 ms 21.04 ms

App size

Revision Plain With Sentry Diff
9ce25d7 22.84 KiB 402.82 KiB 379.97 KiB
ed8a1f8 22.84 KiB 402.65 KiB 379.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

@armcknight armcknight merged commit 7f14650 into main Aug 15, 2023
64 of 66 checks passed
@armcknight armcknight deleted the armcknight/fix/cpu-usage-readings branch August 15, 2023 21:45
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