From 091edd2ef7440706bd2811a5262bab4bfbf99d56 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Wed, 16 Jan 2019 20:17:01 -0800 Subject: [PATCH] Fabric: Clear separation between `commmit` and `tryCommit` Summary: Previously we had a single `commit` function that accepts an argument that enables auto-retry mechanism. As Spencer pointed out, that wasn't so useful and clear. So, I split the method into two with more expressive names. Reviewed By: sahrens Differential Revision: D13681594 fbshipit-source-id: 529f729d597206e9a0ac940f005a1569a9bef707 --- ReactCommon/fabric/uimanager/Scheduler.cpp | 25 ++-- ReactCommon/fabric/uimanager/ShadowTree.cpp | 120 +++++++++++--------- ReactCommon/fabric/uimanager/ShadowTree.h | 16 ++- ReactCommon/fabric/uimanager/UIManager.cpp | 16 +-- 4 files changed, 95 insertions(+), 82 deletions(-) diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index f30e23eea..307a6c3cd 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -104,7 +104,7 @@ void Scheduler::renderTemplateToSurface( reactNativeConfig_); shadowTreeRegistry_.visit(surfaceId, [=](const ShadowTree &shadowTree) { - return shadowTree.commit( + return shadowTree.tryCommit( [&](const SharedRootShadowNode &oldRootShadowNode) { return std::make_shared( *oldRootShadowNode, @@ -124,7 +124,7 @@ void Scheduler::stopSurface(SurfaceId surfaceId) const { shadowTreeRegistry_.visit(surfaceId, [](const ShadowTree &shadowTree) { // As part of stopping the Surface, we have to commit an empty tree. - return shadowTree.commit( + return shadowTree.tryCommit( [&](const SharedRootShadowNode &oldRootShadowNode) { return std::make_shared( *oldRootShadowNode, @@ -151,7 +151,7 @@ Size Scheduler::measureSurface( Size size; shadowTreeRegistry_.visit(surfaceId, [&](const ShadowTree &shadowTree) { - shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { + shadowTree.tryCommit([&](const SharedRootShadowNode &oldRootShadowNode) { auto rootShadowNode = oldRootShadowNode->clone(layoutConstraints, layoutContext); rootShadowNode->layout(); @@ -169,11 +169,9 @@ void Scheduler::constraintSurfaceLayout( SystraceSection s("Scheduler::constraintSurfaceLayout"); shadowTreeRegistry_.visit(surfaceId, [&](const ShadowTree &shadowTree) { - shadowTree.commit( - [&](const SharedRootShadowNode &oldRootShadowNode) { - return oldRootShadowNode->clone(layoutConstraints, layoutContext); - }, - std::numeric_limits::max()); + shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { + return oldRootShadowNode->clone(layoutConstraints, layoutContext); + }); }); } @@ -208,13 +206,10 @@ void Scheduler::uiManagerDidFinishTransaction( SystraceSection s("Scheduler::uiManagerDidFinishTransaction"); shadowTreeRegistry_.visit(surfaceId, [&](const ShadowTree &shadowTree) { - shadowTree.commit( - [&](const SharedRootShadowNode &oldRootShadowNode) { - return std::make_shared( - *oldRootShadowNode, - ShadowNodeFragment{.children = rootChildNodes}); - }, - std::numeric_limits::max()); + shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { + return std::make_shared( + *oldRootShadowNode, ShadowNodeFragment{.children = rootChildNodes}); + }); }); } diff --git a/ReactCommon/fabric/uimanager/ShadowTree.cpp b/ReactCommon/fabric/uimanager/ShadowTree.cpp index ccb125ef4..f496e0fb3 100644 --- a/ReactCommon/fabric/uimanager/ShadowTree.cpp +++ b/ReactCommon/fabric/uimanager/ShadowTree.cpp @@ -109,71 +109,81 @@ Tag ShadowTree::getSurfaceId() const { return surfaceId_; } -bool ShadowTree::commit( - std::function transaction, - int attempts, - int *revision) const { +void ShadowTree::commit(ShadowTreeCommitTransaction transaction, int *revision) + const { SystraceSection s("ShadowTree::commit"); - while (attempts) { - attempts--; + int attempts = 0; - SharedRootShadowNode oldRootShadowNode; - - { - // Reading `rootShadowNode_` in shared manner. - std::shared_lock lock(commitMutex_); - oldRootShadowNode = rootShadowNode_; + while (true) { + attempts++; + if (tryCommit(transaction, revision)) { + return; } - UnsharedRootShadowNode newRootShadowNode = transaction(oldRootShadowNode); + // After multiple attempts, we failed to commit the transaction. + // Something internally went terribly wrong. + assert(attempts < 1024); + } +} - if (!newRootShadowNode) { - break; - } +bool ShadowTree::tryCommit( + ShadowTreeCommitTransaction transaction, + int *revision) const { + SystraceSection s("ShadowTree::tryCommit"); - newRootShadowNode->layout(); - newRootShadowNode->sealRecursive(); + SharedRootShadowNode oldRootShadowNode; - auto mutations = - calculateShadowViewMutations(*oldRootShadowNode, *newRootShadowNode); - - { - // Updating `rootShadowNode_` in unique manner if it hasn't changed. - std::unique_lock lock(commitMutex_); - - if (rootShadowNode_ != oldRootShadowNode) { - continue; - } - - rootShadowNode_ = newRootShadowNode; - - { - std::lock_guard dispatchLock(EventEmitter::DispatchMutex()); - - updateMountedFlag( - oldRootShadowNode->getChildren(), newRootShadowNode->getChildren()); - } - - revision_++; - - // Returning last revision if requested. - if (revision) { - *revision = revision_; - } - } - - emitLayoutEvents(mutations); - - if (delegate_) { - delegate_->shadowTreeDidCommit(*this, mutations); - } - - return true; + { + // Reading `rootShadowNode_` in shared manner. + std::shared_lock lock(commitMutex_); + oldRootShadowNode = rootShadowNode_; } - return false; + UnsharedRootShadowNode newRootShadowNode = transaction(oldRootShadowNode); + + if (!newRootShadowNode) { + return false; + } + + newRootShadowNode->layout(); + newRootShadowNode->sealRecursive(); + + auto mutations = + calculateShadowViewMutations(*oldRootShadowNode, *newRootShadowNode); + + { + // Updating `rootShadowNode_` in unique manner if it hasn't changed. + std::unique_lock lock(commitMutex_); + + if (rootShadowNode_ != oldRootShadowNode) { + return false; + } + + rootShadowNode_ = newRootShadowNode; + + { + std::lock_guard dispatchLock(EventEmitter::DispatchMutex()); + + updateMountedFlag( + oldRootShadowNode->getChildren(), newRootShadowNode->getChildren()); + } + + revision_++; + + // Returning last revision if requested. + if (revision) { + *revision = revision_; + } + } + + emitLayoutEvents(mutations); + + if (delegate_) { + delegate_->shadowTreeDidCommit(*this, mutations); + } + + return true; } void ShadowTree::emitLayoutEvents( diff --git a/ReactCommon/fabric/uimanager/ShadowTree.h b/ReactCommon/fabric/uimanager/ShadowTree.h index c0b8302f9..38c39314c 100644 --- a/ReactCommon/fabric/uimanager/ShadowTree.h +++ b/ReactCommon/fabric/uimanager/ShadowTree.h @@ -19,6 +19,9 @@ namespace facebook { namespace react { +using ShadowTreeCommitTransaction = std::function; + /* * Represents the shadow tree and its lifecycle. */ @@ -45,15 +48,18 @@ class ShadowTree final { * The `transaction` function can abort commit returning `nullptr`. * If a `revision` pointer is not null, the method will store there a * contiguous revision number of the successfully performed transaction. - * Specify `attempts` to allow performing multiple tries. * Returns `true` if the operation finished successfully. */ - bool commit( - std::function transaction, - int attempts = 1, + bool tryCommit( + ShadowTreeCommitTransaction transaction, int *revision = nullptr) const; + /* + * Calls `tryCommit` in a loop until it finishes successfully. + */ + void commit(ShadowTreeCommitTransaction transaction, int *revision = nullptr) + const; + #pragma mark - Delegate /* diff --git a/ReactCommon/fabric/uimanager/UIManager.cpp b/ReactCommon/fabric/uimanager/UIManager.cpp index 33ba75544..8b5698007 100644 --- a/ReactCommon/fabric/uimanager/UIManager.cpp +++ b/ReactCommon/fabric/uimanager/UIManager.cpp @@ -86,9 +86,10 @@ void UIManager::setNativeProps( shadowTreeRegistry_->visit( shadowNode->getRootTag(), [&](const ShadowTree &shadowTree) { - shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { - return oldRootShadowNode->clone(shadowNode, newShadowNode); - }); + shadowTree.tryCommit( + [&](const SharedRootShadowNode &oldRootShadowNode) { + return oldRootShadowNode->clone(shadowNode, newShadowNode); + }); }); } @@ -100,10 +101,11 @@ LayoutMetrics UIManager::getRelativeLayoutMetrics( if (!ancestorShadowNode) { shadowTreeRegistry_->visit( shadowNode.getRootTag(), [&](const ShadowTree &shadowTree) { - shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { - ancestorShadowNode = oldRootShadowNode.get(); - return nullptr; - }); + shadowTree.tryCommit( + [&](const SharedRootShadowNode &oldRootShadowNode) { + ancestorShadowNode = oldRootShadowNode.get(); + return nullptr; + }); }); }