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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ignore FramesTracker thread data races #3922

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

philipphofmann
Copy link
Member

馃摐 Description

The ThreadSanitizer reports data races for the SentryFramesTracker, which we can ignore.

I defined a macro for ignoring thread sanitizer warnings directly in the code. I will open another PR to replace the ThreadSanitizer.sup by ignoring the warnings directly in the code.

馃挕 Motivation and Context

Fixes GH-3702

馃挌 How did you test it?

Running the iOS-Swift project with the thread sanitizer enabled.

馃摑 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

馃敭 Next steps

The ThreadSanitizer reports data races for the SentryFramesTracker,
which we can ignore.

Fixes GH-3702
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

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

Project coverage is 90.692%. Comparing base (f96d41a) to head (48cd411).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3922       +/-   ##
=============================================
- Coverage   90.709%   90.692%   -0.018%     
=============================================
  Files          582       582               
  Lines        45520     45521        +1     
  Branches     16212     16220        +8     
=============================================
- Hits         41291     41284        -7     
+ Misses        4159      4057      -102     
- Partials        70       180      +110     
Files Coverage 螖
Sources/Sentry/include/SentryInternalDefines.h 66.666% <酶> (酶)
Sources/Sentry/SentryFramesTracker.m 99.173% <50.000%> (-0.827%) 猬囷笍

... and 32 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 f96d41a...48cd411. Read the comment docs.

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.

Im ok with this solution, just wondering if someone could see this and say that we're hiding problems in our SDK.

@philipphofmann
Copy link
Member Author

Im ok with this solution, just wondering if someone could see this and say that we're hiding problems in our SDK.

@brustolin, that's why I added the explanation:

This class ignores a couple of methods for the thread sanitizer. We intentionally accept several data races in this class, a decision that is driven by the fact that the code always writes on the main thread. This approach, while it may not provide 100% correct frame statistic for background spans, significantly reduces the overhead of synchronization, thereby enhancing performance.

Do you still believe users could think we're hiding problems?

Copy link

github-actions bot commented Apr 30, 2024

Performance metrics 馃殌

Plain With Sentry Diff
Startup time 1234.08 ms 1249.08 ms 15.00 ms
Size 21.58 KiB 616.76 KiB 595.18 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
60bfc91 1251.29 ms 1261.67 ms 10.39 ms
f0ce81c 1231.06 ms 1244.79 ms 13.73 ms
ac614f1 1235.61 ms 1265.60 ms 29.99 ms
5ab598f 1213.90 ms 1240.45 ms 26.55 ms
60dd0f5 1247.35 ms 1267.59 ms 20.24 ms
d61b939 1238.61 ms 1240.08 ms 1.47 ms
160a073 1260.72 ms 1270.10 ms 9.38 ms
135dec6 1231.06 ms 1251.69 ms 20.63 ms
d40512b 1231.12 ms 1244.54 ms 13.42 ms
67460f4 1244.56 ms 1255.96 ms 11.40 ms

App size

Revision Plain With Sentry Diff
60bfc91 20.76 KiB 434.94 KiB 414.18 KiB
f0ce81c 21.58 KiB 418.33 KiB 396.75 KiB
ac614f1 22.85 KiB 408.88 KiB 386.03 KiB
5ab598f 22.84 KiB 403.17 KiB 380.33 KiB
60dd0f5 20.76 KiB 393.37 KiB 372.60 KiB
d61b939 22.85 KiB 407.63 KiB 384.78 KiB
160a073 22.85 KiB 408.88 KiB 386.03 KiB
135dec6 21.58 KiB 615.19 KiB 593.61 KiB
d40512b 20.76 KiB 427.77 KiB 407.00 KiB
67460f4 20.76 KiB 426.15 KiB 405.39 KiB

Previous results on branch: fix/frames-tracker-data-races

Startup times

Revision Plain With Sentry Diff
e58332a 1239.98 ms 1256.73 ms 16.76 ms
47ec27d 1241.94 ms 1257.24 ms 15.31 ms

App size

Revision Plain With Sentry Diff
e58332a 21.58 KiB 616.72 KiB 595.14 KiB
47ec27d 21.58 KiB 616.71 KiB 595.13 KiB

@brustolin
Copy link
Contributor

Do you still believe users could think we're hiding problems?

There will be always someone out there to complain. 馃榿
Lets merge this!!

@philipphofmann philipphofmann merged commit e887ddc into main Apr 30, 2024
67 of 70 checks passed
@philipphofmann philipphofmann deleted the fix/frames-tracker-data-races branch April 30, 2024 14:35
philipphofmann added a commit that referenced this pull request May 6, 2024
The ThreadSanitizer reports data races for the SentryFramesTracker,
which we can ignore.

Fixes GH-3702
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
The ThreadSanitizer reports data races for the SentryFramesTracker,
which we can ignore.

Fixes getsentryGH-3702
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
The ThreadSanitizer reports data races for the SentryFramesTracker,
which we can ignore.

Fixes getsentryGH-3702
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.

Data race in -[SentryFramesTracker displayLinkCallback] at 0x10cd42f70
2 participants