From d04e017c2435c29052d0826138b5b2dda990d4ee Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 14 Sep 2018 15:24:42 -0700 Subject: [PATCH 1/5] Remove use of NSHashTable for interface state delegates #trivial --- Source/ASDisplayNode.mm | 96 +++++++++++++++++--------- Source/Private/ASDisplayNodeInternal.h | 16 +++-- 2 files changed, 74 insertions(+), 38 deletions(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index d84b5cbe0..69bf2ac02 100644 --- 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 delegate in delegates) { \ - [delegate method]; \ - } - - (BOOL)_locked_shouldLoadViewOrLayer { ASAssertLocked(__instanceLock__); @@ -575,7 +564,9 @@ - (void)_didLoad for (ASDisplayNodeDidLoadBlock block in onDidLoadBlocks) { block(self); } - ASDisplayNodeCallInterfaceStateDelegates(nodeDidLoad); + [self enumerateInterfaceStateDelegates:^(id del) { + [del nodeDidLoad]; + }]; } - (void)didLoad @@ -1280,7 +1271,9 @@ - (void)layout ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); ASDisplayNodeAssertTrue(self.isNodeLoaded); - ASDisplayNodeCallInterfaceStateDelegates(nodeDidLayout); + [self enumerateInterfaceStateDelegates:^(id del) { + [del nodeDidLayout]; + }]; } #pragma mark Layout Transition @@ -3218,12 +3211,9 @@ - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfac // Subclass hook ASAssertUnlocked(__instanceLock__); ASDisplayNodeAssertMainThread(); - __instanceLock__.lock(); - NSHashTable *delegates = [_interfaceStateDelegates copy]; - __instanceLock__.unlock(); - for (id delegate in delegates) { - [delegate interfaceStateDidChange:newState fromState:oldState]; - } + [self enumerateInterfaceStateDelegates:^(id del) { + [del interfaceStateDidChange:newState fromState:oldState]; + }]; } - (BOOL)shouldScheduleDisplayWithNewInterfaceState:(ASInterfaceState)newInterfaceState @@ -3236,20 +3226,25 @@ - (BOOL)shouldScheduleDisplayWithNewInterfaceState:(ASInterfaceState)newInterfac - (void)addInterfaceStateDelegate:(id )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 < 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", MAX_INTERFACE_STATE_DELEGATES); } - (void)removeInterfaceStateDelegate:(id )interfaceStateDelegate { ASDN::MutexLocker l(__instanceLock__); - _copiedInterfaceStateDelegates = nil; - [_interfaceStateDelegates removeObject:interfaceStateDelegate]; + for (int i = 0; i < MAX_INTERFACE_STATE_DELEGATES; i++) { + if (_interfaceStateDelegates[i] == interfaceStateDelegate) { + _interfaceStateDelegates[i] = nil; + break; + } + } } - (BOOL)isVisible @@ -3263,7 +3258,9 @@ - (void)didEnterVisibleState // subclass override ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); - ASDisplayNodeCallInterfaceStateDelegates(didEnterVisibleState); + [self enumerateInterfaceStateDelegates:^(id del) { + [del didEnterVisibleState]; + }]; #if AS_ENABLE_TIPS [ASTipsController.shared nodeDidAppear:self]; #endif @@ -3274,7 +3271,9 @@ - (void)didExitVisibleState // subclass override ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); - ASDisplayNodeCallInterfaceStateDelegates(didExitVisibleState); + [self enumerateInterfaceStateDelegates:^(id del) { + [del didExitVisibleState]; + }]; } - (BOOL)isInDisplayState @@ -3288,7 +3287,9 @@ - (void)didEnterDisplayState // subclass override ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); - ASDisplayNodeCallInterfaceStateDelegates(didEnterDisplayState); + [self enumerateInterfaceStateDelegates:^(id del) { + [del didEnterDisplayState]; + }]; } - (void)didExitDisplayState @@ -3296,7 +3297,10 @@ - (void)didExitDisplayState // subclass override ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); - ASDisplayNodeCallInterfaceStateDelegates(didExitDisplayState); + + [self enumerateInterfaceStateDelegates:^(id del) { + [del didExitDisplayState]; + }]; } - (BOOL)isInPreloadState @@ -3346,14 +3350,18 @@ - (void)didEnterPreloadState if (self.automaticallyManagesSubnodes) { [self layoutIfNeeded]; } - ASDisplayNodeCallInterfaceStateDelegates(didEnterPreloadState); + [self enumerateInterfaceStateDelegates:^(id del) { + [del didEnterPreloadState]; + }]; } - (void)didExitPreloadState { ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); - ASDisplayNodeCallInterfaceStateDelegates(didExitPreloadState); + [self enumerateInterfaceStateDelegates:^(id del) { + [del didExitPreloadState]; + }]; } - (void)clearContents @@ -3380,7 +3388,29 @@ - (void)recursivelyClearContents }); } +- (void)enumerateInterfaceStateDelegates:(void (NS_NOESCAPE ^)(id))block +{ + ASAssertUnlocked(__instanceLock__); + + id dels[MAX_INTERFACE_STATE_DELEGATES]; + int count = 0; + { + ASLockScopeSelf(); + // Fast path for non-delegating nodes. + if (!_hasHadInterfaceStateDelegates) { + return; + } + for (int i = 0; i < 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 index 18eb03c5c..10965a9bc 100644 --- a/Source/Private/ASDisplayNodeInternal.h +++ b/Source/Private/ASDisplayNodeInternal.h @@ -75,6 +75,7 @@ AS_EXTERN NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimest #define TIME_DISPLAYNODE_OPS 0 // If you're using this information frequently, try: (DEBUG || PROFILE) #define NUM_CLIP_CORNER_LAYERS 4 +#define MAX_INTERFACE_STATE_DELEGATES 4 @interface ASDisplayNode () <_ASTransitionContextCompletionDelegate> { @@ -247,12 +248,10 @@ AS_EXTERN NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimest NSTimeInterval _debugTimeToAddSubnodeViews; NSTimeInterval _debugTimeForDidLoad; #endif - - NSHashTable > *_interfaceStateDelegates; - // never mutated, used to enumerate delegates outside of lock. - // set to nil when mutating _interfaceStateDelegates. - NSHashTable > *_copiedInterfaceStateDelegates; + /// Fast path: tells whether we've ever had an interface state delegate before. + BOOL _hasHadInterfaceStateDelegates; + __weak id _interfaceStateDelegates[MAX_INTERFACE_STATE_DELEGATES]; } + (void)scheduleNodeForRecursiveDisplay:(ASDisplayNode *)node; @@ -334,6 +333,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 delegate))block; + /** * // TODO: NOT YET IMPLEMENTED * From 1d515f9710fdbac9f732f992c721d631889b4699 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 14 Sep 2018 15:27:10 -0700 Subject: [PATCH 2/5] Stray line --- Source/ASDisplayNode.mm | 1 - 1 file changed, 1 deletion(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 69bf2ac02..5fa0ad1f5 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -3297,7 +3297,6 @@ - (void)didExitDisplayState // subclass override ASDisplayNodeAssertMainThread(); ASAssertUnlocked(__instanceLock__); - [self enumerateInterfaceStateDelegates:^(id del) { [del didExitDisplayState]; }]; From 9cb2bc9ee5058b806f9e077a5f0e109164add078 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 17 Sep 2018 09:13:40 -0700 Subject: [PATCH 3/5] One more case --- Source/ASDisplayNode.mm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 5fa0ad1f5..19202ea65 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -1496,7 +1496,9 @@ - (void)_pendingNodeDidDisplay:(ASDisplayNode *)node if (_pendingDisplayNodes.isEmpty) { [self hierarchyDisplayDidFinish]; - ASDisplayNodeCallInterfaceStateDelegates(hierarchyDisplayDidFinish); + [self enumerateInterfaceStateDelegates:^(id delegate) { + [delegate hierarchyDisplayDidFinish]; + }]; BOOL placeholderShouldPersist = [self placeholderShouldPersist]; From 9a663a8aef4ae64e36daabcdd1f3fe03f262dcbf Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 17 Sep 2018 11:13:10 -0700 Subject: [PATCH 4/5] Add code to let people have more delegates --- Source/ASDisplayNode.h | 6 ++++++ Source/Private/ASDisplayNodeInternal.h | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Source/ASDisplayNode.h b/Source/ASDisplayNode.h index 9bf9236d0..437436b9f 100644 --- 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/Private/ASDisplayNodeInternal.h b/Source/Private/ASDisplayNodeInternal.h index 10965a9bc..cf0fc57dc 100644 --- a/Source/Private/ASDisplayNodeInternal.h +++ b/Source/Private/ASDisplayNodeInternal.h @@ -75,7 +75,6 @@ AS_EXTERN NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimest #define TIME_DISPLAYNODE_OPS 0 // If you're using this information frequently, try: (DEBUG || PROFILE) #define NUM_CLIP_CORNER_LAYERS 4 -#define MAX_INTERFACE_STATE_DELEGATES 4 @interface ASDisplayNode () <_ASTransitionContextCompletionDelegate> { From 07914c02a2a83ca7a4312e71db7cc627891f3130 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Mon, 17 Sep 2018 11:15:54 -0700 Subject: [PATCH 5/5] Do it more --- Source/ASDisplayNode.mm | 10 +++++----- Source/Private/ASDisplayNodeInternal.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 19202ea65..3136600ed 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -3229,19 +3229,19 @@ - (void)addInterfaceStateDelegate:(id )interfaceStateD { ASDN::MutexLocker l(__instanceLock__); _hasHadInterfaceStateDelegates = YES; - for (int i = 0; i < MAX_INTERFACE_STATE_DELEGATES; i++) { + for (int i = 0; i < AS_MAX_INTERFACE_STATE_DELEGATES; i++) { if (_interfaceStateDelegates[i] == nil) { _interfaceStateDelegates[i] = interfaceStateDelegate; return; } } - ASDisplayNodeFailAssert(@"Exceeded interface state delegate limit: %d", MAX_INTERFACE_STATE_DELEGATES); + ASDisplayNodeFailAssert(@"Exceeded interface state delegate limit: %d", AS_MAX_INTERFACE_STATE_DELEGATES); } - (void)removeInterfaceStateDelegate:(id )interfaceStateDelegate { ASDN::MutexLocker l(__instanceLock__); - for (int i = 0; i < MAX_INTERFACE_STATE_DELEGATES; i++) { + for (int i = 0; i < AS_MAX_INTERFACE_STATE_DELEGATES; i++) { if (_interfaceStateDelegates[i] == interfaceStateDelegate) { _interfaceStateDelegates[i] = nil; break; @@ -3393,7 +3393,7 @@ - (void)enumerateInterfaceStateDelegates:(void (NS_NOESCAPE ^)(id _interfaceStateDelegates[MAX_INTERFACE_STATE_DELEGATES]; + __weak id _interfaceStateDelegates[AS_MAX_INTERFACE_STATE_DELEGATES]; } + (void)scheduleNodeForRecursiveDisplay:(ASDisplayNode *)node;