From c5f704b8e3f1acceff3284bfeca6f3df5d626c6b Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Mon, 21 Oct 2019 09:42:43 -0700 Subject: [PATCH] Changing signature of `RootShadowNode::clone()` to remove a `shared_from_this()` call Summary: This is the first diff in the journey of removing `enable_shared_from_this` from `ShadowNode` class. In general, using `enable_shared_from_this` is fine, it's a normal standard feature and there is nothing wrong with that... besides the fact that using this thing is usually an indication of overall poor design. In Fabric, we don't really have a good reason for that, I think in all cases it can be avoided, so I think we should remove that. Removing that is also code-size and perf win. This particular diff changes the signature of `RootShadowNode::clone()` which allows removing the necessity of usage `.shared_from_this()` from callsites. The rest changes are purely cosmetical. Changelog: [Internal] Small Fabric-specific optimization. Reviewed By: sammy-SC Differential Revision: D17973958 fbshipit-source-id: 5539ceff9d11b4281a6ebe8d80e90d6bd90e44d8 --- .../fabric/components/root/RootShadowNode.cpp | 18 +++++------ .../fabric/components/root/RootShadowNode.h | 18 +++++------ ReactCommon/fabric/mounting/ShadowTree.cpp | 8 ++--- ReactCommon/fabric/mounting/ShadowTree.h | 21 ++++++------ ReactCommon/fabric/uimanager/Scheduler.cpp | 19 +++++------ ReactCommon/fabric/uimanager/UIManager.cpp | 32 +++++++++---------- ReactCommon/fabric/uimanager/UIManager.h | 9 +++--- .../fabric/uimanager/UIManagerBinding.cpp | 2 +- 8 files changed, 64 insertions(+), 63 deletions(-) diff --git a/ReactCommon/fabric/components/root/RootShadowNode.cpp b/ReactCommon/fabric/components/root/RootShadowNode.cpp index 5030bc7934c..363b383f4a4 100644 --- a/ReactCommon/fabric/components/root/RootShadowNode.cpp +++ b/ReactCommon/fabric/components/root/RootShadowNode.cpp @@ -33,10 +33,10 @@ void RootShadowNode::layout( } } -UnsharedRootShadowNode RootShadowNode::clone( - const LayoutConstraints &layoutConstraints, - const LayoutContext &layoutContext) const { - auto props = std::make_shared( +RootShadowNode::Unshared RootShadowNode::clone( + LayoutConstraints const &layoutConstraints, + LayoutContext const &layoutContext) const { + auto props = std::make_shared( *getProps(), layoutConstraints, layoutContext); auto newRootShadowNode = std::make_shared( *this, @@ -48,13 +48,13 @@ UnsharedRootShadowNode RootShadowNode::clone( return newRootShadowNode; } -UnsharedRootShadowNode RootShadowNode::clone( - SharedShadowNode const &oldShadowNode, - SharedShadowNode const &newShadowNode) const { - auto ancestors = oldShadowNode->getAncestors(*this); +RootShadowNode::Unshared RootShadowNode::clone( + ShadowNode const &oldShadowNode, + ShadowNode::Shared const &newShadowNode) const { + auto ancestors = oldShadowNode.getAncestors(*this); if (ancestors.size() == 0) { - return UnsharedRootShadowNode{nullptr}; + return RootShadowNode::Unshared{nullptr}; } auto childNode = newShadowNode; diff --git a/ReactCommon/fabric/components/root/RootShadowNode.h b/ReactCommon/fabric/components/root/RootShadowNode.h index 013e72027e0..c880f30da45 100644 --- a/ReactCommon/fabric/components/root/RootShadowNode.h +++ b/ReactCommon/fabric/components/root/RootShadowNode.h @@ -18,9 +18,6 @@ namespace react { class RootShadowNode; -using SharedRootShadowNode = std::shared_ptr; -using UnsharedRootShadowNode = std::shared_ptr; - extern const char RootComponentName[]; /* @@ -34,6 +31,9 @@ class RootShadowNode final public: using ConcreteViewShadowNode::ConcreteViewShadowNode; + using Shared = std::shared_ptr; + using Unshared = std::shared_ptr; + /* * Layouts the shadow tree. */ @@ -42,9 +42,9 @@ class RootShadowNode final /* * Clones the node with given `layoutConstraints` and `layoutContext`. */ - UnsharedRootShadowNode clone( - const LayoutConstraints &layoutConstraints, - const LayoutContext &layoutContext) const; + RootShadowNode::Unshared clone( + LayoutConstraints const &layoutConstraints, + LayoutContext const &layoutContext) const; /* * Clones the node replacing a given old shadow node with a new one in the @@ -52,9 +52,9 @@ class RootShadowNode final * the tree. Returns `nullptr` if the operation cannot be finished * successfully. */ - UnsharedRootShadowNode clone( - const SharedShadowNode &oldShadowNode, - const SharedShadowNode &newShadowNode) const; + RootShadowNode::Unshared clone( + ShadowNode const &oldShadowNode, + ShadowNode::Shared const &newShadowNode) const; private: using YogaLayoutableShadowNode::layout; diff --git a/ReactCommon/fabric/mounting/ShadowTree.cpp b/ReactCommon/fabric/mounting/ShadowTree.cpp index f43655eae88..cf03be5b2ba 100644 --- a/ReactCommon/fabric/mounting/ShadowTree.cpp +++ b/ReactCommon/fabric/mounting/ShadowTree.cpp @@ -135,7 +135,7 @@ bool ShadowTree::tryCommit(ShadowTreeCommitTransaction transaction) const { auto telemetry = MountingTelemetry{}; telemetry.willCommit(); - SharedRootShadowNode oldRootShadowNode; + RootShadowNode::Shared oldRootShadowNode; { // Reading `rootShadowNode_` in shared manner. @@ -143,7 +143,7 @@ bool ShadowTree::tryCommit(ShadowTreeCommitTransaction transaction) const { oldRootShadowNode = rootShadowNode_; } - UnsharedRootShadowNode newRootShadowNode = transaction(oldRootShadowNode); + RootShadowNode::Unshared newRootShadowNode = transaction(oldRootShadowNode); if (!newRootShadowNode) { return false; @@ -197,8 +197,8 @@ bool ShadowTree::tryCommit(ShadowTreeCommitTransaction transaction) const { void ShadowTree::commitEmptyTree() const { commit( - [](const SharedRootShadowNode &oldRootShadowNode) - -> UnsharedRootShadowNode { + [](RootShadowNode::Shared const &oldRootShadowNode) + -> RootShadowNode::Unshared { return std::make_shared( *oldRootShadowNode, ShadowNodeFragment{ diff --git a/ReactCommon/fabric/mounting/ShadowTree.h b/ReactCommon/fabric/mounting/ShadowTree.h index a059e60f70b..95dc67ac1ff 100644 --- a/ReactCommon/fabric/mounting/ShadowTree.h +++ b/ReactCommon/fabric/mounting/ShadowTree.h @@ -23,8 +23,8 @@ namespace facebook { namespace react { -using ShadowTreeCommitTransaction = std::function; +using ShadowTreeCommitTransaction = std::function; /* * Represents the shadow tree and its lifecycle. @@ -36,9 +36,9 @@ class ShadowTree final { */ ShadowTree( SurfaceId surfaceId, - const LayoutConstraints &layoutConstraints, - const LayoutContext &layoutContext, - const RootComponentDescriptor &rootComponentDescriptor); + LayoutConstraints const &layoutConstraints, + LayoutContext const &layoutContext, + RootComponentDescriptor const &rootComponentDescriptor); ~ShadowTree(); @@ -76,17 +76,18 @@ class ShadowTree final { ShadowTreeDelegate const *getDelegate() const; private: - UnsharedRootShadowNode cloneRootShadowNode( - const SharedRootShadowNode &oldRootShadowNode, - const LayoutConstraints &layoutConstraints, - const LayoutContext &layoutContext) const; + RootShadowNode::Unshared cloneRootShadowNode( + RootShadowNode::Shared const &oldRootShadowNode, + LayoutConstraints const &layoutConstraints, + LayoutContext const &layoutContext) const; void emitLayoutEvents( std::vector &affectedLayoutableNodes) const; SurfaceId const surfaceId_; mutable better::shared_mutex commitMutex_; - mutable SharedRootShadowNode rootShadowNode_; // Protected by `commitMutex_`. + mutable RootShadowNode::Shared + rootShadowNode_; // Protected by `commitMutex_`. mutable ShadowTreeRevision::Number revisionNumber_{ 0}; // Protected by `commitMutex_`. ShadowTreeDelegate const *delegate_; diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index a7f53ba9629..b933c28a292 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -32,7 +32,8 @@ Scheduler::Scheduler( auto uiManager = std::make_shared(); auto eventOwnerBox = std::make_shared(); - auto eventPipe = [=](jsi::Runtime &runtime, + auto eventPipe = [uiManager]( + jsi::Runtime &runtime, const EventTarget *eventTarget, const std::string &type, const ValueFactory &payloadFactory) { @@ -42,10 +43,10 @@ Scheduler::Scheduler( }); }; - auto statePipe = [=](const StateData::Shared &data, + auto statePipe = [uiManager]( + const StateData::Shared &data, const StateTarget &stateTarget) { - uiManager->updateState( - stateTarget.getShadowNode().shared_from_this(), data); + uiManager->updateState(stateTarget.getShadowNode(), data); }; eventDispatcher_ = std::make_shared( @@ -127,7 +128,7 @@ void Scheduler::renderTemplateToSurface( uiManager_->getShadowTreeRegistry().visit( surfaceId, [=](const ShadowTree &shadowTree) { - return shadowTree.tryCommit([&](const SharedRootShadowNode + return shadowTree.tryCommit([&](RootShadowNode::Shared const &oldRootShadowNode) { return std::make_shared( *oldRootShadowNode, @@ -189,7 +190,7 @@ Size Scheduler::measureSurface( uiManager_->getShadowTreeRegistry().visit( surfaceId, [&](const ShadowTree &shadowTree) { shadowTree.tryCommit( - [&](const SharedRootShadowNode &oldRootShadowNode) { + [&](RootShadowNode::Shared const &oldRootShadowNode) { auto rootShadowNode = oldRootShadowNode->clone(layoutConstraints, layoutContext); rootShadowNode->layout(); @@ -207,8 +208,8 @@ void Scheduler::constraintSurfaceLayout( SystraceSection s("Scheduler::constraintSurfaceLayout"); uiManager_->getShadowTreeRegistry().visit( - surfaceId, [&](const ShadowTree &shadowTree) { - shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { + surfaceId, [&](ShadowTree const &shadowTree) { + shadowTree.commit([&](RootShadowNode::Shared const &oldRootShadowNode) { return oldRootShadowNode->clone(layoutConstraints, layoutContext); }); }); @@ -250,7 +251,7 @@ void Scheduler::uiManagerDidFinishTransaction( uiManager_->getShadowTreeRegistry().visit( surfaceId, [&](const ShadowTree &shadowTree) { - shadowTree.commit([&](const SharedRootShadowNode &oldRootShadowNode) { + shadowTree.commit([&](RootShadowNode::Shared const &oldRootShadowNode) { return std::make_shared( *oldRootShadowNode, ShadowNodeFragment{ diff --git a/ReactCommon/fabric/uimanager/UIManager.cpp b/ReactCommon/fabric/uimanager/UIManager.cpp index 2ac391ae3dd..aec8da8ce67 100644 --- a/ReactCommon/fabric/uimanager/UIManager.cpp +++ b/ReactCommon/fabric/uimanager/UIManager.cpp @@ -125,22 +125,22 @@ void UIManager::clearJSResponder() const { } void UIManager::setNativeProps( - const SharedShadowNode &shadowNode, - const RawProps &rawProps) const { + ShadowNode const &shadowNode, + RawProps const &rawProps) const { SystraceSection s("UIManager::setNativeProps"); - auto &componentDescriptor = shadowNode->getComponentDescriptor(); - auto props = componentDescriptor.cloneProps(shadowNode->getProps(), rawProps); - auto newShadowNode = shadowNode->clone({ + auto &componentDescriptor = shadowNode.getComponentDescriptor(); + auto props = componentDescriptor.cloneProps(shadowNode.getProps(), rawProps); + auto newShadowNode = shadowNode.clone({ /* .tag = */ ShadowNodeFragment::tagPlaceholder(), /* .surfaceId = */ ShadowNodeFragment::surfaceIdPlaceholder(), /* .props = */ props, }); shadowTreeRegistry_.visit( - shadowNode->getSurfaceId(), [&](const ShadowTree &shadowTree) { + shadowNode.getSurfaceId(), [&](ShadowTree const &shadowTree) { shadowTree.tryCommit( - [&](const SharedRootShadowNode &oldRootShadowNode) { + [&](RootShadowNode::Shared const &oldRootShadowNode) { return oldRootShadowNode->clone(shadowNode, newShadowNode); }); }); @@ -155,7 +155,7 @@ LayoutMetrics UIManager::getRelativeLayoutMetrics( shadowTreeRegistry_.visit( shadowNode.getSurfaceId(), [&](const ShadowTree &shadowTree) { shadowTree.tryCommit( - [&](const SharedRootShadowNode &oldRootShadowNode) { + [&](RootShadowNode::Shared const &oldRootShadowNode) { ancestorShadowNode = oldRootShadowNode.get(); return nullptr; }); @@ -176,12 +176,12 @@ LayoutMetrics UIManager::getRelativeLayoutMetrics( } void UIManager::updateState( - const SharedShadowNode &shadowNode, - const StateData::Shared &rawStateData) const { - auto &componentDescriptor = shadowNode->getComponentDescriptor(); + ShadowNode const &shadowNode, + StateData::Shared const &rawStateData) const { + auto &componentDescriptor = shadowNode.getComponentDescriptor(); auto state = - componentDescriptor.createState(shadowNode->getState(), rawStateData); - auto newShadowNode = shadowNode->clone({ + componentDescriptor.createState(shadowNode.getState(), rawStateData); + auto newShadowNode = shadowNode.clone({ /* .tag = */ ShadowNodeFragment::tagPlaceholder(), /* .surfaceId = */ ShadowNodeFragment::surfaceIdPlaceholder(), /* .props = */ ShadowNodeFragment::propsPlaceholder(), @@ -192,9 +192,9 @@ void UIManager::updateState( }); shadowTreeRegistry_.visit( - shadowNode->getSurfaceId(), [&](const ShadowTree &shadowTree) { + shadowNode.getSurfaceId(), [&](const ShadowTree &shadowTree) { shadowTree.tryCommit( - [&](const SharedRootShadowNode &oldRootShadowNode) { + [&](RootShadowNode::Shared const &oldRootShadowNode) { return oldRootShadowNode->clone(shadowNode, newShadowNode); }); }); @@ -236,7 +236,7 @@ ShadowNode::Shared UIManager::findShadowNodeByTag_DEPRECATED(Tag tag) const { // pointer to a root node because of the possible data race. // To work around this, we ask for a commit and immediately cancel it // returning `nullptr` instead of a new shadow tree. - shadowTree.tryCommit([&](SharedRootShadowNode const &oldRootShadowNode) { + shadowTree.tryCommit([&](RootShadowNode::Shared const &oldRootShadowNode) { rootShadowNode = oldRootShadowNode; return nullptr; }); diff --git a/ReactCommon/fabric/uimanager/UIManager.h b/ReactCommon/fabric/uimanager/UIManager.h index a769b6f8705..58e03073fd5 100644 --- a/ReactCommon/fabric/uimanager/UIManager.h +++ b/ReactCommon/fabric/uimanager/UIManager.h @@ -70,9 +70,8 @@ class UIManager { SurfaceId surfaceId, const SharedShadowNodeUnsharedList &rootChildren) const; - void setNativeProps( - const SharedShadowNode &shadowNode, - const RawProps &rawProps) const; + void setNativeProps(ShadowNode const &shadowNode, RawProps const &rawProps) + const; void setJSResponder( const SharedShadowNode &shadowNode, @@ -94,8 +93,8 @@ class UIManager { * and performs a commit. */ void updateState( - const SharedShadowNode &shadowNode, - const StateData::Shared &rawStateData) const; + ShadowNode const &shadowNode, + StateData::Shared const &rawStateData) const; void dispatchCommand( const SharedShadowNode &shadowNode, diff --git a/ReactCommon/fabric/uimanager/UIManagerBinding.cpp b/ReactCommon/fabric/uimanager/UIManagerBinding.cpp index fcc8255a892..15f5da97724 100644 --- a/ReactCommon/fabric/uimanager/UIManagerBinding.cpp +++ b/ReactCommon/fabric/uimanager/UIManagerBinding.cpp @@ -551,7 +551,7 @@ jsi::Value UIManagerBinding::get( const jsi::Value *arguments, size_t count) -> jsi::Value { uiManager->setNativeProps( - shadowNodeFromValue(runtime, arguments[0]), + *shadowNodeFromValue(runtime, arguments[0]), RawProps(runtime, arguments[1])); return jsi::Value::undefined();