[ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. #522

appleguy · 2017-08-20T08:39:05Z

Over time, there have actually been a lot of legitimate uses for an API like this.
In fact, I'm not quite sure what has held us back from adding one!

I believe that at least some portion of -wait calls (even if less than 50%) could be
replaced with -onDidFinishProcessingUpdates:, which could potentially improve the
performance of applications using -wait by a significant amount.

Please take a close look at implementation correctness. Although I'm in a bit of a
rush, I'm aiming to make this properly documented and added a basic test -- but it
could certainly use some more detailed testing as a followup.

appleguy · 2017-08-20T08:39:26Z

This could be used to quite easily fix this ASBatchContext edge case: #521

…gUpdates: APIs. Over time, there have actually been a lot of legitimate uses for an API like this. In fact, I'm not quite sure what has held us back from adding one! I believe that at least some portion of -wait calls (even if less than 50%) could be replaced with -onDidFinishProcessingUpdates:, which could potentially improve the performance of applications using -wait by a significant amount. Please take a close look at implementation correctness. Although I'm in a bit of a rush, I'm aiming to make this properly documented and added a basic test -- but it could certainly use some more detailed testing as a followup.

ghost · 2017-08-20T08:41:15Z

🚫 CI failed with log

appleguy · 2017-08-20T09:03:45Z

I see one of the new tests is failing on the build server, although it doesn't fail on my local machine - even trying 9.3, 10.0, and 10.3.1 simulators

I'll wait for the last build to finish and then will start debugging-by-commit-pushing :).

Adlai-Holler

This feature has been a long time coming. More transparency around the state of the collection node means (1) more options during development and (2) more developer understanding of the async nature of collection node.

Comments that merit no more than 10 minutes of work in my estimation.

Adlai-Holler · 2017-08-21T17:40:08Z

Source/ASCollectionNode.h

+ *
+ *  This method will always return NO if called immediately after -waitUntilAllUpdatesAreProcessed.
+ */
+- (BOOL)isProcessingUpdates;


Trivial: Make this a readonly property for better swift interop?

Great suggestion, I'll try to remember this in the future (so foreign to me :-P)

Adlai-Holler · 2017-08-21T17:43:31Z

Source/Details/ASDataController.mm

+{
+  ASDisplayNodeAssertMainThread();
+  long waitResult = dispatch_group_wait(_editingTransactionGroup, DISPATCH_TIME_NOW);
+  return (waitResult == 0 ? NO : YES);


Is it possible that we could return a false NO if they call this in between the time the editing transaction group is finished and the main queue services the data controller's UIKit flush?

We could either flush the main serial queue (but not the editing transaction queue) or add API to check if the main serial queue is empty.

Yeah, this is a good catch. I think this might be possible.

If we flush the main serial queue here, you could get situations where calling isProcessingUpdates results in calls out to the delegate / dataSource, which would be pretty weird and maybe unexpected by clients. So I think I'll just check the length of the queue.

@Adlai-Holler do you think we should perform a setNeedsLayout in the case that the queues are drained, before returning NO? This would technically be needed to ensure all information is flushed to / reflected in the UICollectionViewLayout...

Are you maybe thinking about layoutIfNeeded? That would absolutely make sense 👍

Adlai-Holler · 2017-08-21T17:48:09Z

Source/Details/ASDataController.mm

+      });
+    });
+  }
+}


A couple low-impact notes on this method:

If we're asserting main, is it worth removing ASPerformBlockOnMainThread? Presumably an error of this kind can be reliably caught during development. If the notion here is "better safe than sorry," I'm down with that as well.

Should we schedule the retry on the main serial queue, so that if they wait on updates to complete we can be sure this block is run before returning from the wait? I suppose it's not a big deal – there's no guarantee this block will be run as soon as we finish processing updates. But at the same time, this is a break from the usual pattern of editingQueue -> main serial queue for main-thread-injection into data controller's async work.

Yes, the idea is some extra safety in the case the assertion fails in production. It is not likely to be necessary, but the semantics of PerformOnMain are clear enough that it seems OK.

Interested in your input on the other point. I had originally implemented it as waiting on the mainSerialQueue instead of a dispatch.

I was thinking (and now believe this may be incorrect) that doing it this way would allow us to call out to the blocks in an "intermediate quiesced" state to perform some operations before potentially more edits start getting processed.

However with the changes to isProcessingUpdates, I think this is actually invalid and we should schedule on the serial queue.

@Adlai-Holler is it correct that we do NOT perform using the _editingTransactionGroup? I don't want multiple scheduled blocks to get stuck waiting for each other (as they do schedule in the main serial queue, and so I think this is correct, but I'm not completely confident.

That's right! The transaction group is only (and always) applied to work blocks on the editing transaction queue.

Adlai-Holler · 2017-08-21T17:48:44Z

Tests/ASCollectionViewTests.mm

  // Wait for ASDK reload to finish
  [cn waitUntilAllUpdatesAreCommitted];
+
+  XCTAssertTrue(!cn.isProcessingUpdates, @"ASCollectionNode should no longer be processing updates after -wait call");
+


…gUpdates and -onDidFinishProcessingUpdates:

nguyenhuy

Will accept as soon as you address the locking concern in ASMainSerialQueue.

nguyenhuy · 2017-08-22T16:28:07Z

Source/Details/ASMainSerialQueue.mm

@@ -40,6 +40,11 @@ - (instancetype)init
  return self;
 }

