impr: Speed up adding breadcrumbs (#4029)

Adding breadcrumbs was O(n) because the SDK used removeObjectAtIndex:0 when reaching the max breadcrumb amount, which requires reshifting the whole array. Now, we use a ring buffer making adding breadcrumbs O(1). Fixes GH-3341 Co-authored-by: Sentry Github Bot <[email protected]>
getsentry · Jun 4, 2024 · 96af87c · 96af87c
1 parent 1f9387b
commit 96af87c
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,7 @@ to receive SIGTERM events, set the option `enableSigtermReporting = true`.
 ### Improvements
 
 - Stop FramesTracker when app is in background (#3979)
+- Speed up adding breadcrumbs (#4029)
 - Skip evaluating log messages when not logged (#4028)
 
 ### Fixes

diff --git a/Sources/Sentry/SentryScope.m b/Sources/Sentry/SentryScope.m
@@ -45,7 +45,9 @@
  */
 @property (atomic) enum SentryLevel levelEnum;
 
-@property (atomic) NSInteger maxBreadcrumbs;
+@property (atomic) NSUInteger maxBreadcrumbs;
+@property (atomic) NSUInteger currentBreadcrumbIndex;
+@property (atomic, strong) NSMutableArray<SentryBreadcrumb *> *breadcrumbArray;
 
 @property (atomic, strong) NSMutableArray<SentryAttachment *> *attachmentArray;
 
@@ -62,8 +64,9 @@ @implementation SentryScope {
 - (instancetype)initWithMaxBreadcrumbs:(NSInteger)maxBreadcrumbs
 {
     if (self = [super init]) {
-        self.maxBreadcrumbs = maxBreadcrumbs;
-        self.breadcrumbArray = [NSMutableArray new];
+        _maxBreadcrumbs = MAX(0, maxBreadcrumbs);
+        _currentBreadcrumbIndex = 0;
+        _breadcrumbArray = [[NSMutableArray alloc] initWithCapacity:_maxBreadcrumbs];
         self.tagDictionary = [NSMutableDictionary new];
         self.extraDictionary = [NSMutableDictionary new];
         self.contextDictionary = [NSMutableDictionary new];
@@ -87,7 +90,9 @@ - (instancetype)initWithScope:(SentryScope *)scope
         [_extraDictionary addEntriesFromDictionary:[scope extras]];
         [_tagDictionary addEntriesFromDictionary:[scope tags]];
         [_contextDictionary addEntriesFromDictionary:[scope context]];
-        [_breadcrumbArray addObjectsFromArray:[scope breadcrumbs]];
+        NSArray<SentryBreadcrumb *> *crumbs = [scope breadcrumbs];
+        _currentBreadcrumbIndex = crumbs.count;
+        [_breadcrumbArray addObjectsFromArray:crumbs];
         [_fingerprintArray addObjectsFromArray:[scope fingerprints]];
         [_attachmentArray addObjectsFromArray:[scope attachments]];
 
@@ -117,10 +122,14 @@ - (void)addBreadcrumb:(SentryBreadcrumb *)crumb
     }
     SENTRY_LOG_DEBUG(@"Add breadcrumb: %@", crumb);
     @synchronized(_breadcrumbArray) {
-        [_breadcrumbArray addObject:crumb];
-        if ([_breadcrumbArray count] > self.maxBreadcrumbs) {
-            [_breadcrumbArray removeObjectAtIndex:0];
-        }
+        // Use a ring buffer making adding breadcrumbs O(1).
+        // In a prior version, we added the new breadcrumb at the end of the array and used
+        // removeObjectAtIndex:0 when reaching the max breadcrumb amount. removeObjectAtIndex:0 is
+        // O(n) because it needs to reshift the whole array. So when the breadcrumbs array was full
+        // every add operation was O(n).
+
+        _breadcrumbArray[_currentBreadcrumbIndex] = crumb;
+        _currentBreadcrumbIndex = (_currentBreadcrumbIndex + 1) % _maxBreadcrumbs;
 
         for (id<SentryScopeObserver> observer in self.observers) {
             [observer addSerializedBreadcrumb:[crumb serialize]];
@@ -150,9 +159,7 @@ - (void)clear
     // references instead of self we remove all objects instead of creating new instances. Removing
     // all objects is usually O(n). This is acceptable as we don't expect a huge amount of elements
     // in the arrays or dictionaries, that would slow down the performance.
-    @synchronized(_breadcrumbArray) {
-        [_breadcrumbArray removeAllObjects];
-    }
+    [self clearBreadcrumbs];
     @synchronized(_tagDictionary) {
         [_tagDictionary removeAllObjects];
     }
@@ -183,7 +190,8 @@ - (void)clear
 - (void)clearBreadcrumbs
 {
     @synchronized(_breadcrumbArray) {
-        [_breadcrumbArray removeAllObjects];
+        _currentBreadcrumbIndex = 0;
+        _breadcrumbArray = [[NSMutableArray alloc] initWithCapacity:_maxBreadcrumbs];
 
         for (id<SentryScopeObserver> observer in self.observers) {
             [observer clearBreadcrumbs];
@@ -194,7 +202,18 @@ - (void)clearBreadcrumbs
 - (NSArray<SentryBreadcrumb *> *)breadcrumbs
 {
     @synchronized(_breadcrumbArray) {
-        return _breadcrumbArray.copy;
+        NSMutableArray<SentryBreadcrumb *> *crumbs = [NSMutableArray new];
+        for (int i = 0; i < _maxBreadcrumbs; i++) {
+            // Crumbs use a ring buffer. We need to start at the current crumb to get the
+            // crumbs in the correct order.
+            NSInteger index = (_currentBreadcrumbIndex + i) % _maxBreadcrumbs;
+
+            if (index < _breadcrumbArray.count) {
+                [crumbs addObject:_breadcrumbArray[index]];
+            }
+        }
+
+        return crumbs;
     }
 }
 

diff --git a/Sources/Sentry/SentrySessionReplay.m b/Sources/Sentry/SentrySessionReplay.m
@@ -292,7 +292,7 @@ - (void)captureSegment:(NSInteger)segment
     __block NSArray<SentryRRWebEvent *> *events;
 
     [SentrySDK.currentHub configureScope:^(SentryScope *_Nonnull scope) {
-        events = [self->_breadcrumbConverter convertWithBreadcrumbs:scope.breadcrumbArray
+        events = [self->_breadcrumbConverter convertWithBreadcrumbs:scope.breadcrumbs
                                                                from:videoInfo.start
                                                               until:videoInfo.end];
     }];

diff --git a/Sources/Sentry/include/SentryScope+Private.h b/Sources/Sentry/include/SentryScope+Private.h
@@ -23,10 +23,7 @@ SentryScope ()
 
 @property (atomic, strong) SentryPropagationContext *propagationContext;
 
-/**
- * Contains the breadcrumbs which will be sent with the event
- */
-@property (atomic, strong) NSMutableArray<SentryBreadcrumb *> *breadcrumbArray;
+- (NSArray<SentryBreadcrumb *> *)breadcrumbs;
 
 /**
  * used to add values in event context.

diff --git a/Tests/SentryTests/SentryScopeSwiftTests.swift b/Tests/SentryTests/SentryScopeSwiftTests.swift
@@ -579,6 +579,59 @@ class SentryScopeSwiftTests: XCTestCase {
         XCTAssertEqual(2, observer.clearInvocations)
     }
 
+    func testDefaultBreadcrumbCapacity() {
+        let scope = Scope()
+        for i in 0..<197 {
+            let crumb = Breadcrumb()
+            crumb.message = "\(i)"
+            scope.addBreadcrumb(crumb)
+        }
+
+        let scopeSerialized = scope.serialize()
+        let scopeCrumbs = scopeSerialized["breadcrumbs"] as? [[String: Any]]
+        XCTAssertEqual(100, scopeCrumbs?.count ?? 0)
+
+        var j = 0
+        for i in 97..<197 {
+            let actualMessage = scopeCrumbs?[j]["message"] as? String
+            XCTAssertEqual("\(i)", actualMessage)
+
+            j += 1
+        }
+    }
+
+    func testBreadcrumbsNotFull() {
+        let scope = Scope()
+        for i in 0..<97 {
+            let crumb = Breadcrumb()
+            crumb.message = "\(i)"
+            scope.addBreadcrumb(crumb)
+        }
+
+        let scopeSerialized = scope.serialize()
+        let scopeCrumbs = scopeSerialized["breadcrumbs"] as? [[String: Any]]
+        XCTAssertEqual(97, scopeCrumbs?.count ?? 0)
+
+        for i in 0..<97 {
+            let actualMessage = scopeCrumbs?[i]["message"] as? String
+            XCTAssertEqual("\(i)", actualMessage)
+        }
+    }
+
+    func testClearBreadcrumb() {
+        let scope = Scope()
+        scope.clearBreadcrumbs()
+        for _ in 0..<101 {
+            scope.addBreadcrumb(fixture.breadcrumb)
+        }
+        scope.clearBreadcrumbs()
+
+        let scopeSerialized = scope.serialize()
+
+        let scopeCrumbs = scopeSerialized["breadcrumbs"] as? [[String: Any]]
+        XCTAssertEqual(0, scopeCrumbs?.count ?? 0)
+    }
+
     class TestScopeObserver: NSObject, SentryScopeObserver {
         var tags: [String: String]?
         func setTags(_ tags: [String: String]?) {

diff --git a/Tests/SentryTests/SentryScopeTests.m b/Tests/SentryTests/SentryScopeTests.m
@@ -57,18 +57,6 @@ - (void)testBreadcrumbOlderReplacedByNewer
     XCTAssertEqual(expectedMaxBreadcrumb, [scope2Crumbs count]);
 }
 
-- (void)testDefaultMaxCapacity
-{
-    SentryScope *scope = [[SentryScope alloc] init];
-    for (int i = 0; i < 2000; ++i) {
-        [scope addBreadcrumb:[[SentryBreadcrumb alloc] init]];
-    }
-
-    NSDictionary<NSString *, id> *scopeSerialized = [scope serialize];
-    NSArray *scopeCrumbs = [scopeSerialized objectForKey:@"breadcrumbs"];
-    XCTAssertEqual(100, [scopeCrumbs count]);
-}
-
 - (void)testSetTagValueForKey
 {
     NSDictionary<NSString *, NSString *> *excpected = @{ @"A" : @"1", @"B" : @"2", @"C" : @"" };
@@ -163,13 +151,4 @@ - (void)testReplaySerializes
     XCTAssertEqualObjects([[scope serialize] objectForKey:@"replay_id"], expectedReplayId);
 }
 
-- (void)testClearBreadcrumb
-{
-    SentryScope *scope = [[SentryScope alloc] init];
-    [scope clearBreadcrumbs];
-    [scope addBreadcrumb:[self getBreadcrumb]];
-    [scope clearBreadcrumbs];
-    XCTAssertTrue([[[scope serialize] objectForKey:@"breadcrumbs"] count] == 0);
-}
-
 @end