From f5460a44e0c1e9d67378ac5ae896f4e073c44713 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Thu, 16 Jun 2016 17:10:09 -0700 Subject: [PATCH 1/4] [ASDisplayNode] Reduce locking in removeFromSupernode --- AsyncDisplayKit/ASDisplayNode.mm | 45 ++++++++++++++------------------ 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 5bf9966ccb..30383ee899 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1515,40 +1515,35 @@ static NSInteger incrementIfFound(NSInteger i) { [subnode __setSupernode:nil]; } +// NOTE: You must not called this method while holding the receiver's property lock. This may cause deadlocks. - (void)removeFromSupernode { - ASDisplayNodeAssertThreadAffinity(self); - BOOL shouldRemoveFromSuperviewOrSuperlayer = NO; + _propertyLock.lock(); + __weak ASDisplayNode *supernode = _supernode; + __weak UIView *view = _view; + __weak CALayer *layer = _layer; + BOOL layerBacked = _flags.layerBacked; + _propertyLock.unlock(); - { - ASDN::MutexLocker l(_propertyLock); - if (!_supernode) - return; + if (supernode == nil) { + return; + } + [supernode _removeSubnode:self]; + + if (self.nodeLoaded && supernode.nodeLoaded) { // Check to ensure that our view or layer is actually inside of our supernode; otherwise, don't remove it. // Though _ASDisplayView decouples the supernode if it is inserted inside another view hierarchy, this is // more difficult to guarantee with _ASDisplayLayer because CoreAnimation doesn't have a -didMoveToSuperlayer. - - if (self.nodeLoaded && _supernode.nodeLoaded) { - if (_flags.layerBacked || _supernode.layerBacked) { - shouldRemoveFromSuperviewOrSuperlayer = (_layer.superlayer == _supernode.layer); - } else { - shouldRemoveFromSuperviewOrSuperlayer = (_view.superview == _supernode.view); - } - } - } - - // Do this before removing the view from the hierarchy, as the node will clear its supernode pointer when its view is removed from the hierarchy. - // This call may result in the object being destroyed. - [_supernode _removeSubnode:self]; - - if (shouldRemoveFromSuperviewOrSuperlayer) { ASPerformBlockOnMainThread(^{ - ASDN::MutexLocker l(_propertyLock); - if (_flags.layerBacked) { - [_layer removeFromSuperlayer]; + if (layerBacked || supernode.layerBacked) { + if (layer.superlayer == supernode.layer) { + [layer removeFromSuperlayer]; + } } else { - [_view removeFromSuperview]; + if (view.superview == supernode.view) { + [view removeFromSuperview]; + } } }); } From 15b6f2e2811348fa94ee8d5e8dc131913df662ae Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Thu, 16 Jun 2016 17:10:33 -0700 Subject: [PATCH 2/4] [ASLayoutTransition] Optimize add/remove subnode methods --- AsyncDisplayKit/Private/ASLayoutTransition.mm | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKit/Private/ASLayoutTransition.mm b/AsyncDisplayKit/Private/ASLayoutTransition.mm index 67709cc302..560ac6135e 100644 --- a/AsyncDisplayKit/Private/ASLayoutTransition.mm +++ b/AsyncDisplayKit/Private/ASLayoutTransition.mm @@ -48,9 +48,12 @@ { ASDN::MutexLocker l(_propertyLock); [self calculateSubnodeOperationsIfNeeded]; - for (NSUInteger i = 0; i < [_insertedSubnodes count]; i++) { + + NSUInteger i = 0; + for (ASDisplayNode *node in _insertedSubnodes) { NSUInteger p = _insertedSubnodePositions[i]; - [_node insertSubnode:_insertedSubnodes[i] atIndex:p]; + [_node insertSubnode:node atIndex:p]; + i += 1; } } @@ -58,8 +61,8 @@ { ASDN::MutexLocker l(_propertyLock); [self calculateSubnodeOperationsIfNeeded]; - for (NSUInteger i = 0; i < [_removedSubnodes count]; i++) { - [_removedSubnodes[i] removeFromSupernode]; + for (ASDisplayNode *subnode in _removedSubnodes) { + [subnode removeFromSupernode]; } } From 0002d333f0edfc907b72c9656f0169c0d62bf87f Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Thu, 16 Jun 2016 17:59:50 -0700 Subject: [PATCH 3/4] [ASDisplayNodeTests] Add some removeFromSupernode tests --- AsyncDisplayKitTests/ASDisplayNodeTests.m | 51 +++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/AsyncDisplayKitTests/ASDisplayNodeTests.m b/AsyncDisplayKitTests/ASDisplayNodeTests.m index ed8ee61632..c85b7d3180 100644 --- a/AsyncDisplayKitTests/ASDisplayNodeTests.m +++ b/AsyncDisplayKitTests/ASDisplayNodeTests.m @@ -1638,6 +1638,57 @@ static inline BOOL _CGPointEqualToPointWithEpsilon(CGPoint point1, CGPoint point [c release]; } +- (void)testRemoveFromViewBackedLoadedSupernode +{ + DeclareNodeNamed(a); + DeclareNodeNamed(b); + [b addSubnode:a]; + [a view]; + [b view]; + XCTAssertNodesLoaded(a, b); + XCTAssertEqual(a.supernode, b); + XCTAssertEqual(a.view.superview, b.view); + + [a removeFromSupernode]; + XCTAssertNil(a.supernode); + XCTAssertNil(a.view.superview); +} + +- (void)testRemoveFromLayerBackedLoadedSupernode +{ + DeclareNodeNamed(a); + a.layerBacked = YES; + DeclareNodeNamed(b); + b.layerBacked = YES; + [b addSubnode:a]; + [a layer]; + [b layer]; + XCTAssertNodesLoaded(a, b); + XCTAssertEqual(a.supernode, b); + XCTAssertEqual(a.layer.superlayer, b.layer); + + [a removeFromSupernode]; + XCTAssertNil(a.supernode); + XCTAssertNil(a.layer.superlayer); +} + +- (void)testRemoveLayerBackedFromViewBackedLoadedSupernode +{ + DeclareNodeNamed(a); + a.layerBacked = YES; + DeclareNodeNamed(b); + [b addSubnode:a]; + [a layer]; + [b view]; + XCTAssertNodesLoaded(a, b); + XCTAssertEqual(a.supernode, b); + XCTAssertEqual(a.layer.superlayer, b.layer); + + [a removeFromSupernode]; + XCTAssertNil(a.supernode); + XCTAssertNil(a.layer.superlayer); +} + - (void)testSubnodeAddedBeforeLoadingExternalView { UIView *view = [[UIDisplayNodeTestView alloc] init]; From 42aa52b4076257fc50faafeac15c86783ae275a7 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 17 Jun 2016 09:51:09 -0700 Subject: [PATCH 4/4] [ASDisplayNode] Put the thread affinity assertion back --- AsyncDisplayKit/ASDisplayNode.mm | 1 + 1 file changed, 1 insertion(+) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 30383ee899..9609d183df 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1518,6 +1518,7 @@ static NSInteger incrementIfFound(NSInteger i) { // NOTE: You must not called this method while holding the receiver's property lock. This may cause deadlocks. - (void)removeFromSupernode { + ASDisplayNodeAssertThreadAffinity(self); _propertyLock.lock(); __weak ASDisplayNode *supernode = _supernode; __weak UIView *view = _view;