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: Reading applicationState in the background #3372

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Nov 2, 2023

Keep track of application state by notification instead of reading directly from UIApplication. This way we avoid using UIApplication in the background thread when detecting an ANR.

#skip-changelog

Copy link

github-actions bot commented Nov 2, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1232.50 ms 1248.00 ms 15.50 ms
Size 22.85 KiB 411.93 KiB 389.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a176fc4 1226.24 ms 1247.50 ms 21.26 ms
51307b7 1223.08 ms 1240.76 ms 17.68 ms
8f212af 1246.41 ms 1270.98 ms 24.57 ms
621ba9b 1190.66 ms 1230.84 ms 40.18 ms
56ec5d0 1236.65 ms 1261.90 ms 25.25 ms
c6773e5 1222.48 ms 1240.02 ms 17.54 ms
98cca71 1230.43 ms 1252.36 ms 21.93 ms
5b6694b 1221.71 ms 1259.06 ms 37.35 ms
69d8759 1229.88 ms 1240.45 ms 10.57 ms
d253cdf 1231.61 ms 1259.52 ms 27.91 ms

App size

Revision Plain With Sentry Diff
a176fc4 22.84 KiB 403.24 KiB 380.39 KiB
51307b7 22.85 KiB 407.63 KiB 384.78 KiB
8f212af 22.84 KiB 403.14 KiB 380.29 KiB
621ba9b 20.76 KiB 414.45 KiB 393.69 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.69 KiB
c6773e5 20.76 KiB 435.25 KiB 414.49 KiB
98cca71 22.85 KiB 411.14 KiB 388.29 KiB
5b6694b 20.76 KiB 426.11 KiB 405.34 KiB
69d8759 20.76 KiB 393.05 KiB 372.29 KiB
d253cdf 20.76 KiB 427.66 KiB 406.90 KiB

Previous results on branch: fix/reading-applicationState

Startup times

Revision Plain With Sentry Diff
acd1676 1207.94 ms 1226.78 ms 18.84 ms
bf85bdd 1258.86 ms 1276.78 ms 17.92 ms
dc0d331 1246.68 ms 1256.45 ms 9.77 ms

App size

Revision Plain With Sentry Diff
acd1676 22.85 KiB 411.80 KiB 388.95 KiB
bf85bdd 22.85 KiB 411.80 KiB 388.95 KiB
dc0d331 22.85 KiB 411.93 KiB 389.08 KiB

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #3372 (75e0ce9) into main (dd0557f) will increase coverage by 0.054%.
The diff coverage is 96.153%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3372       +/-   ##
=============================================
+ Coverage   89.181%   89.236%   +0.054%     
=============================================
  Files          500       500               
  Lines        54683     54730       +47     
  Branches     19620     19640       +20     
=============================================
+ Hits         48767     48839       +72     
- Misses        4928      5021       +93     
+ Partials       988       870      -118     
Files Coverage Δ
Sources/Sentry/SentryUIApplication.m 93.442% <100.000%> (+3.968%) ⬆️
Tests/SentryTests/SentryUIApplicationTests.swift 100.000% <100.000%> (ø)
...Tests/Helper/TestNSNotificationCenterWrapper.swift 66.666% <50.000%> (+9.523%) ⬆️

... and 34 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 dd0557f...75e0ce9. Read the comment docs.

@brustolin brustolin marked this pull request as ready for review November 2, 2023 15:25
@brustolin brustolin requested a review from armcknight as a code owner November 2, 2023 15:25
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.

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@

- Stop sending empty thread names (#3361)
- Work around edge case with a thread info kernel call sometimes returning invalid data, leading to a crash (#3364)
- Reading applicationState in the background (#3372)
Copy link
Member

Choose a reason for hiding this comment

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

m: I don't this requires a changelog entry as it fixes a fix 😄

CHANGELOG.md Outdated Show resolved Hide resolved
@brustolin brustolin merged commit 5f8ee7a into main Nov 2, 2023
32 checks passed
@brustolin brustolin deleted the fix/reading-applicationState branch November 2, 2023 16:53
@philipphofmann philipphofmann mentioned this pull request Nov 3, 2023
1 task
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