Experiment with different strategies for image downloader priority (#1349)

Right now when an image node enters preload state, we kick off an image request with the default priority. Then when it enters display state, we change the priority to "imminent" which is mapped to the default priority as well. This means that requests from preload and display nodes have the same priority and are put to the same pool. The right behavior would be that preload requests should have a lower priority from the beginning.

Another problem is that, due to the execution order of -didEnter(Preload|Display|Visible)State calls, a node may kick off a low priority request when it enters preload state even though it knows that it's also visible. By the time -didEnterVisibleState is called, the low priority request may have already been consumed and the download/data task won't pick up the new higher priority, or some work needs to be done to move it to another queue. A better behavior would be to always use the current interface state to determine the priority. This means that visible nodes will kick off high priority requests as soon as -didEnterPreloadState is called.

The last (and smaller) issue is that a node marks its request as preload/low priority as soon as it exits visible state. I'd argue that this is too agressive. It may be reasonble for nodes in the trailing direction. Even so, we already handle this case by (almost always) have smaller trailing buffers. So this diff makes sure that nodes that exited visible state will have imminent/default priority if they remain in the display range.

All of these new behaviors are wrapped in an experiment and will be tested carefully before being rolled out.

* Add imports

* Fix build failure

* Encapsulate common logics into methods

* Address comments

Author: nguyenhuy

Podfile.lock

@@ -1,38 +1,32 @@
 PODS:
   - FBSnapshotTestCase/Core (2.1.4)
-  - FLAnimatedImage (1.0.12)
   - JGMethodSwizzler (2.0.1)
   - OCMock (3.4.1)
-  - PINCache (3.0.1-beta.6):
-    - PINCache/Arc-exception-safe (= 3.0.1-beta.6)
-    - PINCache/Core (= 3.0.1-beta.6)
-  - PINCache/Arc-exception-safe (3.0.1-beta.6):
+  - PINCache (3.0.1-beta.7):
+    - PINCache/Arc-exception-safe (= 3.0.1-beta.7)
+    - PINCache/Core (= 3.0.1-beta.7)
+  - PINCache/Arc-exception-safe (3.0.1-beta.7):
     - PINCache/Core
-  - PINCache/Core (3.0.1-beta.6):
-    - PINOperation (~> 1.1.0)
+  - PINCache/Core (3.0.1-beta.7):
+    - PINOperation (~> 1.1.1)
   - PINOperation (1.1.1)
-  - PINRemoteImage (3.0.0-beta.13):
-    - PINRemoteImage/FLAnimatedImage (= 3.0.0-beta.13)
-    - PINRemoteImage/PINCache (= 3.0.0-beta.13)
-  - PINRemoteImage/Core (3.0.0-beta.13):
+  - PINRemoteImage (3.0.0-beta.14):
+    - PINRemoteImage/PINCache (= 3.0.0-beta.14)
+  - PINRemoteImage/Core (3.0.0-beta.14):
     - PINOperation
-  - PINRemoteImage/FLAnimatedImage (3.0.0-beta.13):
-    - FLAnimatedImage (>= 1.0)
-    - PINRemoteImage/Core
-  - PINRemoteImage/PINCache (3.0.0-beta.13):
-    - PINCache (= 3.0.1-beta.6)
+  - PINRemoteImage/PINCache (3.0.0-beta.14):
+    - PINCache (= 3.0.1-beta.7)
     - PINRemoteImage/Core
 
 DEPENDENCIES:
   - FBSnapshotTestCase/Core (~> 2.1)
   - JGMethodSwizzler (from `https://github.com/JonasGessner/JGMethodSwizzler`, branch `master`)
   - OCMock (= 3.4.1)
-  - PINRemoteImage (= 3.0.0-beta.13)
+  - PINRemoteImage (= 3.0.0-beta.14)
 
 SPEC REPOS:
   https://github.com/cocoapods/specs.git:
     - FBSnapshotTestCase
-    - FLAnimatedImage
     - OCMock
     - PINCache
     - PINOperation
@@ -50,13 +44,12 @@ CHECKOUT OPTIONS:
 
 SPEC CHECKSUMS:
   FBSnapshotTestCase: 094f9f314decbabe373b87cc339bea235a63e07a
-  FLAnimatedImage: 4a0b56255d9b05f18b6dd7ee06871be5d3b89e31
   JGMethodSwizzler: 7328146117fffa8a4038c42eb7cd3d4c75006f97
   OCMock: 2cd0716969bab32a2283ff3a46fd26a8c8b4c5e3
-  PINCache: d195fdba255283f7e9900a55e3cced377f431f9b
+  PINCache: 7cb9ae068c8f655717f7c644ef1dff9fd573e979
   PINOperation: a6219e6fc9db9c269eb7a7b871ac193bcf400aac
-  PINRemoteImage: d6d51c5d2adda55f1ce30c96e850b6c4ebd2856a
+  PINRemoteImage: 81bbff853acc71c6de9e106e9e489a791b8bbb08
 
-PODFILE CHECKSUM: 42715d61f73cc22cc116bf80d7b268cb1f9e4742
+PODFILE CHECKSUM: 445046ac151568c694ff286684322273f0b597d6
 
-COCOAPODS: 1.5.3
+COCOAPODS: 1.6.0

Schemas/configuration.json

@@ -25,7 +25,8 @@
                     "exp_disable_a11y_cache",
                     "exp_skip_a11y_wait",
                     "exp_new_default_cell_layout_mode",
-                    "exp_dispatch_apply"
+                    "exp_dispatch_apply",
+                    "exp_image_downloader_priority"
                 ]
     		}
 		}

