Improve Batch Fetching Call Pattern to Reduce Fetch Calls (#2675)

* Test & tweak batch fetching implementation

* Tighten the test

* Stop batch fetching at the end of range controller pass

* Clean up the test

* Still check for batch fetching after each frame when scrolling

* Ensure batch fetching happens for empty collection/table

Author: Adlai Holler

AsyncDisplayKit/ASCollectionView.mm

@@ -174,6 +174,15 @@ @interface ASCollectionView () <ASRangeControllerDataSource, ASRangeControllerDe
    * The collection view never queried your data source before the update to see that it actually had 0 items.
    */
   BOOL _superIsPendingDataLoad;
+
+  /**
+   * It's important that we always check for batch fetching at least once, but also
+   * that we do not check for batch fetching for empty updates (as that may cause an infinite
+   * loop of batch fetching, where the batch completes and performBatchUpdates: is called without
+   * actually making any changes.) So to handle the case where a collection is completely empty
+   * (0 sections) we always check at least once after each update (initial reload is the first update.)
+   */
+  BOOL _hasEverCheckedForBatchFetchingDueToUpdate;
 
   struct {
     unsigned int scrollViewDidScroll:1;
@@ -1080,6 +1089,7 @@ - (void)scrollViewDidScroll:(UIScrollView *)scrollView
   ASInterfaceState interfaceState = [self interfaceStateForRangeController:_rangeController];
   if (ASInterfaceStateIncludesVisible(interfaceState)) {
     [_rangeController updateCurrentRangeWithMode:ASLayoutRangeModeFull];
+    [self _checkForBatchFetching];
   }
 
   for (_ASCollectionViewCell *collectionCell in _cellsForVisibilityUpdates) {
@@ -1103,7 +1113,7 @@ - (void)scrollViewWillEndDragging:(UIScrollView *)scrollView withVelocity:(CGPoi
 
   if (targetContentOffset != NULL) {
     ASDisplayNodeAssert(_batchContext != nil, @"Batch context should exist");
-    [self _beginBatchFetchingIfNeededWithScrollView:self forScrollDirection:[self scrollDirection] contentOffset:*targetContentOffset];
+    [self _beginBatchFetchingIfNeededWithContentOffset:*targetContentOffset];
   }
 
   if (_asyncDelegateFlags.scrollViewWillEndDragging) {
@@ -1244,9 +1254,10 @@ - (BOOL)canBatchFetch
 - (void)_scheduleCheckForBatchFetchingForNumberOfChanges:(NSUInteger)changes
 {
   // Prevent fetching will continually trigger in a loop after reaching end of content and no new content was provided
-  if (changes == 0) {
+  if (changes == 0 && _hasEverCheckedForBatchFetchingDueToUpdate) {
     return;
   }
+  _hasEverCheckedForBatchFetchingDueToUpdate = YES;
 
   // Push this to the next runloop to be sure the scroll view has the right content size
   dispatch_async(dispatch_get_main_queue(), ^{
@@ -1261,12 +1272,12 @@ - (void)_checkForBatchFetching
     return;
   }
 
-  [self _beginBatchFetchingIfNeededWithScrollView:self forScrollDirection:[self scrollableDirections] contentOffset:self.contentOffset];
+  [self _beginBatchFetchingIfNeededWithContentOffset:self.contentOffset];
 }
 
-- (void)_beginBatchFetchingIfNeededWithScrollView:(UIScrollView<ASBatchFetchingScrollView> *)scrollView forScrollDirection:(ASScrollDirection)scrollDirection contentOffset:(CGPoint)contentOffset
+- (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset
 {
-  if (ASDisplayShouldFetchBatchForScrollView(self, scrollDirection, contentOffset)) {
+  if (ASDisplayShouldFetchBatchForScrollView(self, self.scrollDirection, self.scrollableDirections, contentOffset)) {
     [self _beginBatchFetching];
   }
 }
@@ -1521,11 +1532,6 @@ - (void)rangeController:(ASRangeController *)rangeController didEndUpdatesAnimat
   _performingBatchUpdates = NO;
 }
 
-- (void)didCompleteUpdatesInRangeController:(ASRangeController *)rangeController
-{
-  [self _checkForBatchFetching];
-}
-
 - (void)rangeController:(ASRangeController *)rangeController didInsertNodes:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions
 {
   ASDisplayNodeAssertMainThread();

AsyncDisplayKit/ASTableView.mm

@@ -140,6 +140,9 @@ @interface ASTableView () <ASRangeControllerDataSource, ASRangeControllerDelegat
   BOOL _performingBatchUpdates;
   NSMutableSet *_cellsForVisibilityUpdates;
 
+  // See documentation on same property in ASCollectionView
+  BOOL _hasEverCheckedForBatchFetchingDueToUpdate;
+
   // The section index overlay view, if there is one present.
   // This is useful because we need to measure our row nodes against (width - indexView.width).
   __weak UIView *_sectionIndexView;
@@ -1040,6 +1043,7 @@ - (void)scrollViewDidScroll:(UIScrollView *)scrollView
   ASInterfaceState interfaceState = [self interfaceStateForRangeController:_rangeController];
   if (ASInterfaceStateIncludesVisible(interfaceState)) {
     [_rangeController updateCurrentRangeWithMode:ASLayoutRangeModeFull];
+    [self _checkForBatchFetching];
   }
 
   for (_ASTableViewCell *tableCell in _cellsForVisibilityUpdates) {
@@ -1062,7 +1066,7 @@ - (void)scrollViewWillEndDragging:(UIScrollView *)scrollView withVelocity:(CGPoi
 
   if (targetContentOffset != NULL) {
     ASDisplayNodeAssert(_batchContext != nil, @"Batch context should exist");
-    [self _beginBatchFetchingIfNeededWithScrollView:self forScrollDirection:[self scrollDirection] contentOffset:*targetContentOffset];
+    [self _beginBatchFetchingIfNeededWithContentOffset:*targetContentOffset];
   }
 
   if (_asyncDelegateFlags.scrollViewWillEndDragging) {
@@ -1168,9 +1172,10 @@ - (BOOL)canBatchFetch
 - (void)_scheduleCheckForBatchFetchingForNumberOfChanges:(NSUInteger)changes
 {
   // Prevent fetching will continually trigger in a loop after reaching end of content and no new content was provided
-  if (changes == 0) {
+  if (changes == 0 && _hasEverCheckedForBatchFetchingDueToUpdate) {
     return;
   }
+  _hasEverCheckedForBatchFetchingDueToUpdate = YES;
 
   // Push this to the next runloop to be sure the scroll view has the right content size
   dispatch_async(dispatch_get_main_queue(), ^{
@@ -1185,12 +1190,12 @@ - (void)_checkForBatchFetching
     return;
   }
 
-  [self _beginBatchFetchingIfNeededWithScrollView:self forScrollDirection:[self scrollableDirections] contentOffset:self.contentOffset];
+  [self _beginBatchFetchingIfNeededWithContentOffset:self.contentOffset];
 }
 
-- (void)_beginBatchFetchingIfNeededWithScrollView:(UIScrollView<ASBatchFetchingScrollView> *)scrollView forScrollDirection:(ASScrollDirection)scrollDirection contentOffset:(CGPoint)contentOffset
+- (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset
 {
-  if (ASDisplayShouldFetchBatchForScrollView(self, scrollDirection, contentOffset)) {
+  if (ASDisplayShouldFetchBatchForScrollView(self, self.scrollDirection, ASScrollDirectionVerticalDirections, contentOffset)) {
     [self _beginBatchFetching];
   }
 }
@@ -1329,11 +1334,6 @@ - (void)rangeController:(ASRangeController *)rangeController didEndUpdatesAnimat
   }
 }
 
-- (void)didCompleteUpdatesInRangeController:(ASRangeController *)rangeController
-{
-  [self _checkForBatchFetching];
-}
-
 - (void)rangeController:(ASRangeController *)rangeController didInsertNodes:(NSArray *)nodes atIndexPaths:(NSArray *)indexPaths withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions
 {
   ASDisplayNodeAssertMainThread();

AsyncDisplayKit/Details/ASRangeController.h

@@ -159,13 +159,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)rangeController:(ASRangeController * )rangeController didEndUpdatesAnimated:(BOOL)animated completion:(void (^)(BOOL))completion;
 
-/**
- * Completed updates to cell node addition and removal.
- *
- * @param rangeController Sender.
- */
-- (void)didCompleteUpdatesInRangeController:(ASRangeController *)rangeController;
-
 /**
  * Called for nodes insertion.
  *

AsyncDisplayKit/Details/ASRangeController.mm

@@ -379,7 +379,6 @@ - (void)_updateVisibleNodeIndexPaths
   [modifiedIndexPaths sortUsingSelector:@selector(compare:)];
   NSLog(@"Range update complete; modifiedIndexPaths: %@", [self descriptionWithIndexPaths:modifiedIndexPaths]);
 #endif
-  [_delegate didCompleteUpdatesInRangeController:self];
 
   ASProfilingSignpostEnd(1, self);
 }

AsyncDisplayKit/Private/ASBatchFetching.h

@@ -29,16 +29,18 @@ ASDISPLAYNODE_EXTERN_C_BEGIN
  * ASCollectionView batch fetching API.
  @param context The scroll view that in-flight fetches are happening.
  @param scrollDirection The current scrolling direction of the scroll view.
+ @param scrollableDirections The possible scrolling directions of the scroll view.
  @param targetOffset The offset that the scrollview will scroll to.
  @return Whether or not the current state should proceed with batch fetching.
  */
-BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollView> *scrollView, ASScrollDirection scrollDirection, CGPoint contentOffset);
+BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollView> *scrollView, ASScrollDirection scrollDirection, ASScrollDirection scrollableDirections, CGPoint contentOffset);
 
 
 /**
  @abstract Determine if batch fetching should begin based on the state of the parameters.
  @param context The batch fetching context that contains knowledge about in-flight fetches.
  @param scrollDirection The current scrolling direction of the scroll view.
+ @param scrollableDirections The possible scrolling directions of the scroll view.
  @param bounds The bounds of the scrollview.
  @param contentSize The content size of the scrollview.
  @param targetOffset The offset that the scrollview will scroll to.
@@ -49,6 +51,7 @@ BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollVi
  */
 extern BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
                                                 ASScrollDirection scrollDirection,
+                                                ASScrollDirection scrollableDirections,
                                                 CGRect bounds,
                                                 CGSize contentSize,
                                                 CGPoint targetOffset,

AsyncDisplayKit/Private/ASBatchFetching.m

@@ -10,7 +10,7 @@
 
 #import "ASBatchFetching.h"
 
-BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollView> *scrollView, ASScrollDirection scrollDirection, CGPoint contentOffset)
+BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollView> *scrollView, ASScrollDirection scrollDirection, ASScrollDirection scrollableDirections, CGPoint contentOffset)
 {
   // Don't fetch if the scroll view does not allow
   if (![scrollView canBatchFetch]) {
@@ -22,11 +22,12 @@ BOOL ASDisplayShouldFetchBatchForScrollView(UIScrollView<ASBatchFetchingScrollVi
   CGRect bounds = scrollView.bounds;
   CGSize contentSize = scrollView.contentSize;
   CGFloat leadingScreens = scrollView.leadingScreensForBatching;
-  return ASDisplayShouldFetchBatchForContext(context, scrollDirection, bounds, contentSize, contentOffset, leadingScreens);
+  return ASDisplayShouldFetchBatchForContext(context, scrollDirection, scrollableDirections, bounds, contentSize, contentOffset, leadingScreens);
 }
 
 BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
                                          ASScrollDirection scrollDirection,
+                                         ASScrollDirection scrollableDirections,
                                          CGRect bounds,
                                          CGSize contentSize,
                                          CGPoint targetOffset,
@@ -37,19 +38,14 @@ BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
     return NO;
   }
 
-  // Only Down and Right scrolls are currently supported (tail loading)
-  if (!ASScrollDirectionContainsDown(scrollDirection) && !ASScrollDirectionContainsRight(scrollDirection)) {
-    return NO;
-  }
-
   // No fetching for null states
-  if (leadingScreens <= 0.0 || CGRectEqualToRect(bounds, CGRectZero)) {
+  if (leadingScreens <= 0.0 || CGRectIsEmpty(bounds)) {
     return NO;
   }
 
   CGFloat viewLength, offset, contentLength;
 
-  if (ASScrollDirectionContainsDown(scrollDirection)) {
+  if (ASScrollDirectionContainsVerticalDirection(scrollableDirections)) {
     viewLength = bounds.size.height;
     offset = targetOffset.y;
     contentLength = contentSize.height;
@@ -61,9 +57,14 @@ BOOL ASDisplayShouldFetchBatchForContext(ASBatchContext *context,
 
   // target offset will always be 0 if the content size is smaller than the viewport
   BOOL hasSmallContent = offset == 0.0 && contentLength < viewLength;
+  if (hasSmallContent) {
+    return YES;
+  }
+
+  BOOL isScrollingTowardEnd = (ASScrollDirectionContainsDown(scrollDirection) || ASScrollDirectionContainsRight(scrollDirection));
 
   CGFloat triggerDistance = viewLength * leadingScreens;
   CGFloat remainingDistance = contentLength - viewLength - offset;
 
-  return hasSmallContent || remainingDistance <= triggerDistance;
+  return isScrollingTowardEnd && remainingDistance <= triggerDistance;
 }