From 95892dc95b3eb7afc033ae1eee36d9a4453bc422 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 26 Jun 2019 11:31:05 -0700 Subject: [PATCH 1/4] [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. --- Source/Details/ASDataController.mm | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Source/Details/ASDataController.mm b/Source/Details/ASDataController.mm index 0f8008ce0..fa6e8ab96 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -561,12 +561,6 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet 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 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. From f1296a821f574688eb47810af334c3edcc58abf0 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Fri, 28 Jun 2019 11:51:49 -0700 Subject: [PATCH 2/4] Wrap in an experiment --- Source/ASExperimentalFeatures.h | 3 ++- Source/ASExperimentalFeatures.mm | 3 ++- Source/Details/ASDataController.mm | 10 +++++++++- Tests/ASConfigurationTests.mm | 6 ++++-- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/Source/ASExperimentalFeatures.h b/Source/ASExperimentalFeatures.h index 51849dfca..11c0cca02 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 9be1a3169..856905434 100644 --- a/Source/ASExperimentalFeatures.mm +++ b/Source/ASExperimentalFeatures.mm @@ -23,7 +23,8 @@ @"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 fa6e8ab96..d4fc5598c 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -560,7 +560,15 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet } else { os_log_debug(ASCollectionLog(), "performBatchUpdates %@ %@", ASViewToDisplayNode(ASDynamicCast(self.dataSource, UIView)), changeSet); } - + + 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/ASConfigurationTests.mm b/Tests/ASConfigurationTests.mm index 97b75ccb5..c50fa3181 100644 --- a/Tests/ASConfigurationTests.mm +++ b/Tests/ASConfigurationTests.mm @@ -29,7 +29,8 @@ ASExperimentalDispatchApply, ASExperimentalOOMBackgroundDeallocDisable, ASExperimentalRemoveTextKitInitialisingLock, - ASExperimentalDrawingGlobal + ASExperimentalDrawingGlobal, + ASExperimentalOptimizeDataControllerPipeline }; @interface ASConfigurationTests : ASTestCase @@ -53,7 +54,8 @@ + (NSArray *)names { @"exp_dispatch_apply", @"exp_oom_bg_dealloc_disable", @"exp_remove_textkit_initialising_lock", - @"exp_drawing_global" + @"exp_drawing_global", + @"exp_optimize_data_controller_pipeline" ]; } From 8a5e16bfcebe24081147c19d6c4f56c610e12c8b Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Fri, 28 Jun 2019 12:05:56 -0700 Subject: [PATCH 3/4] Enable experiment in tests --- Tests/ASCollectionModernDataSourceTests.mm | 5 +++++ Tests/ASCollectionViewTests.mm | 12 +++++++++++- Tests/ASCollectionViewThrashTests.mm | 13 ++++++++++++- Tests/ASConfigurationTests.mm | 3 ++- Tests/ASTableViewTests.mm | 11 ++++++++++- Tests/ASTableViewThrashTests.mm | 12 +++++++++++- 6 files changed, 51 insertions(+), 5 deletions(-) diff --git a/Tests/ASCollectionModernDataSourceTests.mm b/Tests/ASCollectionModernDataSourceTests.mm index 5d0a77899..e987914f4 100644 --- a/Tests/ASCollectionModernDataSourceTests.mm +++ b/Tests/ASCollectionModernDataSourceTests.mm @@ -34,6 +34,11 @@ @implementation ASCollectionModernDataSourceTests { - (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 3ff5af440..7359aaddc 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 @@ @interface ASCollectionView (InternalTesting) @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 4650639d1..691406f1b 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,18 @@ @implementation ASCollectionViewThrashTests 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 c50fa3181..c5e533e22 100644 --- a/Tests/ASConfigurationTests.mm +++ b/Tests/ASConfigurationTests.mm @@ -59,7 +59,8 @@ + (NSArray *)names { ]; } -- (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 e367b0b76..f5e98c270 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 @@ - (ASSizeRange)tableView:(ASTableView *)tableView constrainedSizeForRowAtIndexPa @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 8d7064cd0..695bf67ff 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 @@ @implementation ASTableViewThrashTests #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); } From 5619fd8f99cdae9285f42d9701356834e610ead3 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Fri, 28 Jun 2019 13:22:44 -0700 Subject: [PATCH 4/4] Minor change --- Tests/ASCollectionViewThrashTests.mm | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/ASCollectionViewThrashTests.mm b/Tests/ASCollectionViewThrashTests.mm index 691406f1b..368b33d6c 100644 --- a/Tests/ASCollectionViewThrashTests.mm +++ b/Tests/ASCollectionViewThrashTests.mm @@ -37,7 +37,6 @@ - (void)setUp - (void)tearDown { [super tearDown]; - if (_failed && _update != nil) { NSLog(@"Failed update %@: %@", _update, _update.logFriendlyBase64Representation); }