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

@@ -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<RootShadowNode>(
                 *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<RootShadowNode>(
               *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<int>::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<RootShadowNode>(
-              *oldRootShadowNode,
-              ShadowNodeFragment{.children = rootChildNodes});
-        },
-        std::numeric_limits<int>::max());
+    shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) {
+      return std::make_shared<RootShadowNode>(
+          *oldRootShadowNode, ShadowNodeFragment{.children = rootChildNodes});
+    });
   });
 }
 

ReactCommon/fabric/uimanager/ShadowTree.cpp

@@ -109,71 +109,81 @@ Tag ShadowTree::getSurfaceId() const {
   return surfaceId_;
 }
 
-bool ShadowTree::commit(
-    std::function<UnsharedRootShadowNode(
-        const SharedRootShadowNode &oldRootShadowNode)> transaction,
-    int attempts,
-    int *revision) const {
+void ShadowTree::commit(ShadowTreeCommitTransaction transaction, int *revision)
+    const {
   SystraceSection s("ShadowTree::commit");
 
-  while (attempts) {
-    attempts--;
-
-    SharedRootShadowNode oldRootShadowNode;
+  int attempts = 0;
 
-    {
-      // Reading `rootShadowNode_` in shared manner.
-      std::shared_lock<folly::SharedMutex> 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);
+  {
+    // Reading `rootShadowNode_` in shared manner.
+    std::shared_lock<folly::SharedMutex> lock(commitMutex_);
+    oldRootShadowNode = rootShadowNode_;
+  }
 
-    {
-      // Updating `rootShadowNode_` in unique manner if it hasn't changed.
-      std::unique_lock<folly::SharedMutex> lock(commitMutex_);
+  UnsharedRootShadowNode newRootShadowNode = transaction(oldRootShadowNode);
+
+  if (!newRootShadowNode) {
+    return false;
+  }
+
+  newRootShadowNode->layout();
+  newRootShadowNode->sealRecursive();
 
-      if (rootShadowNode_ != oldRootShadowNode) {
-        continue;
-      }
+  auto mutations =
+      calculateShadowViewMutations(*oldRootShadowNode, *newRootShadowNode);
 
-      rootShadowNode_ = newRootShadowNode;
+  {
+    // Updating `rootShadowNode_` in unique manner if it hasn't changed.
+    std::unique_lock<folly::SharedMutex> lock(commitMutex_);
 
-      {
-        std::lock_guard<std::mutex> dispatchLock(EventEmitter::DispatchMutex());
+    if (rootShadowNode_ != oldRootShadowNode) {
+      return false;
+    }
 
-        updateMountedFlag(
-            oldRootShadowNode->getChildren(), newRootShadowNode->getChildren());
-      }
+    rootShadowNode_ = newRootShadowNode;
 
-      revision_++;
+    {
+      std::lock_guard<std::mutex> dispatchLock(EventEmitter::DispatchMutex());
 
-      // Returning last revision if requested.
-      if (revision) {
-        *revision = revision_;
-      }
+      updateMountedFlag(
+          oldRootShadowNode->getChildren(), newRootShadowNode->getChildren());
     }
 
-    emitLayoutEvents(mutations);
+    revision_++;
 
-    if (delegate_) {
-      delegate_->shadowTreeDidCommit(*this, mutations);
+    // Returning last revision if requested.
+    if (revision) {
+      *revision = revision_;
     }
+  }
+
+  emitLayoutEvents(mutations);
 
-    return true;
+  if (delegate_) {
+    delegate_->shadowTreeDidCommit(*this, mutations);
   }
 
-  return false;
+  return true;
 }
 
 void ShadowTree::emitLayoutEvents(

ReactCommon/fabric/uimanager/ShadowTree.h

@@ -19,6 +19,9 @@
 namespace facebook {
 namespace react {
 
+using ShadowTreeCommitTransaction = std::function<UnsharedRootShadowNode(
+    const SharedRootShadowNode &oldRootShadowNode)>;
+
 /*
  * 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<UnsharedRootShadowNode(
-          const SharedRootShadowNode &oldRootShadowNode)> 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
 
   /*

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;
+              });
         });
   }