Remove use of NSHashTable for interface state delegates #trivial (#1122)

* Remove use of NSHashTable for interface state delegates #trivial * Stray line * One more case * Add code to let people have more delegates * Do it more
TextureGroup · Sep 17, 2018 · 9bcafef · 9bcafef
1 parent f161665
commit 9bcafef
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 39 deletions.
diff --git a/Source/ASDisplayNode.h b/Source/ASDisplayNode.h
@@ -24,6 +24,10 @@ NS_ASSUME_NONNULL_BEGIN
 
 #define ASDisplayNodeLoggingEnabled 0
 
+#ifndef AS_MAX_INTERFACE_STATE_DELEGATES
+#define AS_MAX_INTERFACE_STATE_DELEGATES 4
+#endif
+
 @class ASDisplayNode;
 @protocol ASContextTransitioning;
 
@@ -258,6 +262,8 @@ AS_EXTERN NSInteger const ASDefaultDrawingPriority;
  * @abstract Adds a delegate to receive notifications on interfaceState changes.
  *
  * @warning This must be called from the main thread.
+ * There is a hard limit on the number of delegates a node can have; see
+ * AS_MAX_INTERFACE_STATE_DELEGATES above.
  *
  * @see ASInterfaceState
  */

diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm
@@ -452,17 +452,6 @@ - (void)dealloc
 
 #pragma mark - Loading
 
-#define ASDisplayNodeCallInterfaceStateDelegates(method) \
-    __instanceLock__.lock(); \
-    NSHashTable *delegates = _copiedInterfaceStateDelegates; \
-    if (!delegates) { \
-      delegates = _copiedInterfaceStateDelegates = [_interfaceStateDelegates copy]; \
-    } \
-    __instanceLock__.unlock(); \
-    for (id <ASInterfaceStateDelegate> delegate in delegates) { \
-      [delegate method]; \
-    }
-
 - (BOOL)_locked_shouldLoadViewOrLayer
 {
   ASAssertLocked(__instanceLock__);
@@ -574,7 +563,9 @@ - (void)_didLoad
   for (ASDisplayNodeDidLoadBlock block in onDidLoadBlocks) {
     block(self);
   }
-  ASDisplayNodeCallInterfaceStateDelegates(nodeDidLoad);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del nodeDidLoad];
+  }];
 }
 
 - (void)didLoad
@@ -1279,7 +1270,9 @@ - (void)layout
   ASDisplayNodeAssertMainThread();
   ASAssertUnlocked(__instanceLock__);
   ASDisplayNodeAssertTrue(self.isNodeLoaded);
-  ASDisplayNodeCallInterfaceStateDelegates(nodeDidLayout);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del nodeDidLayout];
+  }];
 }
 
 #pragma mark Layout Transition
@@ -1502,7 +1495,9 @@ - (void)_pendingNodeDidDisplay:(ASDisplayNode *)node
   if (_pendingDisplayNodes.isEmpty) {
 
     [self hierarchyDisplayDidFinish];
-    ASDisplayNodeCallInterfaceStateDelegates(hierarchyDisplayDidFinish);
+    [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> delegate) {
+      [delegate hierarchyDisplayDidFinish];
+    }];
 
     BOOL placeholderShouldPersist = [self placeholderShouldPersist];
 
@@ -3227,12 +3222,9 @@ - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfac
   // Subclass hook
   ASAssertUnlocked(__instanceLock__);
   ASDisplayNodeAssertMainThread();
-  __instanceLock__.lock();
-  NSHashTable *delegates = [_interfaceStateDelegates copy];
-  __instanceLock__.unlock();
-  for (id <ASInterfaceStateDelegate> delegate in delegates) {
-    [delegate interfaceStateDidChange:newState fromState:oldState];
-  }
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del interfaceStateDidChange:newState fromState:oldState];
+  }];
 }
 
 - (BOOL)shouldScheduleDisplayWithNewInterfaceState:(ASInterfaceState)newInterfaceState
@@ -3245,20 +3237,25 @@ - (BOOL)shouldScheduleDisplayWithNewInterfaceState:(ASInterfaceState)newInterfac
 - (void)addInterfaceStateDelegate:(id <ASInterfaceStateDelegate>)interfaceStateDelegate
 {
   ASDN::MutexLocker l(__instanceLock__);
-  // Not a fan of lazy loading, but this method won't get called very often and avoiding
-  // the overhead of creating this is probably worth it.
-  if (_interfaceStateDelegates == nil) {
-    _interfaceStateDelegates = [NSHashTable weakObjectsHashTable];
+  _hasHadInterfaceStateDelegates = YES;
+  for (int i = 0; i < AS_MAX_INTERFACE_STATE_DELEGATES; i++) {
+    if (_interfaceStateDelegates[i] == nil) {
+      _interfaceStateDelegates[i] = interfaceStateDelegate;
+      return;
+    }
   }
-  _copiedInterfaceStateDelegates = nil;
-  [_interfaceStateDelegates addObject:interfaceStateDelegate];
+  ASDisplayNodeFailAssert(@"Exceeded interface state delegate limit: %d", AS_MAX_INTERFACE_STATE_DELEGATES);
 }
 
 - (void)removeInterfaceStateDelegate:(id <ASInterfaceStateDelegate>)interfaceStateDelegate
 {
   ASDN::MutexLocker l(__instanceLock__);
-  _copiedInterfaceStateDelegates = nil;
-  [_interfaceStateDelegates removeObject:interfaceStateDelegate];
+  for (int i = 0; i < AS_MAX_INTERFACE_STATE_DELEGATES; i++) {
+    if (_interfaceStateDelegates[i] == interfaceStateDelegate) {
+      _interfaceStateDelegates[i] = nil;
+      break;
+    }
+  }
 }
 
 - (BOOL)isVisible
@@ -3272,7 +3269,9 @@ - (void)didEnterVisibleState
   // subclass override
   ASDisplayNodeAssertMainThread();
   ASAssertUnlocked(__instanceLock__);
-  ASDisplayNodeCallInterfaceStateDelegates(didEnterVisibleState);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del didEnterVisibleState];
+  }];
 #if AS_ENABLE_TIPS
   [ASTipsController.shared nodeDidAppear:self];
 #endif
