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(session-replay): improve multi-threading of session replay processing #5018

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

philprime
Copy link
Contributor

📜 Description

  • Changes the priorities of the background queues of session replay.
  • Refactors the async dispatch handling of session replay.

💡 Motivation and Context

Some of the methods of the session replay video processing are dispatched to a low priority background queue, which then dispatches additional AV work on another dispatch queue.

The current implementation does not respect queue priorities and can cause thread starvation, because the processing queue and the work queue are either on the same priority, or the work queue is on a lower priority.

In the worst case, the processing queue is blocked and waits for the work queue, but the work queue has a lower priority than the processing queue and is awaiting execution on the main thread.

This is what the thread inversion warning is showing.

Closes #4730

💚 How did you test it?

Manual testing.

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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.

@philprime philprime self-assigned this Mar 25, 2025
@philprime philprime moved this to In Progress in Mobile SDKs Mar 25, 2025
Copy link

github-actions bot commented Mar 25, 2025

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- improve multi-threading of session replay processing ([#5018](https://github.com/getsentry/sentry-cocoa/pull/5018))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against a27d74d

__block NSArray<SentryVideoInfo *> *videos;
__block NSError *_Nullable error;

dispatch_group_t group = dispatch_group_create();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would appreciate feedback if you asee any issues with this approach for synchronizing the async call

replayMaker.bitRate = replayOptions.replayBitRate;
replayMaker.videoScale = replayOptions.sizeScale;
replayMaker.cacheMaxSize
= (NSInteger)(shouldReplayFullSession ? replayOptions.sessionSegmentDuration + 1
: replayOptions.errorReplayDuration + 1);

dispatch_queue_attr_t attributes = dispatch_queue_attr_make_with_qos_class(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this setup to the SentryDispatchQueueWrapper to make it replaceable

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.29 ms 1241.67 ms 11.38 ms
Size 22.30 KiB 836.54 KiB 814.24 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4597906 1228.37 ms 1242.29 ms 13.92 ms
9d61bea 1243.88 ms 1254.36 ms 10.48 ms
6001822 1234.49 ms 1265.20 ms 30.71 ms
706c41f 1231.00 ms 1248.52 ms 17.52 ms
9ef729b 1228.79 ms 1245.36 ms 16.57 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms
80da9d1 1193.76 ms 1211.83 ms 18.08 ms
b9b0f0a 1251.45 ms 1257.86 ms 6.41 ms
75f7fc8 1239.31 ms 1266.12 ms 26.82 ms
0589699 1233.49 ms 1249.09 ms 15.60 ms

App size

Revision Plain With Sentry Diff
4597906 21.58 KiB 678.19 KiB 656.61 KiB
9d61bea 20.76 KiB 436.29 KiB 415.53 KiB
6001822 22.85 KiB 410.98 KiB 388.13 KiB
706c41f 21.58 KiB 699.25 KiB 677.67 KiB
9ef729b 20.76 KiB 432.88 KiB 412.12 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
80da9d1 21.58 KiB 418.45 KiB 396.86 KiB
b9b0f0a 20.76 KiB 434.94 KiB 414.18 KiB
75f7fc8 21.58 KiB 704.23 KiB 682.65 KiB
0589699 21.58 KiB 656.60 KiB 635.02 KiB

Copy link

codecov bot commented Mar 25, 2025

Codecov Report

Attention: Patch coverage is 0% with 260 lines in your changes missing coverage. Please review.

Project coverage is 8.861%. Comparing base (ba5cc98) to head (a27d74d).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...egrations/SessionReplay/SentryOnDemandReplay.swift 0.000% 183 Missing ⚠️
Sources/Sentry/SentrySessionReplayIntegration.m 0.000% 51 Missing ⚠️
...tegrations/SessionReplay/SentrySessionReplay.swift 0.000% 22 Missing ⚠️
Sources/Sentry/SentryDispatchQueueWrapper.m 0.000% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (ba5cc98) and HEAD (a27d74d). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (ba5cc98) HEAD (a27d74d)
4 1
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main    #5018        +/-   ##
=============================================
- Coverage   92.705%   8.861%   -83.844%     
=============================================
  Files          672      358       -314     
  Lines        82202    25570     -56632     
  Branches     29841       94     -29747     
=============================================
- Hits         76206     2266     -73940     
- Misses        5899    23304     +17405     
+ Partials        97        0        -97     
Files with missing lines Coverage Δ
...rations/SessionReplay/SentryReplayVideoMaker.swift 0.000% <ø> (-100.000%) ⬇️
Sources/Sentry/SentryDispatchQueueWrapper.m 52.542% <0.000%> (-47.458%) ⬇️
...tegrations/SessionReplay/SentrySessionReplay.swift 0.000% <0.000%> (-97.657%) ⬇️
Sources/Sentry/SentrySessionReplayIntegration.m 0.000% <0.000%> (-88.611%) ⬇️
...egrations/SessionReplay/SentryOnDemandReplay.swift 0.000% <0.000%> (-94.175%) ⬇️

... and 659 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 ba5cc98...a27d74d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Session Replay thread priority inversions
1 participant