[ASDataController] Refactor ASDataController #3017

nguyenhuy · 2017-02-11T01:41:01Z

Author: Thanh Huy Nguyen

This PR has 2 main goals:

Simplify ASDataController and overhaul the way it processes change sets.
- ASDataController now processes a change set in 3 steps:
  - Synchronizes its internal states with data source on main thread.
  - Switches to a background queue to layout new nodes.
  - Switches back to main to forward the change set to UIKit.
- Each step consumes the whole change set before switching to another thread for the next step. The data shared between these steps are immutable. This helps to guarantee thread safety and data consistency.
- Each step may interpret a change set differently. The first step decomposes reloads into deletes and inserts, while the last step forwards the change set as-is. As a result, the implementation of -reloadData is much simpler. We finally can destroy everything at once and repopulate the whole data set. And flashes should be gone now ([Discussion] Solutions to eliminate flashes caused by reloadData #2536). Furthermore, it's much easier to support native moves ([ASDataController] "Move" Operations Animate Incorrectly (Delete + Insert) #698, [ASDataController] Move animates using UIKit native behaviour #2868) and reloads (reloadSections(_, withRowAnimation:_) does not reload the section header view #469).
Merge ASCollectionDataController into ASDataController.
- ASCollectionDataController subclasses ASDataController to support supplementary views. In order to achieve that, it requires many hooks during the update process. Those hooks add complexity to ASDataController. Moreover, ASCollectionDataController doesn't handle supplementary node contexts correctly. It doesn't persistently store those contexts which is important for upcoming features (i.e node virtualization and measurement range).
- Even after [ASCollectionView] Repopulate supplementary views on item-level changes #1629 and [ASCollectionDataController] Repopulate deleted supplementary nodes if necessary #1773, repopulating supplementary nodes still doesn't work perfectly. After merging the 2 classes, it's much easier to rewrite this part.

Remaining tasks:

Fix failing unit tests
More testing on various projects
Revisit TODOs
Support reloads (reloadSections(_, withRowAnimation:_) does not reload the section header view #469) natively
Support moves ([ASDataController] "Move" Operations Animate Incorrectly (Delete + Insert) #698, [ASDataController] Move animates using UIKit native behaviour #2868) natively
Revisit -[ASDataController relayoutAllNodes] and its callees.
Consider changing _nodeContexts to be a two dimension array of dictionaries (instead of a dictionary of two dimensional arrays).

This PR is still a WIP and contains many TODOs. However, I'd appreciate any early feedbacks and help for testing. Thanks!

Ticket: TextureGroup/Texture#186

Adlai-Holler

This is an absolutely massive improvement in data controller! I love it, and we're gonna be able to move much much faster from here on out. Left some comments but hopefully we can land ASAP.

Adlai-Holler · 2017-02-13T21:38:22Z

AsyncDisplayKit/ASCellNode.h

 */
 @property (nonatomic, assign) BOOL neverShowPlaceholders;

+//TODO shouldn't this be a generic kind (e.g includes ASDataControllerRowNodeKind)?


Since ASDataControllerRowNodeKind is private, we should expose that as nil.

I guess we have a common ground that this property should be called kind and exposed as nil for row kind?

On the other hand, can ASDataControllerRowNodeKind possibly be exposed/public?

Yeah, I'm down for kind or elementKind 👍

Adlai-Holler · 2017-02-13T21:41:02Z

AsyncDisplayKit/ASTableView.mm

+{
+  [self endUpdatesCompletion:nil];
+}
+


Why add this method?

Right on! Removing...

Adlai-Holler · 2017-02-13T21:43:30Z

AsyncDisplayKit/ASTableView.mm

-  }
+  //TODO Reimplement _automaticallyAdjustsContentOffset
+//  if (_automaticallyAdjustsContentOffset) {
+//    [self beginAdjustingContentOffset];


This flag has a very tumultuous history, but we should really strive to support it in every version unless we make it unavailable. If it's absolutely unbearable to implement – even in a rough way – we should be sure to put in the release notes that this flag is not functional in this version.

Adlai-Holler · 2017-02-13T21:44:28Z

AsyncDisplayKit/ASTableView.mm

+    if (!_performingBatchUpdates) {
+      [_rangeController updateIfNeeded];
+    }
+    // TODO Should reloadData triggers batch fetching?


I believe it should. If I reload data, and I don't have e.g. 2 screenfuls of data ahead of me, I should attempt to batch fetch.

Adlai-Holler · 2017-02-13T21:45:10Z

AsyncDisplayKit/ASTableView.mm

+  });
+
+  //TODO is _automaticallyAdjustsContentOffset needed for reloadData?
+//  if (_automaticallyAdjustsContentOffset) {


