From d802ba244eab943204ee6a9a5adeeb7910c3ce42 Mon Sep 17 00:00:00 2001
From: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
Date: Mon, 28 Oct 2024 09:03:49 +0100
Subject: [PATCH] ref: Improve frames tracker performance (#4469)

Improves the speed of the frames tracker by not locking it anymore since it always run in the main thread.
---
 CHANGELOG.md                                  |  1 +
 Sources/Sentry/SentryFramesTracker.m          | 23 ++++++++-----------
 .../SentryFramesTrackerTests.swift            | 11 +++++++++
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 22f31f1cdc..804947c0e6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 
 ### Improvements
 
+- Improve frames tracker performance (#4469)
 - Log a warning when dropping envelopes due to rate-limiting (#4463)
 
 ## 8.39.0-beta.1
diff --git a/Sources/Sentry/SentryFramesTracker.m b/Sources/Sentry/SentryFramesTracker.m
index 13e288e489..24380b7894 100644
--- a/Sources/Sentry/SentryFramesTracker.m
+++ b/Sources/Sentry/SentryFramesTracker.m
@@ -256,14 +256,11 @@ - (void)displayLinkCallback
 
 - (void)reportNewFrame
 {
-    NSArray *localListeners;
-    @synchronized(self.listeners) {
-        localListeners = [self.listeners allObjects];
-    }
-
     NSDate *newFrameDate = [self.dateProvider date];
-
-    for (id<SentryFramesTrackerListener> listener in localListeners) {
+    // We need to copy the list because some listeners will remove themselves
+    // from the list during the callback, causing a crash during iteration.
+    NSArray<id<SentryFramesTrackerListener>> *listeners = [self.listeners copy];
+    for (id<SentryFramesTrackerListener> listener in listeners) {
         [listener framesTrackerHasNewFrame:newFrameDate];
     }
 }
@@ -310,17 +307,15 @@ - (SentryFramesDelayResult *)getFramesDelay:(uint64_t)startSystemTimestamp
 
 - (void)addListener:(id<SentryFramesTrackerListener>)listener
 {
-
-    @synchronized(self.listeners) {
-        [self.listeners addObject:listener];
-    }
+    // Adding listeners on the main thread to avoid race condition with new frame callback
+    [self.dispatchQueueWrapper dispatchAsyncOnMainQueue:^{ [self.listeners addObject:listener]; }];
 }
 
 - (void)removeListener:(id<SentryFramesTrackerListener>)listener
 {
-    @synchronized(self.listeners) {
-        [self.listeners removeObject:listener];
-    }
+    // Removing listeners on the main thread to avoid race condition with new frame callback
+    [self.dispatchQueueWrapper
+        dispatchAsyncOnMainQueue:^{ [self.listeners removeObject:listener]; }];
 }
 
 - (void)pause
diff --git a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
index 69d419ac6d..999593b006 100644
--- a/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
+++ b/Tests/SentryTests/Integrations/Performance/FramesTracking/SentryFramesTrackerTests.swift
@@ -24,6 +24,7 @@ class SentryFramesTrackerTests: XCTestCase {
         }
         
         lazy var sut: SentryFramesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper, dateProvider: dateProvider, dispatchQueueWrapper: SentryDispatchQueueWrapper(), notificationCenter: notificationCenter, keepDelayedFramesDuration: keepDelayedFramesDuration)
+        
     }
     
     private var fixture: Fixture!
@@ -602,6 +603,16 @@ class SentryFramesTrackerTests: XCTestCase {
         XCTAssertEqual(listener2.newFrameInvocations.count, 1)
         XCTAssertEqual(listener2.newFrameInvocations.first?.timeIntervalSince1970, expectedFrameDate.timeIntervalSince1970)
     }
+    
+    func testListenerAreAddedInMainThread() {
+        let dispatchQueueWrapper = TestSentryDispatchQueueWrapper()
+        let sut = SentryFramesTracker(displayLinkWrapper: fixture.displayLinkWrapper, dateProvider: fixture.dateProvider, dispatchQueueWrapper: dispatchQueueWrapper, notificationCenter: fixture.notificationCenter, keepDelayedFramesDuration: fixture.keepDelayedFramesDuration)
+        let listener = FrameTrackerListener()
+        
+        sut.add(listener)
+        
+        XCTAssertEqual(dispatchQueueWrapper.blockOnMainInvocations.count, 1)
+    }
 
     func testRemoveListener() {
         let sut = fixture.sut