@@ -3283,7 +3282,9 @@ - (void)didExitVisibleState
   // subclass override
   ASDisplayNodeAssertMainThread();
   ASAssertUnlocked(__instanceLock__);
-  ASDisplayNodeCallInterfaceStateDelegates(didExitVisibleState);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del didExitVisibleState];
+  }];
 }
 
 - (BOOL)isInDisplayState
@@ -3297,15 +3298,19 @@ - (void)didEnterDisplayState
   // subclass override
   ASDisplayNodeAssertMainThread();
   ASAssertUnlocked(__instanceLock__);
-  ASDisplayNodeCallInterfaceStateDelegates(didEnterDisplayState);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del didEnterDisplayState];
+  }];
 }
 
 - (void)didExitDisplayState
 {
   // subclass override
   ASDisplayNodeAssertMainThread();
   ASAssertUnlocked(__instanceLock__);
-  ASDisplayNodeCallInterfaceStateDelegates(didExitDisplayState);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del didExitDisplayState];
+  }];
 }
 
 - (BOOL)isInPreloadState
@@ -3355,14 +3360,18 @@ - (void)didEnterPreloadState
   if (self.automaticallyManagesSubnodes) {
     [self layoutIfNeeded];
   }
-  ASDisplayNodeCallInterfaceStateDelegates(didEnterPreloadState);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del didEnterPreloadState];
+  }];
 }
 
 - (void)didExitPreloadState
 {
   ASDisplayNodeAssertMainThread();
   ASAssertUnlocked(__instanceLock__);
-  ASDisplayNodeCallInterfaceStateDelegates(didExitPreloadState);
+  [self enumerateInterfaceStateDelegates:^(id<ASInterfaceStateDelegate> del) {
+    [del didExitPreloadState];
+  }];
 }
 
 - (void)clearContents
@@ -3389,7 +3398,29 @@ - (void)recursivelyClearContents
   });
 }
 
+- (void)enumerateInterfaceStateDelegates:(void (NS_NOESCAPE ^)(id<ASInterfaceStateDelegate>))block
+{
+  ASAssertUnlocked(__instanceLock__);
+
+  id dels[AS_MAX_INTERFACE_STATE_DELEGATES];
+  int count = 0;
+  {
+    ASLockScopeSelf();
+    // Fast path for non-delegating nodes.
+    if (!_hasHadInterfaceStateDelegates) {
+      return;
+    }
 
+    for (int i = 0; i < AS_MAX_INTERFACE_STATE_DELEGATES; i++) {
+      if ((dels[count] = _interfaceStateDelegates[i])) {
+        count++;
+      }
+    }
+  }
+  for (int i = 0; i < count; i++) {
+    block(dels[i]);
+  }
+}
 
 #pragma mark - Gesture Recognizing
 

diff --git a/Source/Private/ASDisplayNodeInternal.h b/Source/Private/ASDisplayNodeInternal.h
@@ -249,12 +249,10 @@ AS_EXTERN NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimest
   NSTimeInterval _debugTimeToAddSubnodeViews;
   NSTimeInterval _debugTimeForDidLoad;
 #endif
-
-  NSHashTable <id <ASInterfaceStateDelegate>> *_interfaceStateDelegates;
 
-  // never mutated, used to enumerate delegates outside of lock.
-  // set to nil when mutating _interfaceStateDelegates.
-  NSHashTable <id <ASInterfaceStateDelegate>> *_copiedInterfaceStateDelegates;
+  /// Fast path: tells whether we've ever had an interface state delegate before.
+  BOOL _hasHadInterfaceStateDelegates;
+  __weak id<ASInterfaceStateDelegate> _interfaceStateDelegates[AS_MAX_INTERFACE_STATE_DELEGATES];
 }
 
 + (void)scheduleNodeForRecursiveDisplay:(ASDisplayNode *)node;
@@ -336,6 +334,13 @@ AS_EXTERN NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimest
 
 - (void)applyPendingViewState;
 
+/**
+ * Makes a local copy of the interface state delegates then calls the block on each.
+ *
+ * Lock is not held during block invocation. Method must not be called with the lock held.
+ */
+- (void)enumerateInterfaceStateDelegates:(void(NS_NOESCAPE ^)(id<ASInterfaceStateDelegate> delegate))block;
+
 /**
  * // TODO: NOT YET IMPLEMENTED
  *