Hmm good question. I think it should not apply to reloadData because we have to assume that every single item was replaced by something completely different.

Adlai-Holler · 2017-02-13T21:45:35Z

AsyncDisplayKit/Details/ASCollectionDataController.h

-
-@end
-
-NS_ASSUME_NONNULL_END


WOW I'm glad to see this file gone ⚡️ 🎉 🌴

Super great!!! Thanks @nguyenhuy!

Adlai-Holler · 2017-02-13T21:47:22Z

AsyncDisplayKit/Details/ASDataController.h

 */
- (void)dataController:(ASDataController *)dataController didInsertSections:(NSArray<NSArray<ASCellNode *> *> *)sections atIndexSet:(NSIndexSet *)indexSet withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions;
+- (void)dataController:(ASDataController *)dataController didInsertSectionsAtIndexSet:(NSIndexSet *)indexSet withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions;



Yes! Very happy to see this argument removed. Let's phrase it didInsertSections:(NSIndexSet *)sectionIndexes since it's clear that we're passing an index set.

Adlai-Holler · 2017-02-13T21:49:42Z

AsyncDisplayKit/Details/ASDataController.mm

+        _completedNodeContexts = loadingNodeContexts;
+
+        // Step 3: Now that _completedNodeContexts is ready, call UIKit to update using the original change set.
+        [_delegate dataControllerDidReloadData:self];


Nice! Formal reloadData!

Adlai-Holler · 2017-02-13T21:54:23Z

AsyncDisplayKit/Details/ASDataController.mm

+        _completedNodeContexts = loadingNodeContexts;
+
+        // Step 3: Now that _completedNodeContexts is ready, call UIKit to update using the original change set.
+        [self _forwardChangeSetToDelegate:changeSet animated:animated];


How hard would it be to fuse reloadData into updateWithChangeSet: by adding a flag like _ASHierarchyChangeSet.includesReloadData? Much of the internal processing of _ASHierarchyChangeSet would be skipped when that flag is set, and we use it to determine e.g. how to report to the delegate.

That way there's only one implementation of this core update method.

Adlai-Holler · 2017-02-13T21:56:48Z

AsyncDisplayKit/Details/ASDataController.mm