+- (NSUInteger)numberOfScheduledBlocks
+{
+  return _blocks.count;


I think we need to grab _serialQueueLock lock here.

nguyenhuy · 2017-08-22T16:30:04Z

Source/Details/ASDataController.mm

+{
+  ASDisplayNodeAssertMainThread();
+  if (_mainSerialQueue.numberOfScheduledBlocks > 0) {
+    return YES;


nguyenhuy · 2017-08-22T16:31:44Z

Source/ASCollectionNode.h

+- (void)reloadDataImmediately ASDISPLAYNODE_DEPRECATED_MSG("Use -reloadData / -reloadDataWithCompletion: followed by -waitUntilAllUpdatesAreProcessed instead.");
+
+// TODO: Rename framework uses of this method and add deprecation message.
+// ASDISPLAYNODE_DEPRECATED_MSG("This method has been renamed to -waitUntilAllUpdatesAreProcessed.");


If you're not going to do this in this PR, make sure to create an issue to track it.

nguyenhuy

Good to go now!

…ngUpdates:. Rename -waitUntil to consistent naming.

appleguy · 2017-08-22T19:15:54Z

@nguyenhuy @Adlai-Holler thanks for the great feedback! I went ahead and renamed the method from Committed to Processing everywhere except the *View classes, where a deprecation warning already exists and now directs people to the Processing version (it seemed silly to add a second, also deprecated name).

This change makes me realize that we should give AS{Collection,Table}Node access to the ASDataController directly. There are a number of APIs where no participation from the View class is required. Even without breaking support for using the View classes independently (which is becoming less and less important), calls like isProcessingUpdates or waitUntil.. could be passed directly from the Node to ASDataController.

nguyenhuy · 2017-08-22T19:23:23Z

@appleguy: Yes, agree that the nodes should have direct access to data controller. In a far future, we may even tell the data controller to do the initial load before the view is even allocated. Definitely possible with an async collection layout.

nguyenhuy · 2017-08-23T10:16:19Z

ok, let's get this in!

appleguy · 2017-08-24T08:26:13Z

Thanks Huy!

…gUpdates: APIs. (TextureGroup#522) * [ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. Over time, there have actually been a lot of legitimate uses for an API like this. In fact, I'm not quite sure what has held us back from adding one! I believe that at least some portion of -wait calls (even if less than 50%) could be replaced with -onDidFinishProcessingUpdates:, which could potentially improve the performance of applications using -wait by a significant amount. Please take a close look at implementation correctness. Although I'm in a bit of a rush, I'm aiming to make this properly documented and added a basic test -- but it could certainly use some more detailed testing as a followup. * [ASCollectionNode] Improvements to the implementation of -isProcessingUpdates and -onDidFinishProcessingUpdates: * Add lock to ASMainSerialQueue count method. * [ASTableNode] Implement -isProcessingUpdates and -onDidFinishProcessingUpdates:. Rename -waitUntil to consistent naming.

appleguy added API improvement Collection Table Pager labels Aug 20, 2017

appleguy requested review from maicki, nguyenhuy and Adlai-Holler August 20, 2017 08:39

appleguy mentioned this pull request Aug 20, 2017

[ASBatchContext] If user scroll lifts while inserting last batch, new fetch starts early. #521

Open

appleguy force-pushed the ProcessingUpdates branch from 9fa2d2a to 6d687a8 Compare August 20, 2017 08:41

Merge branch 'master' into ProcessingUpdates

de635f2

Adlai-Holler reviewed Aug 21, 2017

View reviewed changes

appleguy added 2 commits August 21, 2017 13:11

Merge branch 'master' into ProcessingUpdates

b3b7384

[ASCollectionNode] Improvements to the implementation of -isProcessin…

b66415b

…gUpdates and -onDidFinishProcessingUpdates:

nguyenhuy requested changes Aug 22, 2017

View reviewed changes

Add lock to ASMainSerialQueue count method.

f8b1fab

nguyenhuy approved these changes Aug 22, 2017

View reviewed changes

[ASTableNode] Implement -isProcessingUpdates and -onDidFinishProcessi…

b54d4d1

…ngUpdates:. Rename -waitUntil to consistent naming.

nguyenhuy merged commit ccc5786 into master Aug 23, 2017

nguyenhuy deleted the ProcessingUpdates branch August 23, 2017 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. #522

[ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. #522

appleguy commented Aug 20, 2017

appleguy commented Aug 20, 2017

ghost commented Aug 20, 2017

appleguy commented Aug 20, 2017

Adlai-Holler left a comment

Adlai-Holler Aug 21, 2017

appleguy Aug 21, 2017

Adlai-Holler Aug 21, 2017

appleguy Aug 21, 2017

appleguy Aug 21, 2017

Adlai-Holler Aug 22, 2017

Adlai-Holler Aug 21, 2017

appleguy Aug 21, 2017

appleguy Aug 21, 2017

Adlai-Holler Aug 22, 2017

Adlai-Holler Aug 21, 2017

nguyenhuy left a comment

nguyenhuy Aug 22, 2017

nguyenhuy Aug 22, 2017

nguyenhuy Aug 22, 2017

nguyenhuy left a comment

appleguy commented Aug 22, 2017

nguyenhuy commented Aug 22, 2017

nguyenhuy commented Aug 23, 2017

appleguy commented Aug 24, 2017

[ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. #522

[ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. #522

Conversation

appleguy commented Aug 20, 2017

appleguy commented Aug 20, 2017

ghost commented Aug 20, 2017

appleguy commented Aug 20, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

appleguy commented Aug 22, 2017

nguyenhuy commented Aug 22, 2017

nguyenhuy commented Aug 23, 2017

appleguy commented Aug 24, 2017