Source/ASExperimentalFeatures.h

@@ -31,6 +31,7 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
   ASExperimentalSkipAccessibilityWait = 1 << 10,            // exp_skip_a11y_wait
   ASExperimentalNewDefaultCellLayoutMode = 1 << 11,         // exp_new_default_cell_layout_mode
   ASExperimentalDispatchApply = 1 << 12,                    // exp_dispatch_apply
+  ASExperimentalImageDownloaderPriority = 1 << 13,          // exp_image_downloader_priority
   ASExperimentalFeatureAll = 0xFFFFFFFF
 };
 

Source/ASExperimentalFeatures.mm

@@ -24,8 +24,9 @@
                                       @"exp_disable_a11y_cache",
                                       @"exp_skip_a11y_wait",
                                       @"exp_new_default_cell_layout_mode",
-                                      @"exp_dispatch_apply"]));
-  
+                                      @"exp_dispatch_apply",
+                                      @"exp_image_downloader_priority"]));
+
   if (flags == ASExperimentalFeatureAll) {
     return allNames;
   }

Source/ASMultiplexImageNode.mm

@@ -53,7 +53,13 @@ @interface ASMultiplexImageNode ()
 @private
   // Core.
   id<ASImageCacheProtocol> _cache;
+
   id<ASImageDownloaderProtocol> _downloader;
+  struct {
+    unsigned int downloaderImplementsSetProgress:1;
+    unsigned int downloaderImplementsSetPriority:1;
+    unsigned int downloaderImplementsDownloadWithPriority:1;
+  } _downloaderFlags;
 
   __weak id<ASMultiplexImageNodeDelegate> _delegate;
   struct {
@@ -89,8 +95,6 @@ @interface ASMultiplexImageNode ()
   BOOL _shouldRenderProgressImages;
 
   //set on init only
-  BOOL _downloaderImplementsSetProgress;
-  BOOL _downloaderImplementsSetPriority;
   BOOL _cacheSupportsClearing;
 }
 
@@ -171,13 +175,14 @@ - (instancetype)initWithCache:(id<ASImageCacheProtocol>)cache downloader:(id<ASI
   _cache = (id<ASImageCacheProtocol>)cache;
   _downloader = (id<ASImageDownloaderProtocol>)downloader;
 
-  _downloaderImplementsSetProgress = [downloader respondsToSelector:@selector(setProgressImageBlock:callbackQueue:withDownloadIdentifier:)];
-  _downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
+  _downloaderFlags.downloaderImplementsSetProgress = [downloader respondsToSelector:@selector(setProgressImageBlock:callbackQueue:withDownloadIdentifier:)];
+  _downloaderFlags.downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
+  _downloaderFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:priority:callbackQueue:downloadProgress:completion:)];
 
   _cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)];
 
   _shouldRenderProgressImages = YES;
-  
+
   self.shouldBypassEnsureDisplay = YES;
 
   return self;