-  ASMoveElementInTwoDimensionalArray(_externalCompletedNodes, indexPath, newIndexPath);
-  ASMoveElementInTwoDimensionalArray(_completedNodes[ASDataControllerRowNodeKind], indexPath, newIndexPath);
+  NSMutableArray<ASNodeContextTwoDimensionalArray *> *deepCopy = [NSMutableArray arrayWithCapacity:originalDict.count];
+  for (ASNodeContextTwoDimensionalArray *sections in originalDict.allValues) {


Rather than copying the array with allValues, let's use objectEnumerator here and below.

Check optional methods in ASDataControllerSource

- No more new/inserted contexts flag - Encapsulate code shared between reloadData and updateWithChangeSet - Remove dataControllerWillDeleteAllData delegate method - Hierarchy changes no longer needs to conform to NSCopying - Reword TODOs

…nullable

… index space - This helps to avoid immature node allocation, especially when node virtualization is a thing - However, this means that -reloadDataInitiallyIfNeeded in ASCollectionNode and ASTableNode must wait until all updates are finished.

… out

…in UIKit index space" This reverts commit 7bc977b.

…h the assumption that clients absolutely need it. - Revisit this when node virtualization is implemented.

…cause it'll be niled out

…fore their data controller does anything

- It can't be used in conjuntion with other updates. - Calling it inside a batch update during the initial load can cause data inconsistency thrown by UICollectionView/UITableView

- Replace delegate methods in these protocols with -willUpdateWithChangeSet and -didUpdateWithChangeSet. - ASRangeController, ASCollectionView and ASTableView are simplified because of this.

…ctionView's reloadData - Since UICollectionView's reloadData doesn't requery data source but defers until the next layout pass, we need to wait until then to update range controller and do batch fetching. - `-[ASCollectionView waitUntilAllUpdatesAreCommited]` needs to force a layout pass to make sure everything is ready after it returns.

…e nodes. Other nodes will be re-measured later.

nguyenhuy · 2017-02-21T16:01:15Z

@Adlai-Holler @appleguy This PR is finally ready for review! As it is getting huge, I'd like to merge this ASAP and work on follow up tasks. Thank you!

Adlai-Holler

Absolutely fantastic! I am so so happy to have these changes in. Mostly non-blocking feedback. We can build quite a bit from here! ❤️ @nguyenhuy

Adlai-Holler · 2017-02-21T21:26:21Z

AsyncDisplayKit/Details/ASDataController.mm

-//#define LOG(...) NSLog(__VA_ARGS__)
-#define LOG(...)
+#define LOG(...) NSLog(__VA_ARGS__)
+//#define LOG(...)


Turn logging back off

Adlai-Holler · 2017-02-21T21:28:50Z

AsyncDisplayKit/Details/ASRangeController.h

+ * @param changeSet The change set that includes all updates
 */
- (void)rangeController:(ASRangeController *)rangeController didDeleteItemsAtIndexPaths:(NSArray<NSIndexPath *> *)indexPaths withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions;
+- (void)rangeController:(ASRangeController * )rangeController willUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;;


Nit: Remove the space

Adlai-Holler · 2017-02-21T21:36:25Z

AsyncDisplayKit/Details/ASDataController.h

+ * @param changeSet The change set that includes all updates
 */
- (void)dataController:(ASDataController *)dataController didInsertSections:(NSArray<NSArray<ASCellNode *> *> *)sections atIndexSet:(NSIndexSet *)indexSet withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions;
+- (void)dataController:(ASDataController * )dataController willUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;;


Remove stray space

Adlai-Holler · 2017-02-21T21:36:45Z

AsyncDisplayKit/Details/ASDataController.mm

+// Dictionary with each entry is a pair of "kind" key and two dimensional array of node contexts
+#define ASNodeContextTwoDimensionalDictionary         NSDictionary<NSString *, ASNodeContextTwoDimensionalArray *>
+// Mutable dictionary with each entry is a pair of "kind" key and two dimensional array of node contexts
+#define ASNodeContextTwoDimensionalMutableDictionary  NSMutableDictionary<NSString *, ASNodeContextTwoDimensionalMutableArray *>


Adlai-Holler · 2017-02-21T21:37:44Z

AsyncDisplayKit/Details/ASDataController.mm

 #pragma mark - Cell Layout

- (void)batchLayoutNodesFromContexts:(NSArray<ASIndexedNodeContext *> *)contexts batchCompletion:(ASDataControllerCompletionBlock)batchCompletionHandler
+- (void)batchLayoutNodesFromContexts:(NSArray<ASIndexedNodeContext *> *)contexts batchSize:(NSUInteger)batchSize batchCompletion:(ASDataControllerCompletionBlock)batchCompletionHandler


Nit: When adding new integer variables, let's standardize on NSInteger because it's simpler and the Swift import is way easier.

Adlai-Holler · 2017-02-21T21:42:38Z

AsyncDisplayKit/Details/ASDataController.mm

+  }
+
+  ASDisplayNodeAssert(NO, @"Unknown constrained size for node of kind %@ by data source %@", kind, _dataSource);
+  return ASSizeRangeMake(CGSizeZero, CGSizeZero);


