Skip to content

Commit

Permalink
graduate IGListExperimentBackgroundDiffing to a property
Browse files Browse the repository at this point in the history
Summary:
`IGListExperimentBackgroundDiffing` has been running on Instagram for a couple months now, and we're seeing nice scroll performance improvements (less frame drops) without increasing crashes. Lets move the experiment to a real property!

Should it be enabled by default?
   * I'm leaning toward no, but I could be convinced otherwise. This optimization is good for large/complex applications with lots of diffing, but I'm not sure the more common apps would need it initially. It does make updates more complicated and the order of operation will be a bit different. For example, we've seen some places call `-performUpdatesAnimated` and almost immediately expect the `IGListAdapter` data to be updated, leading to missing views. This issue can still happen without background diffing, but less often.

Reviewed By: joetam

Differential Revision: D25884786

fbshipit-source-id: 3c8755a774f63868b7dfbc8e7a2e5c012a9e7e27
  • Loading branch information
maxolls authored and facebook-github-bot committed Jan 22, 2021
1 parent 032e1b0 commit 9a11f6b
Show file tree
Hide file tree
Showing 9 changed files with 20 additions and 10 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag

- Removed `allowsBackgroundReloading` from `IGListAdapterUpdater` because it's causing performance issues and other bugs. [Maxime Ollivier](https://github.com/maxolls) (tbd)

