From afec07aca273503b0647dbf1f73c518c6e52e8ba Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Thu, 9 Feb 2023 05:52:42 -0800 Subject: [PATCH] Add option to make commit asynchronous Summary: changelog: [internal] `useLayoutEffect` has two guarantees which React Native breaks: - Layout metrics are ready. - Updates triggered inside `useLayoutEffect` are applied before paint. State between first commit and update is not shown on the screen. React Native breaks the first guarantee because it uses Background Executor. Background executor moves Yoga layout to another thread. If user core reads layout metrics in `useLayoutEffect` hook, it is a race. The information might be there, or it might not. They can even read partially update information. This diff does not affect this. We already have a way to turn off Background Executor. We haven't done this because it introduces regressions on one screen but we have a solution for that. React Native breaks the second guarantee. After Fabric's commit phase, Fabric moves to mounting the changes right away. In this diff, we queue the mounting phase and give React a chance to change what is committed to the screen. To do that, we schedule a task with user blocking priority in `RuntimeScheduler`. React, if there is an update in `useLayoutEffect`, schedules a task in the scheduler with higher priority and stops the mounting phase. We are not delaying mounting, this just gives React a chance to interrupt it. Fabric commit phase may be triggered by different mechanisms. C++ state update, surface tear down, template update (not used atm), setNativeProps, to name a few. Fabric only needs to block paint if commit originates from React. Otherwise the scheduling is wrong and we will get into undefined behaviour land. Rollout: This change is gated behind `react_fabric:block_paint_for_use_layout_effect` and will be rolled out incrementally. Reviewed By: javache Differential Revision: D43083051 fbshipit-source-id: bb494cf56a11763e38dce7ba0093c4dafdd8bf43 --- React/Fabric/Mounting/RCTMountingManager.mm | 12 ++++---- React/Fabric/RCTScheduler.h | 2 +- React/Fabric/RCTSurfacePresenter.mm | 2 +- .../react/renderer/core/CoreFeatures.cpp | 1 + .../react/renderer/core/CoreFeatures.h | 5 ++++ .../react/renderer/mounting/ShadowTree.cpp | 13 ++++---- .../react/renderer/mounting/ShadowTree.h | 9 +++++- .../renderer/mounting/ShadowTreeDelegate.h | 3 +- .../tests/StateReconciliationTest.cpp | 3 +- .../runtimescheduler/RuntimeScheduler.cpp | 4 +-- .../react/renderer/scheduler/Scheduler.cpp | 30 +++++++++++++++++-- .../react/renderer/scheduler/Scheduler.h | 3 +- .../react/renderer/uimanager/UIManager.cpp | 6 ++-- .../react/renderer/uimanager/UIManager.h | 3 +- .../renderer/uimanager/UIManagerBinding.cpp | 12 ++++++-- .../renderer/uimanager/UIManagerDelegate.h | 3 +- 16 files changed, 84 insertions(+), 27 deletions(-) diff --git a/React/Fabric/Mounting/RCTMountingManager.mm b/React/Fabric/Mounting/RCTMountingManager.mm index 74704af2d5706d..ec5b9152c39ff1 100644 --- a/React/Fabric/Mounting/RCTMountingManager.mm +++ b/React/Fabric/Mounting/RCTMountingManager.mm @@ -203,13 +203,13 @@ - (void)scheduleTransaction:(MountingCoordinator::Shared)mountingCoordinator // * No need to do a thread jump; // * No need to do expensive copy of all mutations; // * No need to allocate a block. - [self initiateTransaction:std::move(mountingCoordinator)]; + [self initiateTransaction:*mountingCoordinator]; return; } RCTExecuteOnMainQueue(^{ RCTAssertMainQueue(); - [self initiateTransaction:std::move(mountingCoordinator)]; + [self initiateTransaction:*mountingCoordinator]; }); } @@ -243,7 +243,7 @@ - (void)sendAccessibilityEvent:(ReactTag)reactTag eventType:(NSString *)eventTyp }); } -- (void)initiateTransaction:(MountingCoordinator::Shared)mountingCoordinator +- (void)initiateTransaction:(MountingCoordinator const &)mountingCoordinator { SystraceSection s("-[RCTMountingManager initiateTransaction:]"); RCTAssertMainQueue(); @@ -261,14 +261,14 @@ - (void)initiateTransaction:(MountingCoordinator::Shared)mountingCoordinator } while (_followUpTransactionRequired); } -- (void)performTransaction:(MountingCoordinator::Shared const &)mountingCoordinator +- (void)performTransaction:(MountingCoordinator const &)mountingCoordinator { SystraceSection s("-[RCTMountingManager performTransaction:]"); RCTAssertMainQueue(); - auto surfaceId = mountingCoordinator->getSurfaceId(); + auto surfaceId = mountingCoordinator.getSurfaceId(); - mountingCoordinator->getTelemetryController().pullTransaction( + mountingCoordinator.getTelemetryController().pullTransaction( [&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) { [self.delegate mountingManager:self willMountComponentsWithRootTag:surfaceId]; _observerCoordinator.notifyObserversMountingTransactionWillMount(transaction, surfaceTelemetry); diff --git a/React/Fabric/RCTScheduler.h b/React/Fabric/RCTScheduler.h index bebc5fd90f0050..1424fc6c4bd863 100644 --- a/React/Fabric/RCTScheduler.h +++ b/React/Fabric/RCTScheduler.h @@ -28,7 +28,7 @@ NS_ASSUME_NONNULL_BEGIN */ @protocol RCTSchedulerDelegate -- (void)schedulerDidFinishTransaction:(facebook::react::MountingCoordinator::Shared const &)mountingCoordinator; +- (void)schedulerDidFinishTransaction:(facebook::react::MountingCoordinator::Shared)mountingCoordinator; - (void)schedulerDidDispatchCommand:(facebook::react::ShadowView const &)shadowView commandName:(std::string const &)commandName diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm index 7d2ac388a72703..61bec9c7148da2 100644 --- a/React/Fabric/RCTSurfacePresenter.mm +++ b/React/Fabric/RCTSurfacePresenter.mm @@ -356,7 +356,7 @@ - (void)_applicationWillTerminate #pragma mark - RCTSchedulerDelegate -- (void)schedulerDidFinishTransaction:(MountingCoordinator::Shared const &)mountingCoordinator +- (void)schedulerDidFinishTransaction:(MountingCoordinator::Shared)mountingCoordinator { [_mountingManager scheduleTransaction:mountingCoordinator]; } diff --git a/ReactCommon/react/renderer/core/CoreFeatures.cpp b/ReactCommon/react/renderer/core/CoreFeatures.cpp index 75153ff17e2753..b71e35cb6236fd 100644 --- a/ReactCommon/react/renderer/core/CoreFeatures.cpp +++ b/ReactCommon/react/renderer/core/CoreFeatures.cpp @@ -12,6 +12,7 @@ namespace react { bool CoreFeatures::enablePropIteratorSetter = false; bool CoreFeatures::enableMapBuffer = false; +bool CoreFeatures::blockPaintForUseLayoutEffect = false; } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/core/CoreFeatures.h b/ReactCommon/react/renderer/core/CoreFeatures.h index 43330d47d92b58..1a106390baac53 100644 --- a/ReactCommon/react/renderer/core/CoreFeatures.h +++ b/ReactCommon/react/renderer/core/CoreFeatures.h @@ -25,6 +25,11 @@ class CoreFeatures { // its ShadowNode traits must set the MapBuffer trait; and this // must be set to "true" globally. static bool enableMapBuffer; + + // When enabled, Fabric will block paint to allow for state updates in + // useLayoutEffect hooks to be processed. This changes affects scheduling of + // when a transaction is mounted. + static bool blockPaintForUseLayoutEffect; }; } // namespace react diff --git a/ReactCommon/react/renderer/mounting/ShadowTree.cpp b/ReactCommon/react/renderer/mounting/ShadowTree.cpp index c8624f44465ba7..318834d2494a5a 100644 --- a/ReactCommon/react/renderer/mounting/ShadowTree.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowTree.cpp @@ -281,7 +281,7 @@ void ShadowTree::setCommitMode(CommitMode commitMode) const { // initial revision never contains any commits so mounting it here is // incorrect if (commitMode == CommitMode::Normal && revision.number != INITIAL_REVISION) { - mount(revision); + mount(revision, true); } } @@ -402,7 +402,7 @@ CommitStatus ShadowTree::tryCommit( emitLayoutEvents(affectedLayoutableNodes); if (commitMode == CommitMode::Normal) { - mount(newRevision); + mount(newRevision, commitOptions.mountSynchronously); } return CommitStatus::Succeeded; @@ -413,9 +413,12 @@ ShadowTreeRevision ShadowTree::getCurrentRevision() const { return currentRevision_; } -void ShadowTree::mount(ShadowTreeRevision const &revision) const { +void ShadowTree::mount( + ShadowTreeRevision const &revision, + bool mountSynchronously) const { mountingCoordinator_->push(revision); - delegate_.shadowTreeDidFinishTransaction(mountingCoordinator_); + delegate_.shadowTreeDidFinishTransaction( + mountingCoordinator_, mountSynchronously); } void ShadowTree::commitEmptyTree() const { @@ -458,7 +461,7 @@ void ShadowTree::emitLayoutEvents( } void ShadowTree::notifyDelegatesOfUpdates() const { - delegate_.shadowTreeDidFinishTransaction(mountingCoordinator_); + delegate_.shadowTreeDidFinishTransaction(mountingCoordinator_, true); } } // namespace facebook::react diff --git a/ReactCommon/react/renderer/mounting/ShadowTree.h b/ReactCommon/react/renderer/mounting/ShadowTree.h index 13c1ec07fa6866..7235fa8c61cab9 100644 --- a/ReactCommon/react/renderer/mounting/ShadowTree.h +++ b/ReactCommon/react/renderer/mounting/ShadowTree.h @@ -59,6 +59,13 @@ class ShadowTree final { struct CommitOptions { bool enableStateReconciliation{false}; + // Indicates if mounting will be triggered synchronously and React will + // not get a chance to interrupt painting. + // This should be set to `false` when a commit is comming from React. It + // will then let React run layout effects and apply updates before paint. + // For all other commits, should be true. + bool mountSynchronously{true}; + // Called during `tryCommit` phase. Returning true indicates current commit // should yield to the next commit. std::function shouldYield; @@ -128,7 +135,7 @@ class ShadowTree final { private: constexpr static ShadowTreeRevision::Number INITIAL_REVISION{0}; - void mount(ShadowTreeRevision const &revision) const; + void mount(ShadowTreeRevision const &revision, bool mountSynchronously) const; void emitLayoutEvents( std::vector &affectedLayoutableNodes) const; diff --git a/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h b/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h index e20f42e1572852..e363eda8bd8f8f 100644 --- a/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h +++ b/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h @@ -34,7 +34,8 @@ class ShadowTreeDelegate { * Called right after Shadow Tree commit a new state of the tree. */ virtual void shadowTreeDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) const = 0; + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) const = 0; virtual ~ShadowTreeDelegate() noexcept = default; }; diff --git a/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp b/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp index f5e56c4cd15403..6b53f264dc2ef7 100644 --- a/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp +++ b/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp @@ -32,7 +32,8 @@ class DummyShadowTreeDelegate : public ShadowTreeDelegate { }; void shadowTreeDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) const override{}; + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) const override{}; }; inline ShadowNode const *findDescendantNode( diff --git a/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp b/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp index 22b4e75fa4e5ff..40b33fd7fad40f 100644 --- a/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp +++ b/ReactCommon/react/renderer/runtimescheduler/RuntimeScheduler.cpp @@ -34,7 +34,7 @@ void RuntimeScheduler::scheduleWork(RawCallback callback) const { std::shared_ptr RuntimeScheduler::scheduleTask( SchedulerPriority priority, jsi::Function callback) { - auto expirationTime = now() + timeoutForSchedulerPriority(priority); + auto expirationTime = now_() + timeoutForSchedulerPriority(priority); auto task = std::make_shared(priority, std::move(callback), expirationTime); taskQueue_.push(task); @@ -47,7 +47,7 @@ std::shared_ptr RuntimeScheduler::scheduleTask( std::shared_ptr RuntimeScheduler::scheduleTask( SchedulerPriority priority, RawCallback callback) { - auto expirationTime = now() + timeoutForSchedulerPriority(priority); + auto expirationTime = now_() + timeoutForSchedulerPriority(priority); auto task = std::make_shared(priority, std::move(callback), expirationTime); taskQueue_.push(task); diff --git a/ReactCommon/react/renderer/scheduler/Scheduler.cpp b/ReactCommon/react/renderer/scheduler/Scheduler.cpp index 5e316102f5aab5..25f31ab4c7dd8f 100644 --- a/ReactCommon/react/renderer/scheduler/Scheduler.cpp +++ b/ReactCommon/react/renderer/scheduler/Scheduler.cpp @@ -128,6 +128,9 @@ Scheduler::Scheduler( reduceDeleteCreateMutationLayoutAnimation_ = true; #endif + CoreFeatures::blockPaintForUseLayoutEffect = reactNativeConfig_->getBool( + "react_fabric:block_paint_for_use_layout_effect"); + if (animationDelegate != nullptr) { animationDelegate->setComponentDescriptorRegistry( componentDescriptorRegistry_); @@ -299,11 +302,34 @@ void Scheduler::animationTick() const { #pragma mark - UIManagerDelegate void Scheduler::uiManagerDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) { + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) { SystraceSection s("Scheduler::uiManagerDidFinishTransaction"); if (delegate_ != nullptr) { - delegate_->schedulerDidFinishTransaction(std::move(mountingCoordinator)); + if (CoreFeatures::blockPaintForUseLayoutEffect) { + auto weakRuntimeScheduler = + contextContainer_->find>( + "RuntimeScheduler"); + auto runtimeScheduler = weakRuntimeScheduler.has_value() + ? weakRuntimeScheduler.value().lock() + : nullptr; + if (runtimeScheduler && !mountSynchronously) { + runtimeScheduler->scheduleTask( + SchedulerPriority::UserBlockingPriority, + [delegate = delegate_, + mountingCoordinator = + std::move(mountingCoordinator)](jsi::Runtime &) { + delegate->schedulerDidFinishTransaction( + std::move(mountingCoordinator)); + }); + } else { + delegate_->schedulerDidFinishTransaction( + std::move(mountingCoordinator)); + } + } else { + delegate_->schedulerDidFinishTransaction(std::move(mountingCoordinator)); + } } } void Scheduler::uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) { diff --git a/ReactCommon/react/renderer/scheduler/Scheduler.h b/ReactCommon/react/renderer/scheduler/Scheduler.h index aad360ece38f93..8467a05e5d341f 100644 --- a/ReactCommon/react/renderer/scheduler/Scheduler.h +++ b/ReactCommon/react/renderer/scheduler/Scheduler.h @@ -88,7 +88,8 @@ class Scheduler final : public UIManagerDelegate { #pragma mark - UIManagerDelegate void uiManagerDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) override; + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) override; void uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) override; void uiManagerDidDispatchCommand( const ShadowNode::Shared &shadowNode, diff --git a/ReactCommon/react/renderer/uimanager/UIManager.cpp b/ReactCommon/react/renderer/uimanager/UIManager.cpp index 252cd543d8cb71..795c9eea18f40e 100644 --- a/ReactCommon/react/renderer/uimanager/UIManager.cpp +++ b/ReactCommon/react/renderer/uimanager/UIManager.cpp @@ -532,11 +532,13 @@ RootShadowNode::Unshared UIManager::shadowTreeWillCommit( } void UIManager::shadowTreeDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) const { + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) const { SystraceSection s("UIManager::shadowTreeDidFinishTransaction"); if (delegate_ != nullptr) { - delegate_->uiManagerDidFinishTransaction(std::move(mountingCoordinator)); + delegate_->uiManagerDidFinishTransaction( + std::move(mountingCoordinator), mountSynchronously); } } diff --git a/ReactCommon/react/renderer/uimanager/UIManager.h b/ReactCommon/react/renderer/uimanager/UIManager.h index 9648cb9fb26fa6..0d4700d9313236 100644 --- a/ReactCommon/react/renderer/uimanager/UIManager.h +++ b/ReactCommon/react/renderer/uimanager/UIManager.h @@ -103,7 +103,8 @@ class UIManager final : public ShadowTreeDelegate { #pragma mark - ShadowTreeDelegate void shadowTreeDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) const override; + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) const override; RootShadowNode::Unshared shadowTreeWillCommit( ShadowTree const &shadowTree, diff --git a/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp b/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp index 9165073adf37ad..bb0cc845a24d23 100644 --- a/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp +++ b/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp @@ -403,7 +403,11 @@ jsi::Value UIManagerBinding::get( auto shadowNodeList = shadowNodeListFromWeakList(weakShadowNodeList); if (shadowNodeList) { - uiManager->completeSurface(surfaceId, shadowNodeList, {true}); + uiManager->completeSurface( + surfaceId, + shadowNodeList, + {/* .enableStateReconciliation = */ true, + /* .mountSynchronously = */ false}); } } else { auto weakShadowNodeList = @@ -429,7 +433,11 @@ jsi::Value UIManagerBinding::get( auto strongUIManager = weakUIManager.lock(); if (shadowNodeList && strongUIManager) { strongUIManager->completeSurface( - surfaceId, shadowNodeList, {true, shouldYield}); + surfaceId, + shadowNodeList, + {/* .enableStateReconciliation = */ true, + /* .mountSynchronously = */ false, + /* .shouldYield = */ shouldYield}); } }); } diff --git a/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h b/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h index 7891f55c011345..8ad86f3c3f7819 100644 --- a/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h +++ b/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h @@ -23,7 +23,8 @@ class UIManagerDelegate { * For this moment the tree is already laid out and sealed. */ virtual void uiManagerDidFinishTransaction( - MountingCoordinator::Shared mountingCoordinator) = 0; + MountingCoordinator::Shared mountingCoordinator, + bool mountSynchronously) = 0; /* * Called each time when UIManager constructs a new Shadow Node. Receiver