fix(session-replay): improve multi-threading of session replay processing #5018
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,8 @@ | |
| // replay absolutely needs segment 0 to make replay work. | ||
| BOOL _rateLimited; | ||
| id<SentryCurrentDateProvider> _dateProvider; | ||
| SentryDispatchQueueWrapper *_replayProcessingQueue; | ||
| SentryDispatchQueueWrapper *_replayAssetWorkerQueue; | ||
| } | ||
|
|
||
| - (instancetype)init | ||
|
|
@@ -121,6 +123,19 @@ | |
| } | ||
|
|
||
| _notificationCenter = SentryDependencyContainer.sharedInstance.notificationCenterWrapper; | ||
| _dateProvider = SentryDependencyContainer.sharedInstance.dateProvider; | ||
|
|
||
| // The asset worker queue is used to work on video and frames data. | ||
| // Use a relative priority of -1 to make it lower than the default background priority. | ||
| _replayAssetWorkerQueue = [SentryDispatchQueueWrapper | ||
| createBackgroundDispatchQueueWithName:"io.sentry.session-replay.asset-worker" | ||
| relativePriority:-1]; | ||
|
Comment on lines
+128
to
+132
There was a problem hiding this comment.
sentry-cocoa/Sources/Sentry/SentryDispatchQueueWrapper.m Lines 10 to 13 in 8856e5a A test should fail if these priorities don't match our requirements here. If you need ideas for testing this properly, please let me know. There was a problem hiding this comment. I would highly appreciate an example how you would test the priorities of queues. There was a problem hiding this comment. Using a factory method that I can swap out in the tests or something like that. Then I can assert that I call the factory with the correct parameters. |
||
| // The dispatch queue is used to asynchronously wait for the asset worker queue to finish its | ||
| // work. To avoid a deadlock, the priority of the processing queue must be lower than the asset | ||
| // worker queue. Use a relative priority of -2 to make it lower than the asset worker queue. | ||
| _replayProcessingQueue = [SentryDispatchQueueWrapper | ||
| createBackgroundDispatchQueueWithName:"io.sentry.session-replay.processing" | ||
|
Comment on lines
+130
to
+137
There was a problem hiding this comment.
|
||
| relativePriority:-2]; | ||
|
|
||
| // The asset worker queue is used to work on video and frames data. | ||
|
|
||
|
|
@@ -192,7 +207,10 @@ | |
| } | ||
|
|
||
| SentryOnDemandReplay *resumeReplayMaker = | ||
| [[SentryOnDemandReplay alloc] initWithContentFrom:lastReplayURL.path | ||
| processingQueue:_replayProcessingQueue | ||
| assetWorkerQueue:_replayAssetWorkerQueue | ||
| dateProvider:_dateProvider]; | ||
| resumeReplayMaker.bitRate = _replayOptions.replayBitRate; | ||
| resumeReplayMaker.videoScale = _replayOptions.sizeScale; | ||
|
|
||
|
|
@@ -208,10 +226,16 @@ | |
| NSArray<SentryVideoInfo *> *videos = [resumeReplayMaker createVideoWithBeginning:beginning | ||
| end:end | ||
| error:&error]; | ||
|
|
||
| // Either error or videos should be set. | ||
| if (error != nil) { | ||
| SENTRY_LOG_ERROR(@"Could not create replay video, reason: %@", error); | ||
| return; | ||
| } | ||
| if (videos == nil) { | ||
| SENTRY_LOG_ERROR(@"Could not create replay video, reason: no videos available"); | ||
| return; | ||
| } | ||
|
|
||
| // For each segment we need to create a new event with the video. | ||
| int _segmentId = segmentId; | ||
|
|
@@ -322,30 +346,27 @@ | |
| error:nil]; | ||
| } | ||
|
|
||
| SentryOnDemandReplay *replayMaker = | ||
| [[SentryOnDemandReplay alloc] initWithOutputPath:docs.path | ||
| processingQueue:_replayProcessingQueue | ||
| assetWorkerQueue:_replayAssetWorkerQueue | ||
| dateProvider:_dateProvider]; | ||
| replayMaker.bitRate = replayOptions.replayBitRate; | ||
| replayMaker.videoScale = replayOptions.sizeScale; | ||
| replayMaker.cacheMaxSize | ||
| = (NSInteger)(shouldReplayFullSession ? replayOptions.sessionSegmentDuration + 1 | ||
| : replayOptions.errorReplayDuration + 1); | ||
|
|
||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| SentryDisplayLinkWrapper *displayLinkWrapper = [[SentryDisplayLinkWrapper alloc] init]; | ||
| self.sessionReplay = [[SentrySessionReplay alloc] initWithReplayOptions:replayOptions | ||
| replayFolderPath:docs | ||
| screenshotProvider:screenshotProvider | ||
| replayMaker:replayMaker | ||
| breadcrumbConverter:breadcrumbConverter | ||
| touchTracker:_touchTracker | ||
| dateProvider:_dateProvider | ||
| delegate:self | ||
| displayLinkWrapper:displayLinkWrapper]; | ||
|
|
||
| [self.sessionReplay | ||
| startWithRootView:SentryDependencyContainer.sharedInstance.application.windows.firstObject | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: There is already a fixes section for the unreleased section