From 6e46d9bcd5aec46f4e5cbd0a3a4f7bc9a3be2cd2 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Sun, 30 Jun 2019 21:08:06 -0700 Subject: [PATCH] [ASDataController] Add an experiment that avoids flushing editing queue before starting the data pipeline pipeline (#1564) * [ASDataController] Avoid flushing editing queue before starting data controller pipeline Step 1 of the pipeline is on main thread and needs pendingMap which was updated by the end of step 1 of the run. And step 1 operates on data source index space. So it should be ok to start it ASAP -- without waiting for any running work on the background editing queue. One potential race condition after this change is that it's possible for main thread to query the data source (step 1) while the editing queue consumes the last change set (step 3). However, as long as the client's each nodeBlock and the node's layout code capture/reference an individual model object (as opposed to the whole data set) -- which is what clients are supposed to do anyways, then everything should be fine. The benefit of this diff is that the pipeline will be able to accept many more change sets within a short time window, for example when clients submit a burst of separate small updates. I tested this diff against our test suite several times and smoke tested it in our code base without any issues. * Wrap in an experiment * Enable experiment in tests * Minor change --- Source/ASExperimentalFeatures.h | 3 ++- Source/ASExperimentalFeatures.mm | 3 ++- Source/Details/ASDataController.mm | 14 ++++++++------ Tests/ASCollectionModernDataSourceTests.mm | 5 +++++ Tests/ASCollectionViewTests.mm | 12 +++++++++++- Tests/ASCollectionViewThrashTests.mm | 12 +++++++++++- Tests/ASConfigurationTests.mm | 9 ++++++--- Tests/ASTableViewTests.mm | 11 ++++++++++- Tests/ASTableViewThrashTests.mm | 12 +++++++++++- 9 files changed, 66 insertions(+), 15 deletions(-) diff --git a/Source/ASExperimentalFeatures.h b/Source/ASExperimentalFeatures.h index 51849dfc..11c0cca0 100644 --- a/Source/ASExperimentalFeatures.h +++ b/Source/ASExperimentalFeatures.h @@ -27,9 +27,10 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) { ASExperimentalSkipClearData = 1 << 6, // exp_skip_clear_data ASExperimentalDidEnterPreloadSkipASMLayout = 1 << 7, // exp_did_enter_preload_skip_asm_layout ASExperimentalDispatchApply = 1 << 8, // exp_dispatch_apply - ASExperimentalOOMBackgroundDeallocDisable = 1 << 9, // exp_oom_bg_dealloc_disable + ASExperimentalOOMBackgroundDeallocDisable = 1 << 9, // exp_oom_bg_dealloc_disable ASExperimentalRemoveTextKitInitialisingLock = 1 << 10, // exp_remove_textkit_initialising_lock ASExperimentalDrawingGlobal = 1 << 11, // exp_drawing_global + ASExperimentalOptimizeDataControllerPipeline = 1 << 12, // exp_optimize_data_controller_pipeline ASExperimentalFeatureAll = 0xFFFFFFFF }; diff --git a/Source/ASExperimentalFeatures.mm b/Source/ASExperimentalFeatures.mm index 9be1a316..85690543 100644 --- a/Source/ASExperimentalFeatures.mm +++ b/Source/ASExperimentalFeatures.mm @@ -23,7 +23,8 @@ NSArray *ASExperimentalFeaturesGetNames(ASExperimentalFeatures flags @"exp_dispatch_apply", @"exp_oom_bg_dealloc_disable", @"exp_remove_textkit_initialising_lock", - @"exp_drawing_global"])); + @"exp_drawing_global", + @"exp_optimize_data_controller_pipeline"])); if (flags == ASExperimentalFeatureAll) { return allNames; } diff --git a/Source/Details/ASDataController.mm b/Source/Details/ASDataController.mm index 0f8008ce..d4fc5598 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -560,13 +560,15 @@ typedef void (^ASDataControllerSynchronizationBlock)(); } else { os_log_debug(ASCollectionLog(), "performBatchUpdates %@ %@", ASViewToDisplayNode(ASDynamicCast(self.dataSource, UIView)), changeSet); } - - NSTimeInterval transactionQueueFlushDuration = 0.0f; - { - AS::ScopeTimer t(transactionQueueFlushDuration); - dispatch_group_wait(_editingTransactionGroup, DISPATCH_TIME_FOREVER); + + if (!ASActivateExperimentalFeature(ASExperimentalOptimizeDataControllerPipeline)) { + NSTimeInterval transactionQueueFlushDuration = 0.0f; + { + AS::ScopeTimer t(transactionQueueFlushDuration); + dispatch_group_wait(_editingTransactionGroup, DISPATCH_TIME_FOREVER); + } } - + // If the initial reloadData has not been called, just bail because we don't have our old data source counts. // See ASUICollectionViewTests.testThatIssuingAnUpdateBeforeInitialReloadIsUnacceptable // for the issue that UICollectionView has that we're choosing to workaround. diff --git a/Tests/ASCollectionModernDataSourceTests.mm b/Tests/ASCollectionModernDataSourceTests.mm index 5d0a7789..e987914f 100644 --- a/Tests/ASCollectionModernDataSourceTests.mm +++ b/Tests/ASCollectionModernDataSourceTests.mm @@ -34,6 +34,11 @@ - (void)setUp { [super setUp]; + + ASConfiguration *config = [ASConfiguration new]; + config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline; + [ASConfigurationManager test_resetWithConfiguration:config]; + // Default is 2 sections: 2 items in first, 1 item in second. sections = [NSMutableArray array]; [sections addObject:[ASTestSection new]]; diff --git a/Tests/ASCollectionViewTests.mm b/Tests/ASCollectionViewTests.mm index 3ff5af44..7359aadd 100644 --- a/Tests/ASCollectionViewTests.mm +++ b/Tests/ASCollectionViewTests.mm @@ -16,7 +16,9 @@ #import #import #import + #import "ASDisplayNodeTestsHelper.h" +#import "ASTestCase.h" @interface ASTextCellNodeWithSetSelectedCounter : ASTextCellNode @@ -166,12 +168,20 @@ @end -@interface ASCollectionViewTests : XCTestCase +@interface ASCollectionViewTests : ASTestCase @end @implementation ASCollectionViewTests +- (void)setUp +{ + [super setUp]; + ASConfiguration *config = [ASConfiguration new]; + config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline; + [ASConfigurationManager test_resetWithConfiguration:config]; +} + - (void)tearDown { // We can't prevent the system from retaining windows, but we can at least clear them out to avoid diff --git a/Tests/ASCollectionViewThrashTests.mm b/Tests/ASCollectionViewThrashTests.mm index 4650639d..368b33d6 100644 --- a/Tests/ASCollectionViewThrashTests.mm +++ b/Tests/ASCollectionViewThrashTests.mm @@ -12,9 +12,10 @@ #import #import +#import "ASTestCase.h" #import "ASThrashUtility.h" -@interface ASCollectionViewThrashTests : XCTestCase +@interface ASCollectionViewThrashTests : ASTestCase @end @@ -25,8 +26,17 @@ BOOL _failed; } +- (void)setUp +{ + [super setUp]; + ASConfiguration *config = [ASConfiguration new]; + config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline; + [ASConfigurationManager test_resetWithConfiguration:config]; +} + - (void)tearDown { + [super tearDown]; if (_failed && _update != nil) { NSLog(@"Failed update %@: %@", _update, _update.logFriendlyBase64Representation); } diff --git a/Tests/ASConfigurationTests.mm b/Tests/ASConfigurationTests.mm index 97b75ccb..c5e533e2 100644 --- a/Tests/ASConfigurationTests.mm +++ b/Tests/ASConfigurationTests.mm @@ -29,7 +29,8 @@ static ASExperimentalFeatures features[] = { ASExperimentalDispatchApply, ASExperimentalOOMBackgroundDeallocDisable, ASExperimentalRemoveTextKitInitialisingLock, - ASExperimentalDrawingGlobal + ASExperimentalDrawingGlobal, + ASExperimentalOptimizeDataControllerPipeline }; @interface ASConfigurationTests : ASTestCase @@ -53,11 +54,13 @@ static ASExperimentalFeatures features[] = { @"exp_dispatch_apply", @"exp_oom_bg_dealloc_disable", @"exp_remove_textkit_initialising_lock", - @"exp_drawing_global" + @"exp_drawing_global", + @"exp_optimize_data_controller_pipeline" ]; } -- (ASExperimentalFeatures)allFeatures { +- (ASExperimentalFeatures)allFeatures +{ ASExperimentalFeatures allFeatures = 0; for (int i = 0; i < sizeof(features)/sizeof(ASExperimentalFeatures); i++) { allFeatures |= features[i]; diff --git a/Tests/ASTableViewTests.mm b/Tests/ASTableViewTests.mm index e367b0b7..f5e98c27 100644 --- a/Tests/ASTableViewTests.mm +++ b/Tests/ASTableViewTests.mm @@ -22,6 +22,7 @@ #import #import +#import "ASTestCase.h" #import "ASXCTExtensions.h" #define NumberOfSections 10 @@ -217,12 +218,20 @@ @end -@interface ASTableViewTests : XCTestCase +@interface ASTableViewTests : ASTestCase @property (nonatomic, retain) ASTableView *testTableView; @end @implementation ASTableViewTests +- (void)setUp +{ + [super setUp]; + ASConfiguration *config = [ASConfiguration new]; + config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline; + [ASConfigurationManager test_resetWithConfiguration:config]; +} + - (void)testDataSourceImplementsNecessaryMethods { ASTestTableView *tableView = [[ASTestTableView alloc] __initWithFrame:CGRectMake(0, 0, 100, 400) diff --git a/Tests/ASTableViewThrashTests.mm b/Tests/ASTableViewThrashTests.mm index 8d7064cd..695bf67f 100644 --- a/Tests/ASTableViewThrashTests.mm +++ b/Tests/ASTableViewThrashTests.mm @@ -13,9 +13,10 @@ #import #import +#import "ASTestCase.h" #import "ASThrashUtility.h" -@interface ASTableViewThrashTests: XCTestCase +@interface ASTableViewThrashTests: ASTestCase @end @implementation ASTableViewThrashTests @@ -27,8 +28,17 @@ #pragma mark Overrides +- (void)setUp +{ + [super setUp]; + ASConfiguration *config = [ASConfiguration new]; + config.experimentalFeatures = ASExperimentalOptimizeDataControllerPipeline; + [ASConfigurationManager test_resetWithConfiguration:config]; +} + - (void)tearDown { + [super tearDown]; if (_failed && _update != nil) { NSLog(@"Failed update %@: %@", _update, _update.logFriendlyBase64Representation); }