New feature flags store implementation that preserves insertion order.

bugsnag · Sep 14, 2022 · 2cb1a9c · 2cb1a9c
1 parent 6c1a4ef
commit 2cb1a9c
Show file tree

Hide file tree

Showing 6 changed files with 236 additions and 34 deletions.
diff --git a/Bugsnag/BugsnagInternals.h b/Bugsnag/BugsnagInternals.h
@@ -22,7 +22,8 @@
 #import "BugsnagHandledState.h"
 #import "BugsnagNotifier.h"
 
-typedef NSMutableArray<BugsnagFeatureFlag *> BSGFeatureFlagStore;
+@interface BSGFeatureFlagStore : NSObject <NSCopying>
+@end
 
 NS_ASSUME_NONNULL_BEGIN
 

diff --git a/Bugsnag/Client/BugsnagClient.m b/Bugsnag/Client/BugsnagClient.m
@@ -195,7 +195,7 @@ - (instancetype)initWithConfiguration:(BugsnagConfiguration *)configuration {
             [_configuration setUser:[BSG_KSSystemInfo deviceAndAppHash] withEmail:_configuration.user.email andName:_configuration.user.name];
         }
 
-        _featureFlagStore = [configuration.featureFlagStore mutableCopy];
+        _featureFlagStore = [configuration.featureFlagStore copy];
 
         _state = [[BugsnagMetadata alloc] initWithDictionary:@{
             BSGKeyClient: @{
@@ -762,7 +762,7 @@ - (void)notifyInternal:(BugsnagEvent *_Nonnull)event
     // App hang events will already contain feature flags
     if (!event.featureFlagStore.count) {
         @synchronized (self.featureFlagStore) {
-            event.featureFlagStore = [self.featureFlagStore mutableCopy];
+            event.featureFlagStore = [self.featureFlagStore copy];
         }
     }
 
@@ -976,7 +976,7 @@ - (void)setObserver:(BSGClientObserver)observer {
         };
 
         @synchronized (self.featureFlagStore) {
-            for (BugsnagFeatureFlag *flag in self.featureFlagStore) {
+            for (BugsnagFeatureFlag *flag in self.featureFlagStore.allFlags) {
                 observer(BSGClientObserverAddFeatureFlag, flag);
             }
         }
@@ -1040,7 +1040,7 @@ - (void)appHangDetectedAtDate:(NSDate *)date withThreads:(NSArray<BugsnagThread
     self.appHangEvent.context = self.context;
 
     @synchronized (self.featureFlagStore) {
-        self.appHangEvent.featureFlagStore = [self.featureFlagStore mutableCopy];
+        self.appHangEvent.featureFlagStore = [self.featureFlagStore copy];
     }
 
     [self.appHangEvent symbolicateIfNeeded];

diff --git a/Bugsnag/Helpers/BSGFeatureFlagStore.h b/Bugsnag/Helpers/BSGFeatureFlagStore.h
@@ -7,6 +7,7 @@
 //
 
 #import "BugsnagInternals.h"
+#import "BSGDefines.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -20,4 +21,24 @@ NSArray<NSDictionary *> * BSGFeatureFlagStoreToJSON(BSGFeatureFlagStore *store);
 
 BSGFeatureFlagStore * BSGFeatureFlagStoreFromJSON(id _Nullable json);
 
+
+BSG_OBJC_DIRECT_MEMBERS
+@interface BSGFeatureFlagStore ()
+
+@property(nonatomic,nonnull,readonly) NSArray<BugsnagFeatureFlag *> * allFlags;
+
++ (nonnull BSGFeatureFlagStore *) fromJSON:(nonnull id)json;
+
+- (NSUInteger) count;
+
+- (void) addFeatureFlag:(nonnull NSString *)name withVariant:(nullable NSString *)variant;
+
+- (void) addFeatureFlags:(nonnull NSArray<BugsnagFeatureFlag *> *)featureFlags;
+
+- (void) clear:(nullable NSString *)name;
+
+- (nonnull NSArray<NSDictionary *> *) toJSON;
+
+@end
+
 NS_ASSUME_NONNULL_END
diff --git a/Bugsnag/Helpers/BSGFeatureFlagStore.m b/Bugsnag/Helpers/BSGFeatureFlagStore.m
@@ -11,43 +11,50 @@
 #import "BSGKeys.h"
 #import "BugsnagFeatureFlag.h"
 
-static void internalAddFeatureFlag(BSGFeatureFlagStore *store, BugsnagFeatureFlag *flag) {
-    [store removeObject:flag];
-    [store addObject:flag];
-}
-
 void BSGFeatureFlagStoreAddFeatureFlag(BSGFeatureFlagStore *store, NSString *name, NSString *_Nullable variant) {
-    internalAddFeatureFlag(store, [BugsnagFeatureFlag flagWithName:name variant:variant]);
+    [store addFeatureFlag:name withVariant:variant];
 }
 
 void BSGFeatureFlagStoreAddFeatureFlags(BSGFeatureFlagStore *store, NSArray<BugsnagFeatureFlag *> *featureFlags) {
-    for (BugsnagFeatureFlag *featureFlag in featureFlags) {
-        internalAddFeatureFlag(store, featureFlag);
-    }
+    [store addFeatureFlags:featureFlags];
 }
 
 void BSGFeatureFlagStoreClear(BSGFeatureFlagStore *store, NSString *_Nullable name) {
-    if (name) {
-        [store removeObject:[BugsnagFeatureFlag flagWithName:(NSString * _Nonnull)name]];
-    } else {
-        [store removeAllObjects];
-    }
+    [store clear:name];
 }
 
 NSArray<NSDictionary *> * BSGFeatureFlagStoreToJSON(BSGFeatureFlagStore *store) {
-    NSMutableArray<NSDictionary *> *result = [NSMutableArray array];
-    for (BugsnagFeatureFlag *flag in store) {
-        if (flag.variant) {
-            [result addObject:@{BSGKeyFeatureFlag: flag.name, BSGKeyVariant: (NSString * _Nonnull)flag.variant}];
-        } else {
-            [result addObject:@{BSGKeyFeatureFlag: flag.name}];
-        }
-    }
-    return result;
+    return [store toJSON];
 }
 
 BSGFeatureFlagStore * BSGFeatureFlagStoreFromJSON(id json) {
-    BSGFeatureFlagStore *store = [NSMutableArray array];
+    return [BSGFeatureFlagStore fromJSON:json];
+}
+
+
+/**
+ * Stores feature flags as a dictionary containing the flag name as a key, with the
+ * value being the index into an array containing the complete feature flag.
+ *
+ * Removals leave holes in the array, which gets rebuilt on clear once there are too many holes.
+ *
+ * This gives the access speed of a dictionary while keeping ordering intact.
+ */
+BSG_OBJC_DIRECT_MEMBERS
+@interface BSGFeatureFlagStore ()
+
+@property(nonatomic, readwrite) NSMutableArray *flags;
+@property(nonatomic, readwrite) NSMutableDictionary *indices;
+
+@end
+
+static const int REBUILD_AT_HOLE_COUNT = 1000;
+
+BSG_OBJC_DIRECT_MEMBERS
+@implementation BSGFeatureFlagStore
+
++ (nonnull BSGFeatureFlagStore *) fromJSON:(nonnull id)json {
+    BSGFeatureFlagStore *store = [BSGFeatureFlagStore new];
     if ([json isKindOfClass:[NSArray class]]) {
         for (id item in json) {
             if ([item isKindOfClass:[NSDictionary class]]) {
@@ -57,10 +64,119 @@ void BSGFeatureFlagStoreClear(BSGFeatureFlagStore *store, NSString *_Nullable na
                     if (![variant isKindOfClass:[NSString class]]) {
                         variant = nil;
                     }
-                    [store addObject:[BugsnagFeatureFlag flagWithName:featureFlag variant:variant]];
+                    [store addFeatureFlag:featureFlag withVariant:variant];
                 }
             }
         }
     }
     return store;
 }
+
+- (nonnull instancetype) init {
+    if ((self = [super init]) != nil) {
+        _flags = [NSMutableArray new];
+        _indices = [NSMutableDictionary new];
+    }
+    return self;
+}
+
+static inline int getIndexFromDict(NSDictionary *dict, NSString *name) {
+    NSNumber *boxedIndex = dict[name];
+    if (boxedIndex == nil) {
+        return -1;
+    }
+    return boxedIndex.intValue;
+}
+
+- (NSUInteger) count {
+    return self.indices.count;
+}
+
+- (nonnull NSArray<BugsnagFeatureFlag *> *) allFlags {
+    NSMutableArray<BugsnagFeatureFlag *> *flags = [NSMutableArray arrayWithCapacity:self.indices.count];
+    for (BugsnagFeatureFlag *flag in self.flags) {
+        if ([flag isKindOfClass:[BugsnagFeatureFlag class]]) {
+            [flags addObject:flag];
+        }
+    }
+    return flags;
+}
+
+- (void)rebuildIfTooManyHoles {
+    int holeCount = (int)self.flags.count - (int)self.indices.count;
+    if (holeCount < REBUILD_AT_HOLE_COUNT) {
+        return;
+    }
+
+    NSMutableArray *newFlags = [NSMutableArray arrayWithCapacity:self.indices.count];
+    NSMutableDictionary *newIndices = [NSMutableDictionary new];
+    for (BugsnagFeatureFlag *flag in self.flags) {
+        if ([flag isKindOfClass:[BugsnagFeatureFlag class]]) {
+            [newFlags addObject:flag];
+        }
+    }
+
+    for (NSUInteger i = 0; i < newFlags.count; i++) {
+        BugsnagFeatureFlag *flag = newFlags[i];
+        newIndices[flag.name] = @(i);
+    }
+    self.flags = newFlags;
+    self.indices = newIndices;
+}
+
+- (void) addFeatureFlag:(nonnull NSString *)name withVariant:(nullable NSString *)variant {
+    BugsnagFeatureFlag *flag = [BugsnagFeatureFlag flagWithName:name variant:variant];
+
+    int index = getIndexFromDict(self.indices, name);
+    if (index >= 0) {
+        self.flags[(unsigned)index] = flag;
+    } else {
+        index = (int)self.flags.count;
+        [self.flags addObject:flag];
+        self.indices[name] = @(index);
+    }
+}
+
+- (void) addFeatureFlags:(nonnull NSArray<BugsnagFeatureFlag *> *)featureFlags {
+    for (BugsnagFeatureFlag *flag in featureFlags) {
+        [self addFeatureFlag:flag.name withVariant:flag.variant];
+    }
+}
+
+- (void) clear:(nullable NSString *)name {
+    if (name != nil) {
+        int index = getIndexFromDict(self.indices, name);
+        if (index >= 0) {
+            self.flags[(unsigned)index] = [NSNull null];
+            [self.indices removeObjectForKey:(id)name];
+            [self rebuildIfTooManyHoles];
+        }
+    } else {
+        [self.indices removeAllObjects];
+        [self.flags removeAllObjects];
+    }
+}
+
+- (nonnull NSArray<NSDictionary *> *) toJSON {
+    NSMutableArray<NSDictionary *> *result = [NSMutableArray array];
+
+    for (BugsnagFeatureFlag *flag in self.flags) {
+        if ([flag isKindOfClass:[BugsnagFeatureFlag class]]) {
+            if (flag.variant) {
+                [result addObject:@{BSGKeyFeatureFlag:flag.name, BSGKeyVariant:(NSString *_Nonnull)flag.variant}];
+            } else {
+                [result addObject:@{BSGKeyFeatureFlag:flag.name}];
+            }
+        }
+    }
+    return result;
+}
+
+- (id)copyWithZone:(NSZone *)zone {
+    BSGFeatureFlagStore *store = [[BSGFeatureFlagStore allocWithZone:zone] init];
+    store.flags = [self.flags mutableCopy];
+    store.indices = [self.indices mutableCopy];
+    return store;
+}
+
+@end
diff --git a/Bugsnag/Payload/BugsnagEvent.m b/Bugsnag/Payload/BugsnagEvent.m
@@ -760,7 +760,7 @@ - (void)setUnhandled:(BOOL)unhandled {
 // MARK: - <BugsnagFeatureFlagStore>
 
 - (NSArray<BugsnagFeatureFlag *> *)featureFlags {
-    return [self.featureFlagStore copy];
+    return self.featureFlagStore.allFlags;
 }
 
 - (void)addFeatureFlagWithName:(NSString *)name variant:(nullable NSString *)variant {

diff --git a/Tests/BugsnagTests/BSGFeatureFlagStoreTests.m b/Tests/BugsnagTests/BSGFeatureFlagStoreTests.m
@@ -44,12 +44,12 @@ - (void)test {
     XCTAssertEqualObjects(BSGFeatureFlagStoreToJSON(store),
                           (@[
                             @{@"featureFlag": @"featureC", @"variant": @"checked"},
+                            @{@"featureFlag": @"featureA"},
                             @{@"featureFlag": @"featureB"},
-                            @{@"featureFlag": @"featureA"}
                           ]));
 
-    XCTAssertEqualObjects(BSGFeatureFlagStoreFromJSON(BSGFeatureFlagStoreToJSON(store)),
-                          store);
+    XCTAssertEqualObjects(BSGFeatureFlagStoreToJSON(BSGFeatureFlagStoreFromJSON(BSGFeatureFlagStoreToJSON(store))),
+                          BSGFeatureFlagStoreToJSON(store));
 
     BSGFeatureFlagStoreClear(store, @"featureB");
     XCTAssertEqualObjects(BSGFeatureFlagStoreToJSON(store),
@@ -62,4 +62,68 @@ - (void)test {
     XCTAssertEqualObjects(BSGFeatureFlagStoreToJSON(store), @[]);
 }
 
+- (void)testAddRemoveMany {
+    // Tests that rebuildIfTooManyHoles works as expected
+
+    BSGFeatureFlagStore *store = [[BSGFeatureFlagStore alloc] init];
+
+    BSGFeatureFlagStoreAddFeatureFlag(store, @"blah", @"testing");
+    for (int j = 0; j < 10; j++) {
+        for (int i = 0; i < 1000; i++) {
+            NSString *name = [NSString stringWithFormat:@"%d-%d", j, i];
+            BSGFeatureFlagStoreAddFeatureFlag(store, name, nil);
+            if (i < 999) {
+                BSGFeatureFlagStoreClear(store, name);
+            }
+        }
+    }
+
+    XCTAssertEqualObjects(BSGFeatureFlagStoreToJSON(store),
+                          (@[
+                            @{@"featureFlag": @"blah", @"variant": @"testing"},
+                            @{@"featureFlag": @"0-999"},
+                            @{@"featureFlag": @"1-999"},
+                            @{@"featureFlag": @"2-999"},
+                            @{@"featureFlag": @"3-999"},
+                            @{@"featureFlag": @"4-999"},
+                            @{@"featureFlag": @"5-999"},
+                            @{@"featureFlag": @"6-999"},
+                            @{@"featureFlag": @"7-999"},
+                            @{@"featureFlag": @"8-999"},
+                            @{@"featureFlag": @"9-999"},
+                          ]));
+}
+
+- (void)testAddFeatureFlagPerformance {
+    BSGFeatureFlagStore *store = [[BSGFeatureFlagStore alloc] init];
+
+    __auto_type block = ^{
+        for (int i = 0; i < 1000; i++) {
+            NSString *name = [NSString stringWithFormat:@"%d", i];
+            BSGFeatureFlagStoreAddFeatureFlag(store, name, nil);
+        }
+    };
+
+    block();
+
+    [self measureBlock:block];
+}
+
+- (void)testDictionaryPerformance {
+    // For comparision to show the best performance possible
+
+    NSMutableDictionary *dictionary = [NSMutableDictionary dictionary];
+
+    __auto_type block = ^{
+        for (int i = 0; i < 1000; i++) {
+            NSString *name = [NSString stringWithFormat:@"%d", i];
+            [dictionary setObject:[NSNull null] forKey:name];
+        }
+    };
+
+    block();
+
+    [self measureBlock:block];
+}
+
 @end