@@ -271,49 +276,34 @@ - (BOOL)placeholderShouldPersist
 - (void)displayWillStartAsynchronously:(BOOL)asynchronously
 {
   [super displayWillStartAsynchronously:asynchronously];
-  
   [self didEnterPreloadState];
-  
-  if (_downloaderImplementsSetPriority) {
-    {
-      ASDN::MutexLocker l(_downloadIdentifierLock);
-      if (_downloadIdentifier != nil) {
-        [_downloader setPriority:ASImageDownloaderPriorityImminent withDownloadIdentifier:_downloadIdentifier];
-      }
-    }
-  }
+  [self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityImminent];
 }
 
 /* didEnterVisibleState / didExitVisibleState in ASNetworkImageNode has a very similar implementation. Changes here are likely necessary
  in ASNetworkImageNode as well. */
 - (void)didEnterVisibleState
 {
   [super didEnterVisibleState];
-  
-  if (_downloaderImplementsSetPriority) {
-    ASDN::MutexLocker l(_downloadIdentifierLock);
-    if (_downloadIdentifier != nil) {
-      [_downloader setPriority:ASImageDownloaderPriorityVisible withDownloadIdentifier:_downloadIdentifier];
-    }
-  }
-  
+  [self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityVisible];
   [self _updateProgressImageBlockOnDownloaderIfNeeded];
 }
 
 - (void)didExitVisibleState
 {
   [super didExitVisibleState];
-  
-  if (_downloaderImplementsSetPriority) {
-    ASDN::MutexLocker l(_downloadIdentifierLock);
-    if (_downloadIdentifier != nil) {
-      [_downloader setPriority:ASImageDownloaderPriorityPreload withDownloadIdentifier:_downloadIdentifier];
-    }
-  }
-  
+  [self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityPreload];
   [self _updateProgressImageBlockOnDownloaderIfNeeded];
 }
 
+- (void)didExitDisplayState
+{
+  [super didExitDisplayState];
+  if (ASActivateExperimentalFeature(ASExperimentalImageDownloaderPriority)) {
+    [self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityPreload];
+  }
+}
+
 #pragma mark - Core
 
 - (void)setImage:(UIImage *)image
@@ -449,7 +439,6 @@ - (void)_setDownloadIdentifier:(id)downloadIdentifier
   _downloadIdentifier = downloadIdentifier;
 }
 
-
 #pragma mark - Image Loading Machinery
 
 - (void)_loadImageIdentifiers
@@ -493,19 +482,37 @@ - (UIImage *)_bestImmediatelyAvailableImageFromDataSource:(id *)imageIdentifierO
 
 #pragma mark -
 