- Introducing `allowsBackgroundDiffing` on `IGListAdapterUpdater`! This property lets the updater perform the diffing on a background thread. Originally introduced by Ryan Nystrom a while back. [Maxime Ollivier](https://github.com/maxolls) (tbd)

### Enhancements

- Added `shouldSelectItemAtIndex:` to `IGListSectionController` . [dirtmelon](https://github.com/dirtmelon)
Expand Down
8 changes: 3 additions & 5 deletions Source/IGListDiffKit/IGListExperiments.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,12 @@ NS_SWIFT_NAME(ListExperiment)
typedef NS_OPTIONS (NSInteger, IGListExperiment) {
/// Specifies no experiments.
IGListExperimentNone = 1 << 1,
/// Test updater diffing performed on a background queue.
IGListExperimentBackgroundDiffing = 1 << 2,
/// Test invalidating layout when cell reloads/updates in IGListBindingSectionController.
IGListExperimentInvalidateLayoutForUpdates = 1 << 3,
IGListExperimentInvalidateLayoutForUpdates = 1 << 2,
/// Test skipping performBatchUpdate if we don't have any updates.
IGListExperimentSkipPerformUpdateIfPossible = 1 << 4,
IGListExperimentSkipPerformUpdateIfPossible = 1 << 3,
/// Test skipping creating {view : section controller} map, which has inconsistency issue.
IGListExperimentSkipViewSectionControllerMap = 1 << 5
IGListExperimentSkipViewSectionControllerMap = 1 << 4
};

/**
Expand Down
7 changes: 7 additions & 0 deletions Source/IGListKit/IGListAdapterUpdater.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ NS_SWIFT_NAME(ListAdapterUpdater)
*/
@property (nonatomic, assign) BOOL allowsReloadingOnTooManyUpdates;

/**
Allow the diffing to be performed on a background thread.
Default is NO.
*/
@property (nonatomic, assign) BOOL allowsBackgroundDiffing;

/**
A bitmask of experiments to conduct on the updater.
*/
Expand Down
1 change: 1 addition & 0 deletions Source/IGListKit/IGListAdapterUpdater.m
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ - (void)update {
.singleItemSectionUpdates = _singleItemSectionUpdates,
.preferItemReloadsForSectionReloads = _preferItemReloadsForSectionReloads,
.allowsReloadingOnTooManyUpdates = _allowsReloadingOnTooManyUpdates,
.allowsBackgroundDiffing = _allowsBackgroundDiffing,
.experiments = _experiments,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ @implementation IGListAdapterUpdater (DebugDescription)
[NSString stringWithFormat:@"sectionMovesAsDeletesInserts: %@", IGListDebugBOOL(self.sectionMovesAsDeletesInserts)],
[NSString stringWithFormat:@"singleItemSectionUpdates: %@", IGListDebugBOOL(self.singleItemSectionUpdates)],
[NSString stringWithFormat:@"preferItemReloadsForSectionReloads: %@", IGListDebugBOOL(self.preferItemReloadsForSectionReloads)],
[NSString stringWithFormat:@"allowsReloadingOnTooManyUpdates: %@", IGListDebugBOOL(self.allowsReloadingOnTooManyUpdates)]
[NSString stringWithFormat:@"allowsReloadingOnTooManyUpdates: %@", IGListDebugBOOL(self.allowsReloadingOnTooManyUpdates)],
[NSString stringWithFormat:@"allowsBackgroundDiffing: %@", IGListDebugBOOL(self.allowsBackgroundDiffing)]
];
[debug addObjectsFromArray:IGListDebugIndentedLines(options)];

Expand Down
2 changes: 1 addition & 1 deletion Source/IGListKit/Internal/IGListBatchUpdateTransaction.m
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ - (void)_diff {
IGListTransitionData *data = self.sectionData;
[self.delegate listAdapterUpdater:self.updater willDiffFromObjects:data.fromObjects toObjects:data.toObjects];

const BOOL onBackground = IGListExperimentEnabled(self.config.experiments, IGListExperimentBackgroundDiffing);
const BOOL onBackground = self.config.allowsBackgroundDiffing;
if (onBackground) {
__weak __typeof__(self) weakSelf = self;
dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{
Expand Down
1 change: 1 addition & 0 deletions Source/IGListKit/Internal/IGListUpdateTransactable.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ typedef struct {
BOOL singleItemSectionUpdates;
BOOL preferItemReloadsForSectionReloads;
BOOL allowsReloadingOnTooManyUpdates;
BOOL allowsBackgroundDiffing;
IGListExperiment experiments;
} IGListUpdateTransactationConfig;

Expand Down
4 changes: 2 additions & 2 deletions Tests/IGListAdapterE2ETests.m
Original file line number Diff line number Diff line change
Expand Up @@ -2377,7 +2377,7 @@ - (void)test_whenSchedulingItemUpdate_thenChangeDataSource_thatLastestDataIsAppl

- (void)test_whenSchedulingSectionUpdate_thenBeginDiffing_thenChangeCollectionView_thatLastestDataIsApplied {
IGListAdapterUpdater *updater = (IGListAdapterUpdater *)self.updater;
updater.experiments |= IGListExperimentBackgroundDiffing;
updater.allowsBackgroundDiffing = YES;

[self setupWithObjects:@[
genTestObject(@0, @"Foo")
Expand Down Expand Up @@ -2437,7 +2437,7 @@ - (void)test_whenSchedulingSectionUpdate_thenBeginDiffing_thenChangeCollectionVi

- (void)test_whenSchedulingSectionUpdate_thenBeginDiffing_thenChangeTheDataSource_thatLastestDataIsApplied {
IGListAdapterUpdater *updater = (IGListAdapterUpdater *)self.updater;
updater.experiments |= IGListExperimentBackgroundDiffing;
updater.allowsBackgroundDiffing = YES;

[self setupWithObjects:@[
genTestObject(@0, @"Foo")
Expand Down
2 changes: 1 addition & 1 deletion Tests/IGListAdapterUpdaterTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -868,7 +868,7 @@ - (void)test_whenPerformingUpdatesMultipleTimesInARow_thenUpdateWorks {
}

- (void)test_whenPerformingUpdate_thatCallsDiffingDelegate {
self.updater.experiments |= IGListExperimentBackgroundDiffing;
self.updater.allowsBackgroundDiffing = YES;

NSArray *from = @[
[IGSectionObject sectionWithObjects:@[] identifier:@"0"]
Expand Down

0 comments on commit 9a11f6b

Please sign in to comment.