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: centralize frames tracker access in SentryDependencyContainer #3119

Merged
merged 4 commits into from
Jun 26, 2023

Conversation

armcknight
Copy link
Member

In service of reworking SentryProfiler's management of GPU frame data in #3090, I need to simplify how it keeps a reference to the frames tracker.

This has bugged me for some time because we vend a singleton of the frames tracker, but then various other objects keep and pass references to it, which is unnecessary; it can just be accessed via SentryDependencyContainer. This seemed like a good opportunity to fix it everywhere.

#skip-changelog

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #3119 (cff6328) into main (6bc5ae5) will decrease coverage by 0.016%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3119       +/-   ##
=============================================
- Coverage   89.040%   89.025%   -0.016%     
=============================================
  Files          500       500               
  Lines        53910     53898       -12     
  Branches     19288     19278       -10     
=============================================
- Hits         48002     47983       -19     
- Misses        4951      4953        +2     
- Partials       957       962        +5     
Impacted Files Coverage Δ
Sources/Sentry/SentryFramesTracker.m 98.449% <ø> (-0.102%) ⬇️
.../Sentry/SentryUIViewControllerPerformanceTracker.m 98.800% <ø> (-0.005%) ⬇️
Sources/Sentry/PrivateSentrySDKOnly.m 88.888% <100.000%> (ø)
Sources/Sentry/SentryDependencyContainer.m 93.478% <100.000%> (+0.414%) ⬆️
Sources/Sentry/SentryFramesTrackingIntegration.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryProfiler.mm 85.350% <100.000%> (-0.245%) ⬇️
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.268% <100.000%> (-0.747%) ⬇️
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.033% <100.000%> (ø)
...racking/SentryFramesTrackingIntegrationTests.swift 98.630% <100.000%> (ø)
... and 5 more

... and 12 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 6bc5ae5...cff6328. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 23, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.39 ms 1256.74 ms 12.35 ms
Size 20.76 KiB 401.56 KiB 380.80 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6943de0 1230.02 ms 1235.32 ms 5.30 ms
20163bb 1248.92 ms 1258.48 ms 9.56 ms
25a5e8b 1249.18 ms 1268.42 ms 19.24 ms
8526e93 1228.24 ms 1239.14 ms 10.90 ms
154f795 1250.38 ms 1274.54 ms 24.16 ms
45d3ca5 1238.53 ms 1263.09 ms 24.55 ms
e1eed6b 1259.08 ms 1270.57 ms 11.49 ms
7e8d5fd 6249.23 ms 6253.90 ms 4.67 ms
dcec216 1238.94 ms 1261.06 ms 22.12 ms
a2af9fa 1236.62 ms 1253.12 ms 16.50 ms

App size

Revision Plain With Sentry Diff
6943de0 20.76 KiB 393.33 KiB 372.57 KiB
20163bb 20.76 KiB 426.82 KiB 406.06 KiB
25a5e8b 20.76 KiB 436.33 KiB 415.57 KiB
8526e93 20.76 KiB 420.22 KiB 399.47 KiB
154f795 20.76 KiB 435.25 KiB 414.49 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
e1eed6b 20.76 KiB 432.17 KiB 411.41 KiB
7e8d5fd 20.76 KiB 435.50 KiB 414.74 KiB
dcec216 20.76 KiB 432.88 KiB 412.11 KiB
a2af9fa 20.76 KiB 432.87 KiB 412.11 KiB

Previous results on branch: armcknight/ref/framestracker

Startup times

Revision Plain With Sentry Diff
c15ad1e 1212.96 ms 1230.06 ms 17.10 ms

App size

Revision Plain With Sentry Diff
c15ad1e 20.76 KiB 401.56 KiB 380.80 KiB

Comment on lines -27 to +29
private let fixture = Fixture()
private lazy var fixture = Fixture()
Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that with a constant instance property, it was initialized as part of a pre-test process, and then when the tests ran, the dependency container shared instance was constructed again and had a difference instance of the frames tracker, so the tests would fail. Making it lazy defers initialization until the test process itself is running.

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 89bc37d into main Jun 26, 2023
@armcknight armcknight deleted the armcknight/ref/framestracker branch June 26, 2023 23: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