From 2c519b0b10511205a143b4e25766eee33d2d4224 Mon Sep 17 00:00:00 2001 From: Zhisheng Huang Date: Mon, 2 Nov 2020 11:17:02 -0800 Subject: [PATCH] Improve the visibleObjects performance Summary: Trying to improve my previous QE regression on all the perf: https://www.internalfb.com/intern/qe2/ig_ios_listkit_improvements_universe/ig_ios_listkit_improvements_skip_view_section_controller_map/review My theory is that it's incresing the load in the -visibleObjects call, then we call -[UICollectionView cellForIndexPath:] which is expensive in the `_sectionControllerForCell:` Thus my new approach is to optimize this -visibleObjects entirely and instead of dealing with visible cells, we can use the `indexPathsForVisibleItems` which can give us the indexpath instead and make it faster. It would be O(N) instead of O(N^2) compared to before. Reviewed By: calimarkus Differential Revision: D24663555 fbshipit-source-id: 69fcf2d0b44f8bc06351c92f96c3cbe227c22479 --- Source/IGListKit/IGListAdapter.m | 48 ++++++++++++++++++++++++-------- Tests/IGListAdapterTests.m | 15 +++++++++- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/Source/IGListKit/IGListAdapter.m b/Source/IGListKit/IGListAdapter.m index 62dc48db..f3ba9b8c 100644 --- a/Source/IGListKit/IGListAdapter.m +++ b/Source/IGListKit/IGListAdapter.m @@ -606,21 +606,40 @@ - (NSArray *)visibleObjects { IGAssertMainThread(); - NSArray *visibleCells = [self.collectionView visibleCells]; - NSMutableSet *visibleObjects = [NSMutableSet new]; - for (UICollectionViewCell *cell in visibleCells) { - IGListSectionController *sectionController = [self _sectionControllerForCell:cell]; - IGAssert(sectionController != nil, @"Section controller nil for cell %@", cell); - if (sectionController != nil) { - const NSInteger section = [self sectionForSectionController:sectionController]; + + if (IGListExperimentEnabled(_experiments, IGListExperimentSkipViewSectionControllerMap)) { + NSArray *visibleIndexPaths = [self.collectionView indexPathsForVisibleItems]; + NSMutableIndexSet *visibleSections = [NSMutableIndexSet new]; + [visibleIndexPaths enumerateObjectsUsingBlock:^(NSIndexPath * _Nonnull indexPath, NSUInteger idx, BOOL * _Nonnull stop) { + [visibleSections addIndex:indexPath.section]; + }]; + + NSMutableArray *visibleObjects = [NSMutableArray new]; + [visibleSections enumerateIndexesUsingBlock:^(NSUInteger section, BOOL * _Nonnull stop) { id object = [self objectAtSection:section]; - IGAssert(object != nil, @"Object not found for section controller %@ at section %li", sectionController, (long)section); + IGAssert(object != nil, @"Object not found at section %li", (long)section); if (object != nil) { [visibleObjects addObject:object]; } + }]; + return visibleObjects; + } else { + NSArray *visibleCells = [self.collectionView visibleCells]; + NSMutableSet *visibleObjects = [NSMutableSet new]; + for (UICollectionViewCell *cell in visibleCells) { + IGListSectionController *sectionController = [self _sectionControllerForCell:cell]; + IGAssert(sectionController != nil, @"Section controller nil for cell %@", cell); + if (sectionController != nil) { + const NSInteger section = [self sectionForSectionController:sectionController]; + id object = [self objectAtSection:section]; + IGAssert(object != nil, @"Object not found for section controller %@ at section %li", sectionController, (long)section); + if (object != nil) { + [visibleObjects addObject:object]; + } + } } + return [visibleObjects allObjects]; } - return [visibleObjects allObjects]; } - (NSArray *)visibleCellsForObject:(id)object { @@ -1055,8 +1074,15 @@ // only return a cell if it belongs to the section controller // this association is created in -collectionView:cellForItemAtIndexPath: UICollectionViewCell *cell = [self.collectionView cellForItemAtIndexPath:indexPath]; - if ([self _sectionControllerForCell:cell] == sectionController) { - return cell; + + if (IGListExperimentEnabled(_experiments, IGListExperimentSkipViewSectionControllerMap)) { + if ([self sectionControllerForSection:indexPath.section] == sectionController) { + return cell; + } + } else { + if ([self _sectionControllerForCell:cell] == sectionController) { + return cell; + } } } return nil; diff --git a/Tests/IGListAdapterTests.m b/Tests/IGListAdapterTests.m index c9b008a8..d852d795 100644 --- a/Tests/IGListAdapterTests.m +++ b/Tests/IGListAdapterTests.m @@ -614,6 +614,19 @@ XCTAssertEqualObjects(visibleObjects, expectedObjects); } +- (void)test_whenAdapterUpdated_withSkipViewSectionControllerMap_withObjectsOverflow_thatVisibleObjectsIsSubsetOfAllObjects { + self.adapter.experiments |= IGListExperimentSkipViewSectionControllerMap; + // each section controller returns n items sized 100x10 + self.dataSource.objects = @[@1, @2, @3, @4, @5, @6]; + [self.adapter reloadDataWithCompletion:nil]; + self.collectionView.contentOffset = CGPointMake(0, 30); + [self.collectionView layoutIfNeeded]; + + NSArray *visibleObjects = [[self.adapter visibleObjects] sortedArrayUsingSelector:@selector(compare:)]; + NSArray *expectedObjects = @[@3, @4, @5]; + XCTAssertEqualObjects(visibleObjects, expectedObjects); +} + - (void)test_whenAdapterUpdated_thatVisibleCellsForObjectAreFound { // each section controller returns n items sized 100x10 self.dataSource.objects = @[@2, @10, @5]; @@ -933,7 +946,7 @@ - (void)test_whenQueryingSectionControllerForSection_thatControllerReturned { self.dataSource.objects = @[@0, @1, @2]; [self.adapter reloadDataWithCompletion:nil]; - + XCTAssertEqual([self.adapter sectionControllerForSection:0], [self.adapter sectionControllerForObject:@0]); XCTAssertEqual([self.adapter sectionControllerForSection:1], [self.adapter sectionControllerForObject:@1]); XCTAssertEqual([self.adapter sectionControllerForSection:2], [self.adapter sectionControllerForObject:@2]);