From 032e1b0b8367e68ef3015f0dc7dfe2f3ff2bae0c Mon Sep 17 00:00:00 2001 From: Maxime Ollivier Date: Thu, 21 Jan 2021 19:55:59 -0800 Subject: [PATCH] goodbye allowsBackgroundReloading Summary: Originally, `allowsBackgroundReloading` was added to improve performance, but ironically, it's causing lots of performance issues among other issues. * Performance: Looking back, it's not too surprising that it causes perf issues. We're falling back to a full `-reloadData` if the view is not in the window, which can happen pretty often. For example, if a view-controller is within a `UINavigationController` stack but not on top, or within a `UITabBarController`. Because a full `-reloadData` will re-query the cells and re-create the entire layout, it's going to be more expensive than an incremental update via `-performUpdatesAnimated`. The proof is in the data and we have a few examples where this flag was the cause of significant UI stalls. * Bugs: Because we might reload cells often, it can create strange animation artifacts. Specifically, it was breaking the `UIView` snapshots just before a transition, like the new zoom animator. Overall, we ended disabling this feature and I think most apps will be in the same boat. But what if this flag does improve my app's performance? * File an issue and lets chat! I'd be curious to understand why that's the case. If a full `-reloadData` is more performant than an incremental `-performUpdatesAnimated`, than something odd is happening and I don't think this flag is the right solution. Reviewed By: joetam Differential Revision: D25884777 fbshipit-source-id: c4626a52082ef4c7b7300b21077529f26c551e70 --- CHANGELOG.md | 2 + Source/IGListKit/IGListAdapterUpdater.h | 13 ----- Source/IGListKit/IGListAdapterUpdater.m | 3 -- .../IGListAdapterUpdater+DebugDescription.m | 1 - .../Internal/IGListBatchUpdateTransaction.m | 7 --- .../Internal/IGListUpdateTransactable.h | 1 - Tests/IGListAdapterUpdaterTests.m | 52 ------------------- Tests/IGListBindingSectionControllerTests.m | 1 - 8 files changed, 2 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dfe9f4f..d7733700 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag - (void)performDataSourceChange:(IGListDataSourceChangeBlock)block; ``` +- Removed `allowsBackgroundReloading` from `IGListAdapterUpdater` because it's causing performance issues and other bugs. [Maxime Ollivier](https://github.com/maxolls) (tbd) + ### Enhancements - Added `shouldSelectItemAtIndex:` to `IGListSectionController` . [dirtmelon](https://github.com/dirtmelon) diff --git a/Source/IGListKit/IGListAdapterUpdater.h b/Source/IGListKit/IGListAdapterUpdater.h index e7e92e15..ae80538e 100644 --- a/Source/IGListKit/IGListAdapterUpdater.h +++ b/Source/IGListKit/IGListAdapterUpdater.h @@ -62,19 +62,6 @@ NS_SWIFT_NAME(ListAdapterUpdater) */ @property (nonatomic, assign) BOOL preferItemReloadsForSectionReloads; -/** - A flag indicating whether this updater should skip diffing and simply call - `reloadData` for updates when the collection view is not in a window. The default value is `YES`. - - Default is YES. - - @note This will result in better performance, but will not generate the same delegate - callbacks. If using a custom layout, it will not receive `prepareForCollectionViewUpdates:`. - - @warning On iOS < 8.3, this behavior is unsupported and will always be treated as `NO`. - */ -@property (nonatomic, assign) BOOL allowsBackgroundReloading; - /** If there's more than 100 diff updates, fallback to using `reloadData` to avoid stalling the main thread. diff --git a/Source/IGListKit/IGListAdapterUpdater.m b/Source/IGListKit/IGListAdapterUpdater.m index 1e303f2c..fe140d7b 100644 --- a/Source/IGListKit/IGListAdapterUpdater.m +++ b/Source/IGListKit/IGListAdapterUpdater.m @@ -31,7 +31,6 @@ @synthesize sectionMovesAsDeletesInserts = _sectionMovesAsDeletesInserts; @synthesize singleItemSectionUpdates = _singleItemSectionUpdates; @synthesize preferItemReloadsForSectionReloads = _preferItemReloadsForSectionReloads; -@synthesize allowsBackgroundReloading = _allowsBackgroundReloading; @synthesize allowsReloadingOnTooManyUpdates = _allowsReloadingOnTooManyUpdates; @synthesize experiments = _experiments; @@ -40,7 +39,6 @@ if (self = [super init]) { _transactionBuilder = [IGListUpdateTransactionBuilder new]; - _allowsBackgroundReloading = YES; _allowsReloadingOnTooManyUpdates = YES; } return self; @@ -91,7 +89,6 @@ .sectionMovesAsDeletesInserts = _sectionMovesAsDeletesInserts, .singleItemSectionUpdates = _singleItemSectionUpdates, .preferItemReloadsForSectionReloads = _preferItemReloadsForSectionReloads, - .allowsBackgroundReloading = _allowsBackgroundReloading, .allowsReloadingOnTooManyUpdates = _allowsReloadingOnTooManyUpdates, .experiments = _experiments, }; diff --git a/Source/IGListKit/Internal/IGListAdapterUpdater+DebugDescription.m b/Source/IGListKit/Internal/IGListAdapterUpdater+DebugDescription.m index 16a76601..3c8b1f44 100644 --- a/Source/IGListKit/Internal/IGListAdapterUpdater+DebugDescription.m +++ b/Source/IGListKit/Internal/IGListAdapterUpdater+DebugDescription.m @@ -22,7 +22,6 @@ [NSString stringWithFormat:@"sectionMovesAsDeletesInserts: %@", IGListDebugBOOL(self.sectionMovesAsDeletesInserts)], [NSString stringWithFormat:@"singleItemSectionUpdates: %@", IGListDebugBOOL(self.singleItemSectionUpdates)], [NSString stringWithFormat:@"preferItemReloadsForSectionReloads: %@", IGListDebugBOOL(self.preferItemReloadsForSectionReloads)], - [NSString stringWithFormat:@"allowsBackgroundReloading: %@", IGListDebugBOOL(self.allowsBackgroundReloading)], [NSString stringWithFormat:@"allowsReloadingOnTooManyUpdates: %@", IGListDebugBOOL(self.allowsReloadingOnTooManyUpdates)] ]; [debug addObjectsFromArray:IGListDebugIndentedLines(options)]; diff --git a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m index db4fb075..927e10dc 100644 --- a/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m +++ b/Source/IGListKit/Internal/IGListBatchUpdateTransaction.m @@ -95,13 +95,6 @@ typedef NS_ENUM (NSInteger, IGListBatchUpdateTransactionMode) { // disables multiple performBatchUpdates: from happening at the same time self.state = IGListBatchUpdateStateQueuedBatchUpdate; - // if the collection view isn't in a visible window, skip diffing and batch updating. execute all transition blocks, - // reload data, execute completion blocks, and get outta here - if (self.config.allowsBackgroundReloading && self.collectionView.window == nil) { - [self _reload]; - return; - } - [self _diff]; } diff --git a/Source/IGListKit/Internal/IGListUpdateTransactable.h b/Source/IGListKit/Internal/IGListUpdateTransactable.h index bdfd898b..9e3d1b03 100644 --- a/Source/IGListKit/Internal/IGListUpdateTransactable.h +++ b/Source/IGListKit/Internal/IGListUpdateTransactable.h @@ -19,7 +19,6 @@ typedef struct { BOOL sectionMovesAsDeletesInserts; BOOL singleItemSectionUpdates; BOOL preferItemReloadsForSectionReloads; - BOOL allowsBackgroundReloading; BOOL allowsReloadingOnTooManyUpdates; IGListExperiment experiments; } IGListUpdateTransactationConfig; diff --git a/Tests/IGListAdapterUpdaterTests.m b/Tests/IGListAdapterUpdaterTests.m index 1bfe3da1..e64cd1cc 100644 --- a/Tests/IGListAdapterUpdaterTests.m +++ b/Tests/IGListAdapterUpdaterTests.m @@ -533,7 +533,6 @@ } - (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isSetNO_diffHappens { - self.updater.allowsBackgroundReloading = NO; [self.collectionView removeFromSuperview]; id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; @@ -557,57 +556,6 @@ [mockDelegate verify]; } -- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_fallbackToReload { - [self.collectionView removeFromSuperview]; - - id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; - self.updater.delegate = mockDelegate; - - [mockDelegate setExpectationOrderMatters:YES]; - [[mockDelegate expect] listAdapterUpdater:self.updater willReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; - [[mockDelegate expect] listAdapterUpdater:self.updater didReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; - - XCTestExpectation *expectation = genExpectation; - NSArray *to = @[ - [IGSectionObject sectionWithObjects:@[]] - ]; - [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] - animated:NO - sectionDataBlock:[self dataBlockFromObjects:self.dataSource.sections toObjects:to] - applySectionDataBlock:self.applySectionDataBlock - completion:^(BOOL finished) { - [expectation fulfill]; - }]; - waitExpectation; - [mockDelegate verify]; -} - -- (void)test_whenCollectionViewNotInWindow_andBackgroundReloadFlag_isDefaultYES_andDataSourceWasSetToNilBefore_fallbackToReload { - [self.collectionView removeFromSuperview]; - - id mockDelegate = [OCMockObject niceMockForProtocol:@protocol(IGListAdapterUpdaterDelegate)]; - self.updater.delegate = mockDelegate; - - [mockDelegate setExpectationOrderMatters:YES]; - [[mockDelegate expect] listAdapterUpdater:self.updater willReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; - [[mockDelegate expect] listAdapterUpdater:self.updater didReloadDataWithCollectionView:self.collectionView isFallbackReload:YES]; - - XCTestExpectation *expectation = genExpectation; - NSArray *to = @[ - [IGSectionObject sectionWithObjects:@[]] - ]; - self.collectionView.dataSource = nil; - [self.updater performUpdateWithCollectionViewBlock:[self collectionViewBlock] - animated:NO - sectionDataBlock:[self dataBlockFromObjects:self.dataSource.sections toObjects:to] - applySectionDataBlock:self.applySectionDataBlock - completion:^(BOOL finished) { - [expectation fulfill]; - }]; - waitExpectation; - [mockDelegate verify]; -} - - (void)test_whenReloadBatchedWithUpdate_thatCompletionBlockStillExecuted { IGSectionObject *object = [IGSectionObject sectionWithObjects:@[@0, @1, @2]]; self.dataSource.sections = @[object]; diff --git a/Tests/IGListBindingSectionControllerTests.m b/Tests/IGListBindingSectionControllerTests.m index 7ba24829..f9a21096 100644 --- a/Tests/IGListBindingSectionControllerTests.m +++ b/Tests/IGListBindingSectionControllerTests.m @@ -413,7 +413,6 @@ IGListCollectionViewLayout *layout = [[IGListCollectionViewLayout alloc] initWithStickyHeaders:NO topContentInset:0 stretchToEdge:NO]; self.collectionView = [[UICollectionView alloc] initWithFrame:self.frame collectionViewLayout:layout]; - [(IGListAdapterUpdater *)self.adapter.updater setAllowsBackgroundReloading:NO]; self.adapter.experiments |= IGListExperimentInvalidateLayoutForUpdates; [self setupWithObjects:@[