-/**
- @note: This should be called without _downloadIdentifierLock held. We will lock
- super to read our interface state and it's best to avoid acquiring both locks.
- */
+- (void)_updatePriorityOnDownloaderIfNeededWithDefaultPriority:(ASImageDownloaderPriority)defaultPriority
+{
+  ASAssertUnlocked(_downloadIdentifierLock);
+
+  if (_downloaderFlags.downloaderImplementsSetPriority) {
+    // Read our interface state before locking so that we don't lock super while holding our lock.
+    ASInterfaceState interfaceState = self.interfaceState;
+    ASDN::MutexLocker l(_downloadIdentifierLock);
+
+    if (_downloadIdentifier != nil) {
+      ASImageDownloaderPriority priority = defaultPriority;
+      if (ASActivateExperimentalFeature(ASExperimentalImageDownloaderPriority)) {
+        priority = ASImageDownloaderPriorityWithInterfaceState(interfaceState);
+      }
+
+      [_downloader setPriority:priority withDownloadIdentifier:_downloadIdentifier];
+    }
+  }
+}
+
 - (void)_updateProgressImageBlockOnDownloaderIfNeeded
 {
+  ASAssertUnlocked(_downloadIdentifierLock);
+
   BOOL shouldRenderProgressImages = self.shouldRenderProgressImages;
 
   // Read our interface state before locking so that we don't lock super while holding our lock.
   ASInterfaceState interfaceState = self.interfaceState;
   ASDN::MutexLocker l(_downloadIdentifierLock);
 
-  if (!_downloaderImplementsSetProgress || _downloadIdentifier == nil) {
+  if (!_downloaderFlags.downloaderImplementsSetProgress || _downloadIdentifier == nil) {
     return;
   }
 
@@ -825,7 +832,7 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
     [_delegate multiplexImageNode:self didStartDownloadOfImageWithIdentifier:imageIdentifier];
 
   __weak __typeof__(self) weakSelf = self;
-  void (^downloadProgressBlock)(CGFloat) = nil;
+  ASImageDownloaderProgress downloadProgressBlock = NULL;
   if (_delegateFlags.downloadProgress) {
     downloadProgressBlock = ^(CGFloat progress) {
       __typeof__(self) strongSelf = weakSelf;
@@ -835,30 +842,67 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
     };
   }
 
+  ASImageDownloaderCompletion completion = ^(id <ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) {
+    // We dereference iVars directly, so we can't have weakSelf going nil on us.
+    __typeof__(self) strongSelf = weakSelf;
+    if (!strongSelf)
+      return;
+
+    ASDN::MutexLocker l(strongSelf->_downloadIdentifierLock);
+    //Getting a result back for a different download identifier, download must not have been successfully canceled
+    if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
+      return;
+    }
+
+    completionBlock([imageContainer asdk_image], error);
+
+    // Delegateify.
+    if (strongSelf->_delegateFlags.downloadFinish)
+      [strongSelf->_delegate multiplexImageNode:weakSelf didFinishDownloadingImageWithIdentifier:imageIdentifier error:error];
+  };
+
   // Download!
   ASPerformBlockOnBackgroundThread(^{
-    [self _setDownloadIdentifier:[_downloader downloadImageWithURL:imageURL
-                                                     callbackQueue:dispatch_get_main_queue()
-                                                  downloadProgress:downloadProgressBlock
-                                                        completion:^(id <ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) {
-                                                          // We dereference iVars directly, so we can't have weakSelf going nil on us.
-                                                          __typeof__(self) strongSelf = weakSelf;
-                                                          if (!strongSelf)
-                                                            return;
-                                                          
-                                                          ASDN::MutexLocker l(_downloadIdentifierLock);
-                                                          //Getting a result back for a different download identifier, download must not have been successfully canceled
-                                                          if (ASObjectIsEqual(_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
-                                                            return;
-                                                          }
-                                                          
-                                                          completionBlock([imageContainer asdk_image], error);
-                                                          
-                                                          // Delegateify.
-                                                          if (strongSelf->_delegateFlags.downloadFinish)
-                                                            [strongSelf->_delegate multiplexImageNode:weakSelf didFinishDownloadingImageWithIdentifier:imageIdentifier error:error];
-                                                        }]];
-    [self _updateProgressImageBlockOnDownloaderIfNeeded];
+    __typeof__(self) strongSelf = weakSelf;
+    if (!strongSelf)
+      return;
+
+    dispatch_queue_t callbackQueue = dispatch_get_main_queue();
+
+    id downloadIdentifier;
+    if (strongSelf->_downloaderFlags.downloaderImplementsDownloadWithPriority
+        && ASActivateExperimentalFeature(ASExperimentalImageDownloaderPriority)) {
+
+      /*
+       Decide a priority based on the current interface state of this node.
+       It can happen that this method was called when the node entered preload state
+       but the interface state, at this point, tells us that the node is (going to be) visible,
+       If that's the case, we jump to a higher priority directly.
+       */
+      ASImageDownloaderPriority priority = ASImageDownloaderPriorityWithInterfaceState(strongSelf.interfaceState);
+      downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
+                                                                priority:priority
+                                                           callbackQueue:callbackQueue
+                                                        downloadProgress:downloadProgressBlock
+                                                              completion:completion];
+    } else {
+      /*
+       Kick off a download with default priority.
+       The actual "default" value is decided by the downloader.
+       ASBasicImageDownloader and ASPINRemoteImageDownloader both use ASImageDownloaderPriorityImminent
+       which is mapped to NSURLSessionTaskPriorityDefault.
+
+       This means that preload and display nodes use the same priority
+       and their requests are put into the same pool.
+       */
+      downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
+                                                           callbackQueue:callbackQueue
+                                                        downloadProgress:downloadProgressBlock
+                                                              completion:completion];
+    }
+
+    [strongSelf _setDownloadIdentifier:downloadIdentifier];
+    [strongSelf _updateProgressImageBlockOnDownloaderIfNeeded];
   });
 }