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

impr: Speed up SentryNetworkTracker #4366

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Speed up SentryNetworkTracker for multiple HTTP requests in parallel by removing unnecessary locks.

@brustolin, you added these locks in the past. Can you recall if there was a good reason for that, except a race condition? I just want to double-check, that we don't break anything.

💡 Motivation and Context

This surfaced while investigating a potential slow down for a specific UIViewController when enabling the Sentry Cocoa SDK.

💚 How did you test it?

Running unit performance tests locally. Sadly, they don't work in CI, so adding them to the repository adds no value.

📝 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

Speed up SentryNetworkTracker for multiple HTTP requests in parallel by
removing unnecessary locks.
Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.512%. Comparing base (2233968) to head (976c1d0).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4366       +/-   ##
=============================================
+ Coverage   91.502%   91.512%   +0.010%     
=============================================
  Files          627       627               
  Lines        50624     50618        -6     
  Branches     18295     18302        +7     
=============================================
  Hits         46322     46322               
+ Misses        4209      4204        -5     
+ Partials        93        92        -1     
Files with missing lines Coverage Δ
Sources/Sentry/SentryNetworkTracker.m 96.742% <100.000%> (-0.073%) ⬇️

... and 4 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 2233968...976c1d0. Read the comment docs.

Copy link

github-actions bot commented Sep 24, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNetworkTracker.m

@philipphofmann philipphofmann merged commit 3138592 into main Sep 24, 2024
63 of 65 checks passed
@philipphofmann philipphofmann deleted the impr/avoid-locks-for-network-tracker branch September 24, 2024 13:42
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