Nit: ASSizeRangeZero

Adlai-Holler · 2017-02-21T21:45:55Z

AsyncDisplayKit/Details/ASRangeController.h

+ * @param changeSet The change set that includes all updates
 */
- (void)rangeController:(ASRangeController *)rangeController didDeleteSectionsAtIndexSet:(NSIndexSet *)indexSet withAnimationOptions:(ASDataControllerAnimationOptions)animationOptions;
+- (void)rangeController:(ASRangeController * )rangeController didUpdateWithChangeSet:(_ASHierarchyChangeSet *)changeSet;;


Remove the space

Adlai-Holler · 2017-02-21T21:46:57Z

AsyncDisplayKit/Private/_ASHierarchyChangeSet.h


 @property (nonatomic, readonly) BOOL completed;
+/// Whether or not changes should be animated.
+@property (nonatomic, readwrite) BOOL animated;


Good call, putting this flag here. My reckoning here is, if any update in the stack is non-animated, the whole update should be non-animated. Would be fine as a follow-up task or discussion, or implement it here.

Adlai-Holler · 2017-02-21T21:47:22Z

AsyncDisplayKit/Private/_ASHierarchyChangeSet.mm

+    if (0 < (_originalDeleteSectionChanges.count + _originalDeleteItemChanges.count
+             +_originalInsertSectionChanges.count + _originalInsertItemChanges.count
+             + _reloadSectionChanges.count + _reloadItemChanges.count)) {
+      ASFailUpdateValidation(@"Attempt to reload data in conjuntion with other updates.");


I think we can probably remove this and be lenient on them. Right?

Adlai-Holler · 2017-02-21T21:47:41Z

AsyncDisplayKit/Private/_ASHierarchyChangeSet.mm

 - (NSMutableArray<NSDictionary *> *)propertiesForDescription
 {
  NSMutableArray<NSDictionary *> *result = [NSMutableArray array];
+  [result addObject:@{ @"_includesReloadData" : @(_includesReloadData) }];


Nit: string should just be "includesReloadData"

Adlai-Holler · 2017-02-22T21:50:03Z

Here, we, go!

facebook-github-bot added the CLA Signed label Feb 11, 2017

nguyenhuy assigned maicki, garrettmoon, appleguy and Adlai-Holler Feb 11, 2017

nguyenhuy force-pushed the HNRefactorDataControllerTake3 branch 2 times, most recently from a07b4a9 to 0d62525 Compare February 11, 2017 08:26

Adlai-Holler reviewed Feb 13, 2017

View reviewed changes

garrettmoon mentioned this pull request Feb 14, 2017

[ASCollectionNode / ASTableNode] Measure scrollable content size (main thread only due to UICV layout?) #814

Closed

nguyenhuy force-pushed the HNRefactorDataControllerTake3 branch from dc9752b to d3a8169 Compare February 15, 2017 16:01

nguyenhuy added 20 commits February 16, 2017 15:58

Refactor ASDataController

946c02f

Check optional methods in ASDataControllerSource

Reimplement reloadData

b0c7f19

Refactor new code

0846ddb

- No more new/inserted contexts flag - Encapsulate code shared between reloadData and updateWithChangeSet - Remove dataControllerWillDeleteAllData delegate method - Hierarchy changes no longer needs to conform to NSCopying - Reword TODOs

Forgot to call completion block of reloadData :P

5906b74

Completion block of -[ASDataController reloadDataWithCompletion:] is …

dffb4cc

…nullable

ASDataController shouldn't assume that allocated nodes were also laid…

bf99461

… out

Revert "Data queried from ASCollectionNode and ASTableNode should be …

b6fddd6

…in UIKit index space" This reverts commit 7bc977b.

-nodeAtIndexPath: of ASDataController now forces node allocation, wit…

2c2b53c

…h the assumption that clients absolutely need it. - Revisit this when node virtualization is implemented.

ASDataController only grab changeSet.completionHandler when needed be…

5fd86af

…cause it'll be niled out

Fix ASTableViewTests related to reloadData

ed90d83

Fix testThatDeletedItemsAreMarkedInvisible in ASCollectionViewTests

32f9fd7

Minor changes in ASCollectionView and ASTableView

d65de41

ASCollectionView and ASTableView shouldn't call [super reloadData] be…

f5fefa7

…fore their data controller does anything

Address comments

9052048

Fuse reloadData into -updateWithChangeSet: of ASDataController

c3f561b

reloadData shouldn't be called as if it's inside a batch

e3f0a49

- It can't be used in conjuntion with other updates. - Calling it inside a batch update during the initial load can cause data inconsistency thrown by UICollectionView/UITableView

Refactor ASDataControllerDelegate and ASRangeControllerDelegate

2c5c5f3

- Replace delegate methods in these protocols with -willUpdateWithChangeSet and -didUpdateWithChangeSet. - ASRangeController, ASCollectionView and ASTableView are simplified because of this.

Fix mismatch between sorting orders in ASDataController

2533ae6

Forgot to call completion handler of reload change sets

c1d38b5

nguyenhuy added 2 commits February 21, 2017 14:00

testSectionIndexHandling of ASTableViewTests should only check visibl…

0983866

…e nodes. Other nodes will be re-measured later.

nguyenhuy force-pushed the HNRefactorDataControllerTake3 branch from d3a8169 to 0983866 Compare February 21, 2017 14:05

ASTableView is not ready until the first layout pass finished

1b20117

nguyenhuy force-pushed the HNRefactorDataControllerTake3 branch from 01423e6 to 1b20117 Compare February 21, 2017 15:32

nguyenhuy unassigned maicki, garrettmoon, appleguy and Adlai-Holler Feb 21, 2017

Adlai-Holler approved these changes Feb 21, 2017

View reviewed changes

nguyenhuy changed the title ~~[WIP][ASDataController] Refactor ASDataController~~ [ASDataController] Refactor ASDataController Feb 21, 2017

Address comments

483f6eb

nguyenhuy force-pushed the HNRefactorDataControllerTake3 branch from b4698a4 to 483f6eb Compare February 22, 2017 12:46

Bug fixes

705a758

Adlai-Holler merged commit 74c6cca into facebookarchive:master Feb 22, 2017

This was referenced Feb 23, 2017

[Discussion] Solutions to eliminate flashes caused by reloadData #2536

Closed

[ASCollectionView/ASTableView] Support native item and section reloads #3066

Merged

Adlai-Holler mentioned this pull request Mar 4, 2017

Don't Eagerly Trigger Relayout in Wait #3134

Merged

nguyenhuy mentioned this pull request Mar 24, 2017

[RFC] The next milestones of ASCollectionNode #3219

Closed

garrettmoon mentioned this pull request May 1, 2017

[ASCollectionNode / ASTableNode] Measure scrollable content size (main thread only due to UICV layout?) TextureGroup/Texture#108

Open

garrettmoon mentioned this pull request May 10, 2017

[RFC] The next milestones of ASCollectionNode TextureGroup/Texture#209

Open


		@end

		NS_ASSUME_NONNULL_END

[ASDataController] Refactor ASDataController #3017

[ASDataController] Refactor ASDataController #3017

Uh oh!

Conversation

nguyenhuy commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Author: Thanh Huy Nguyen

Uh oh!

Adlai-Holler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguyenhuy commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adlai-Holler left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adlai-Holler commented Feb 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nguyenhuy commented Feb 11, 2017 •

edited

Loading

nguyenhuy commented Feb 21, 2017 •

edited

Loading

Adlai-Holler left a comment •

edited

Loading