Merge pull request #962 from facebook/OptimizeVisibilityNotificationDisablement

[ASDisplayNode] Order-of-magnitude speedup in handling of "disable visibility" notifications.

Author: appleguy

AsyncDisplayKit/ASDisplayNode.mm

@@ -1253,42 +1253,46 @@ - (void)removeFromSupernode
 
 - (BOOL)__visibilityNotificationsDisabled
 {
+  // Currently, this method is only used by the testing infrastructure to verify this internal feature.
   ASDN::MutexLocker l(_propertyLock);
   return _flags.visibilityNotificationsDisabled > 0;
 }
 
+- (BOOL)__selfOrParentHasVisibilityNotificationsDisabled
+{
+  ASDN::MutexLocker l(_propertyLock);
+  return (_hierarchyState & ASHierarchyStateTransitioningSupernodes);
+}
+
 - (void)__incrementVisibilityNotificationsDisabled
 {
   ASDN::MutexLocker l(_propertyLock);
   const size_t maxVisibilityIncrement = (1ULL<<VISIBILITY_NOTIFICATIONS_DISABLED_BITS) - 1ULL;
   ASDisplayNodeAssert(_flags.visibilityNotificationsDisabled < maxVisibilityIncrement, @"Oops, too many increments of the visibility notifications API");
-  if (_flags.visibilityNotificationsDisabled < maxVisibilityIncrement)
+  if (_flags.visibilityNotificationsDisabled < maxVisibilityIncrement) {
     _flags.visibilityNotificationsDisabled++;
+  }
+  if (_flags.visibilityNotificationsDisabled == 1) {
+    // Must have just transitioned from 0 to 1.  Notify all subnodes that we are in a disabled state.
+    [self enterHierarchyState:ASHierarchyStateTransitioningSupernodes];
+  }
 }
 
 - (void)__decrementVisibilityNotificationsDisabled
 {
   ASDN::MutexLocker l(_propertyLock);
   ASDisplayNodeAssert(_flags.visibilityNotificationsDisabled > 0, @"Can't decrement past 0");
-  if (_flags.visibilityNotificationsDisabled > 0)
+  if (_flags.visibilityNotificationsDisabled > 0) {
     _flags.visibilityNotificationsDisabled--;
-}
-
-// This uses the layer hieararchy for safety. Who knows what people might do and it would be bad to have visibilty out of sync
-- (BOOL)__selfOrParentHasVisibilityNotificationsDisabled
-{
-  CALayer *layer = _layer;
-  do {
-    ASDisplayNode *node = ASLayerToDisplayNode(layer);
-    if (node) {
-      if (node->_flags.visibilityNotificationsDisabled) {
-        return YES;
-      }
-    }
-    layer = layer.superlayer;
-  } while (layer);
-
-  return NO;
+  }
+  if (_flags.visibilityNotificationsDisabled == 0) {
+    // Must have just transitioned from 1 to 0.  Notify all subnodes that we are no longer in a disabled state.
+    // FIXME: This system should be revisited when refactoring and consolidating the implementation of the
+    // addSubnode: and insertSubnode:... methods.  As implemented, though logically irrelevant for expected use cases,
+    // multiple nodes in the subtree below may have a non-zero visibilityNotification count and still have
+    // the ASHierarchyState bit cleared (the only value checked when reading this state).
+    [self exitHierarchyState:ASHierarchyStateTransitioningSupernodes];
+  }
 }
 
 - (void)__enterHierarchy

AsyncDisplayKit/Details/ASBasicImageDownloader.h

@@ -16,4 +16,7 @@
 
 + (instancetype)sharedImageDownloader;
 
++ (instancetype)new __attribute__((unavailable("+[ASBasicImageDownloader sharedImageDownloader] must be used.")));
+- (instancetype)init __attribute__((unavailable("+[ASBasicImageDownloader sharedImageDownloader] must be used.")));
+
 @end

AsyncDisplayKit/Details/ASBasicImageDownloader.mm

@@ -205,14 +205,14 @@ + (instancetype)sharedImageDownloader
   static ASBasicImageDownloader *sharedImageDownloader = nil;
   static dispatch_once_t once = 0;
   dispatch_once(&once, ^{
-    sharedImageDownloader = [[ASBasicImageDownloader alloc] init];
+    sharedImageDownloader = [[ASBasicImageDownloader alloc] _init];
   });
   return sharedImageDownloader;
 }
 
 #pragma mark Lifecycle.
 
