Skip to content

Commit

Permalink
ship the new updater
Browse files Browse the repository at this point in the history
Summary:
It's time to ship the new updater! `IGListExperimentalAdapterUpdater` has been running on Instagram for a couple months with better performance and stability. In this diff, we're renaming `IGListExperimentalAdapterUpdater` (and related classes) to `IGListAdapterUpdater`.

Here's a recap:

## Stability

* `[IGListAdapter setDataSource]` isn't safe
  * We're changing the underlying data without telling the `UICollectionView`.
  * Fix: Lets invalidate the `UICollectionView` data by changing its dataSource.
* `[IGListAdapter setCollectionView]` isn't safe
  * This is synchronous, but we might have pending or on-going updates. The `UICollectionView` might get synced before the pending update actually start executing, so the diff results will be off.
  * Fix: Lets wrap updates in a transaction that can be cancelled.
* Returning a nil `IGListSectionController` from `IGListAdapterDataSource` could crash
  * The `IGListAdapterUpdater` will still perform the diffing assuming that all the objects will have a section, which isn't the case.
  * Fix: Lets generate the `IGListSectionController` before the diffing.
* Other improvements
  * Lets ask for the `fromObject` just before diffing, instead of asking when scheduling the update.
  * If the `UICollectionView` section count doesn't match `fromObject`, lets fallback to a reload.

## Performance

* Re-test background diffing
  * `IGListExperimentBackgroundDiffing` and coalescing updates wasn't safe because of the issues mentioned above. The longer we wait, the more likely we'll end up in a race condition. Lets try re-testing with the stability improvements.
* Unblocks background layout calculation
  * This is a larger project, but these improvements are required to make background work safe.
* Only create the `backgroundView` if needed (although this doesn't really require the new updater)

## Other

* Transactions
  * `IGListAdapterUpdater` is the workhorse of `IGListKit` and has become a bit hard to follow over the years. We want to break it apart into simpler, more manageable parts.
* Avoid blocks
  * There's a lot of blocks flying around, making crash logs hard to read. Lets try to use methods/functions where possible.

Reviewed By: Haud, lorixx

Differential Revision: D25884782

fbshipit-source-id: 1357fa23513a239051d5b1766823effa3199f656
  • Loading branch information
maxolls authored and facebook-github-bot committed Jan 22, 2021
1 parent 3b47aa2 commit 247e7ca
Show file tree
Hide file tree
Showing 16 changed files with 1,327 additions and 5,169 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Source/IGListDiffKit/IGListExperiments.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ typedef NS_OPTIONS (NSInteger, IGListExperiment) {
IGListExperimentBackgroundDiffing = 1 << 2,
/// Test invalidating layout when cell reloads/updates in IGListBindingSectionController.
IGListExperimentInvalidateLayoutForUpdates = 1 << 3,
/// Test skipping performBatchUpdate if we don't have any updates. `IGListExperimentalAdapterUpdater` only.
/// Test skipping performBatchUpdate if we don't have any updates.
IGListExperimentSkipPerformUpdateIfPossible = 1 << 4,
/// Test skipping creating {view : section controller} map, which has inconsistency issue.
IGListExperimentSkipViewSectionControllerMap = 1 << 5
Expand Down
3 changes: 2 additions & 1 deletion Source/IGListKit/IGListAdapterUpdater.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#import <IGListDiffKit/IGListMacros.h>
#import <IGListKit/IGListAdapterUpdaterCompatible.h>
#import <IGListKit/IGListUpdatingDelegateExperimental.h>

NS_ASSUME_NONNULL_BEGIN

Expand All @@ -22,7 +23,7 @@ NS_ASSUME_NONNULL_BEGIN
*/
IGLK_SUBCLASSING_RESTRICTED
NS_SWIFT_NAME(ListAdapterUpdater)
@interface IGListAdapterUpdater : NSObject <IGListAdapterUpdaterCompatible>
@interface IGListAdapterUpdater : NSObject <IGListAdapterUpdaterCompatible, IGListUpdatingDelegateExperimental>

@end

Expand Down
571 changes: 158 additions & 413 deletions Source/IGListKit/IGListAdapterUpdater.m

Large diffs are not rendered by default.

32 changes: 0 additions & 32 deletions Source/IGListKit/IGListExperimentalAdapterUpdater.h

This file was deleted.

Loading

0 comments on commit 247e7ca

Please sign in to comment.