Data Controller Batch Change Sorting

[Adlai-Holler]

⚠️ Not ready to merge ⚠️

I may be way off in the woods here, so I'm looking for comments on the general direction before I continue to testing and refining. Don't hold back!

• Create a new _ASHierarchyChangeSet class that stores and processes changes. The intention here is, you submit your changes with their animation options in arbitrary order, and when you're done it generates sorted changes, coalesced by animationOptions. This way we have the minimum possible number of updates but still in a safe order for use by ASDataController.
• In ASDataController our enqueued edit command blocks are now (_ASHierarchyChangeSet -> Void). During endUpdates we create a change set, run all the blocks to load it up with data, then call markCompleted so the changeset does its logic, then we do accessDataSourceWithBlock and go through the sorted, coalesced changes, applying them using the same code that we USED to use in the edit command block.

I copied & pasted the edit command code pretty directly, so I'm not sure if e.g. we can get rid of some of the [editTransactionQueue waitUntilAllOperationsAreFinished] or what the fastest safe behavior we can achieve there is.

@nguyenhuy @appleguy

[Adlai-Holler]

#684

[nguyenhuy]

Great progress, @Adlai-Holler. Thanks a ton.

A quick look at the diff gives me the impression that there are just too many changes in ASDataController. That class is very... hot, mainly because it heavily deals with concurrency. Is there any way we can minimize the changes there?

Not sure if this is a good idea. What I'm having in mind is that ASDataController doesn't shouldn't know about change set at all. Instead, there is a new subclass of ASDataController that intercepts edit calls during a transaction and put them to a change set. Then in endUpdates (of the subclass), it loops through the sorted change set and forward each edit call to its super class-- i.e ASDataController. That way, the new class encapsulates the knowledge about change set and ASDataController remains relatively untouched. What do you think?

[appleguy]

This is really awesome, thanks so much @Adlai-Holler for your large-scale contributions! I will review in detail this weekend.

Let me start by saying that I'm not afraid of a change that hugely changes ASDataController — obviously it would need to be rigorously tested, but let's face it, the class needs some upgrades. The critical piece: we must get the architecture RIGHT. It isn't worth the testing pain of a really big change if we can spot any difficult limitations of its structure that wouldn't be resolvable with later improvements moving in the same direction.

That said, it does seem likely that we could solve this problem with a more surgical change. I'd buy it that this direction may be BETTER despite the higher initial risk, if we believe it results in code that is easier to debug, read, maintain, extend, or more performant — or any combination of a few of those.

@Adlai-Holler, what's your general philosophy on this? @levi, @eanagel - interested to hear your thoughts. Don't worry guys, I won't merge something like this without personally helping to test it heavily with stressful use cases in Pinterest and upgraded thrash testers, and I will also not merge something that breaks any client's app — so let's critique and brainstorm what the ideal architectural approach is before worrying about the risk of any given change.

[Adlai-Holler]

@nguyenhuy I really dig your idea! It's a very natural way to implement this buffering/sorting.

@appleguy You pretty much said what I'd say, that size of change is less important than suitability of the architecture and that something as fiddly as this demands an insane amount of tests.

As I look back on it, this PR is compromised – it goes too far for a newcomer to just drop in, but not far enough to justify it. I'll close it and create a new PR that has a better benefit:fear ratio. Sometime in the future we'll update the architecture together and I can discharge my obsession with this problem at that time. Cheers guys!