-- (instancetype)init
+- (instancetype)_init
 {
   if (!(self = [super init]))
     return nil;

AsyncDisplayKit/Private/ASDisplayNode+DebugTiming.mm

@@ -7,12 +7,10 @@
  */
 
 #import "ASDisplayNode+DebugTiming.h"
-
 #import "ASDisplayNodeInternal.h"
 
 @implementation ASDisplayNode (DebugTiming)
 
-
 #if TIME_DISPLAYNODE_OPS
 - (NSTimeInterval)debugTimeToCreateView
 {
@@ -83,6 +81,4 @@ - (NSTimeInterval)debugAllCreationTime
 
 #endif
 
-
-
 @end

AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h

@@ -37,14 +37,17 @@
 typedef NS_OPTIONS(NSUInteger, ASHierarchyState)
 {
   /** The node may or may not have a supernode, but no supernode has a special hierarchy-influencing option enabled. */
-  ASHierarchyStateNormal       = 0,
+  ASHierarchyStateNormal                  = 0,
   /** The node has a supernode with .shouldRasterizeDescendants = YES.
       Note: the root node of the rasterized subtree (the one with the property set on it) will NOT have this state set. */
-  ASHierarchyStateRasterized   = 1 << 0,
+  ASHierarchyStateRasterized              = 1 << 0,
   /** The node or one of its supernodes is managed by a class like ASRangeController.  Most commonly, these nodes are
       ASCellNode objects or a subnode of one, and are used in ASTableView or ASCollectionView.
       These nodes also recieve regular updates to the .interfaceState property with more detailed status information. */
-  ASHierarchyStateRangeManaged = 1 << 1,
+  ASHierarchyStateRangeManaged            = 1 << 1,
+  /** Down-propogated version of _flags.visibilityNotificationsDisabled.  This flag is very rarely set, but by having it
+      locally available to nodes, they do not have to walk up supernodes at the critical points it is checked. */
+  ASHierarchyStateTransitioningSupernodes = 1 << 2
 };
 
 @interface ASDisplayNode () <_ASDisplayLayerDelegate>

AsyncDisplayKit/Private/ASDisplayNodeInternal.h

@@ -105,7 +105,6 @@ typedef NS_OPTIONS(NSUInteger, ASDisplayNodeMethodOverrides)
   NSTimeInterval _debugTimeToAddSubnodeViews;
   NSTimeInterval _debugTimeForDidLoad;
 #endif
-
 }
 
 + (void)scheduleNodeForDisplay:(ASDisplayNode *)node;
@@ -136,6 +135,7 @@ typedef NS_OPTIONS(NSUInteger, ASDisplayNodeMethodOverrides)
 
 // Private API for helper functions / unit tests.  Use ASDisplayNodeDisableHierarchyNotifications() to control this.
 - (BOOL)__visibilityNotificationsDisabled;
+- (BOOL)__selfOrParentHasVisibilityNotificationsDisabled;
 - (void)__incrementVisibilityNotificationsDisabled;
 - (void)__decrementVisibilityNotificationsDisabled;
 

AsyncDisplayKitTests/ASBasicImageDownloaderTests.m

@@ -16,8 +16,9 @@ @interface ASBasicImageDownloaderTests : XCTestCase
 
 @implementation ASBasicImageDownloaderTests
 
-- (void)testAsynchronouslyDownloadTheSameURLTwice {
-    ASBasicImageDownloader *downloader = [ASBasicImageDownloader new];
+- (void)testAsynchronouslyDownloadTheSameURLTwice
+{
+    ASBasicImageDownloader *downloader = [ASBasicImageDownloader sharedImageDownloader];
 
     NSURL *URL = [NSURL URLWithString:@"http://wrongPath/wrongResource.png"];
 

AsyncDisplayKitTests/ASDisplayNodeAppearanceTests.m

@@ -49,6 +49,7 @@ static dispatch_block_t modifyMethodByAddingPrologueBlockAndReturnCleanupBlock(C
 
 @interface ASDisplayNode (PrivateStuffSoWeDontPullInCPPInternalH)
 - (BOOL)__visibilityNotificationsDisabled;
+- (BOOL)__selfOrParentHasVisibilityNotificationsDisabled;
 - (id)initWithViewClass:(Class)viewClass;
 - (id)initWithLayerClass:(Class)layerClass;
 @end
@@ -360,6 +361,7 @@ - (void)checkMoveAcrossHierarchyLayerBacked:(BOOL)isLayerBacked useManualCalls:(
   }
   if (useManualDisable) {
     XCTAssertTrue([child __visibilityNotificationsDisabled], @"Should not have re-enabled yet");
+    XCTAssertTrue([child __selfOrParentHasVisibilityNotificationsDisabled], @"Should not have re-enabled yet");
     ASDisplayNodeEnableHierarchyNotifications(child);
   }
 
@@ -377,6 +379,7 @@ - (void)checkMoveAcrossHierarchyLayerBacked:(BOOL)isLayerBacked useManualCalls:(
   }
   if (useManualDisable) {
     XCTAssertTrue([child __visibilityNotificationsDisabled], @"Should not have re-enabled yet");
+    XCTAssertTrue([child __selfOrParentHasVisibilityNotificationsDisabled], @"Should not have re-enabled yet");
     ASDisplayNodeEnableHierarchyNotifications(child);
   }
 
@@ -390,6 +393,7 @@ - (void)checkMoveAcrossHierarchyLayerBacked:(BOOL)isLayerBacked useManualCalls:(
 
   // Make sure that we don't leave these unbalanced
   XCTAssertFalse([child __visibilityNotificationsDisabled], @"Unbalanced visibility notifications calls");
+  XCTAssertFalse([child __selfOrParentHasVisibilityNotificationsDisabled], @"Should not have re-enabled yet");
 
   [window release];
 }