From 432b434b559ae7f409608fa26513660c46718d21 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Wed, 26 Feb 2020 22:03:14 -0800 Subject: [PATCH] Fabric: LayoutableShadowNode now inherits ShadowNode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This is a very crucial change, everything else in this stack depends on it. Here is the set of constraints that we have for ShadowNode-based class hierarchy: * `ShadowNode` is a base class that defines basic operations on the nodes. It does not have virtual methods by design (virtual dispatch hurts performance; we want to limit and centralize that in `ComponentDescriptor`s). * `ConcreteShadowNode<>` template *statically* wires particular `ShadowNode` and particular `Props` establishing a type-safe relationship between them ensured on compile time. * Not all `ShadowNode`s are "layoutable", not all "layoutable" `ShadowNode`s use Yoga to do layout. * Layotability (and YogaLayotability) feature is implemented as two classes `LayoutableShadowNode` and `YogaLayoutableShadowNode`. These classes essentially need to know something about the ShadowNode nature of the object (get the list of children or replace some children). Before the change, `LayoutableShadowNode` and `YogaLayoutableShadowNode`classes did not inherit `ShadowNode` because we don't want to use *virtual inheritance* for `ShadowNode`: `ConcreteShadowNode<>` already inherits `ShadowNode`, so if we make `LayoutableShadowNode` inherits `ShadowNode`, we will have to somehow flatten this inheritance hierarchy (and the virtual inheritance is an answer to that). (Yes, C++ supports multiple inheritance and virtual inheritance.) Before this change, we solved this dilemma this way: We have a subclass-template `ConcreteViewShadowNode<>` that inherits `ConcreteShadowNode<>` and `YogaLayoutableShadowNode`. Then, we had a bunch of methods that implement the rerouting of some functionality from `YogaLayoutableShadowNode` to `ShadowNode` and vise-versa. (See the diagram "Before".) That worked fine, except the caveats: * That wiring is nasty, complex, hard to reason about and overall limiting. * There is no way to statically cast `LayoutableShadowNode` to `ShadowNode` (because there is no common base class). That forces us to use dynamic_cast on some perf critical paths (including layout, diffing and so on). * Adding features that rely on interop between `LayoutableShadowNode` and `ShadowNode` is a nightmare. It should be a better way to deal with this dilemma, and this diff implements a different approach: We can have a base class of `ConcreteShadowNode<>` as a template parameter. With this approach, we can make `LayoutableShadowNode` inherit `ShadowNode` and when we need to instantiate a `ConcreteShadowNode<>` that needs to be layoutable, we can just specify `YogaLayoutableShadowNode` as a base class. (See the diagram "After".) This simple change will allow us to simplify a lot of things. The rest of the stack is about getting rid of unnecessary moving parts. Which will finally allow us to build "Inline Views" feature. ``` ╭──────────────────────╮ │ ◎ ○ ○ ░░░░░░░░░░░░░░░│ ├──────────────────────┤ │ │ │ │ │ Before │ │ │ ┌────────────────────────────┐ ┌────────────────────────────┐ │ │ │ │ │ │ │ │ │ ShadowNode │ │ LayoutableShadowNode │ └──────────────────────┘ │ │ │ │ └────────────────────────────┘ └────────────────────────────┘ ▲ ▲ │ │ ╔════════════════════════════╗ ┌────────────────────────────┐ ║ ║ │ │ ┌────────�║ ConcreteShadowNode<> ║ │ YogaLayoutableShadowNode │ │ ║ ║ │ │ │ ╚════════════════════════════╝ └────────────────────────────┘ │ ▲ ▲ │ │ │ │ │ │ │ │ │ │ │ ╔════════════════════════════╗ │ │ │ ║ ║ │ │ └─────║ ConcreteViewShadowNode<> ║─┘ │ ║ ║ │ ╚════════════════════════════╝ │ ▲ │ │ │ ┌──────────────┴───────────────┐ │ │ │ │ │ │ │ │ │ ┌────────────────────────────┐ ┌────────────────────────────┐ ┌────────────────────────────┐ │ │ │ │ │ │ │ TextShadowNode │ │ ViewShadowNode │ │ ParagraphShadowNode │ │ │ │ │ │ │ └────────────────────────────┘ └────────────────────────────┘ └────────────────────────────┘ ╭──────────────────────╮ │ ◎ ○ ○ ░░░░░░░░░░░░░░░│ ├──────────────────────┤ │ │ ┌────────────────────────────┐ │ │ │ │ │ After │ │ ShadowNode │ │ │ │ │ │ │ └────────────────────────────┘ │ │ ▲ └──────────────────────┘ ┌────────────────────────────────────┤ │ │ │ ╔════════════════════════════╗ ┌────────────────────────────┐ ║ ConcreteShadowNode ║ │ │ ║ ║ │ LayoutableShadowNode │ ║ ║ │ │ ╚════════════════════════════╝ └────────────────────────────┘ ▲ ▲ │ │ ┌────────────────────────────┐ ┌────────────────────────────┐ │ │ │ │ │ TextShadowNode │ │ YogaLayoutableShadowNode │ │ │ │ │ └────────────────────────────┘ └────────────────────────────┘ ▲ │ ╔════════════════════════════╗ ║ ConcreteShadowNode ║ ║ ║ ╚════════════════════════════╝ ▲ │ ╔════════════════════════════╗ ║ ║ ║ ConcreteViewShadowNode<> ║ ║ ║ ╚════════════════════════════╝ ▲ ├─────────────────────────────────────┐ │ │ ┌────────────────────────────┐ ┌────────────────────────────┐ │ │ │ │ │ ParagraphShadowNode │ │ ViewShadowNode │ │ │ │ │ └────────────────────────────┘ └────────────────────────────┘ ``` Changelog: [Internal] Fabric-specific internal change. Reviewed By: sammy-SC Differential Revision: D19963353 fbshipit-source-id: b65c8a5064bdb54ab64f08a8e546aa9e2b5a486b --- .../text/rawtext/RawTextShadowNode.h | 2 +- .../components/text/text/TextShadowNode.h | 9 ++++-- .../components/view/ConcreteViewShadowNode.h | 29 ++++++++++-------- .../view/yoga/YogaLayoutableShadowNode.cpp | 30 +++++++++++-------- .../view/yoga/YogaLayoutableShadowNode.h | 19 +++++------- .../core/layout/LayoutableShadowNode.cpp | 13 ++++++++ .../fabric/core/layout/LayoutableShadowNode.h | 16 ++++++---- .../core/shadownode/ConcreteShadowNode.h | 18 +++++++---- .../fabric/core/shadownode/ShadowNode.h | 3 +- .../fabric/core/shadownode/ShadowNodeTraits.h | 3 ++ .../debug/DebugStringConvertibleItem.cpp | 4 +-- .../fabric/debug/DebugStringConvertibleItem.h | 2 +- 12 files changed, 93 insertions(+), 55 deletions(-) diff --git a/ReactCommon/fabric/components/text/rawtext/RawTextShadowNode.h b/ReactCommon/fabric/components/text/rawtext/RawTextShadowNode.h index 772834549ee..e93368a8c69 100644 --- a/ReactCommon/fabric/components/text/rawtext/RawTextShadowNode.h +++ b/ReactCommon/fabric/components/text/rawtext/RawTextShadowNode.h @@ -22,7 +22,7 @@ extern const char RawTextComponentName[]; * component must not have any children. */ using RawTextShadowNode = - ConcreteShadowNode; + ConcreteShadowNode; } // namespace react } // namespace facebook diff --git a/ReactCommon/fabric/components/text/text/TextShadowNode.h b/ReactCommon/fabric/components/text/text/TextShadowNode.h index 0ef0e91c58c..c6824e55ce8 100644 --- a/ReactCommon/fabric/components/text/text/TextShadowNode.h +++ b/ReactCommon/fabric/components/text/text/TextShadowNode.h @@ -19,9 +19,12 @@ extern const char TextComponentName[]; using TextEventEmitter = TouchEventEmitter; -class TextShadowNode - : public ConcreteShadowNode, - public BaseTextShadowNode { +class TextShadowNode : public ConcreteShadowNode< + TextComponentName, + ShadowNode, + TextProps, + TextEventEmitter>, + public BaseTextShadowNode { public: using ConcreteShadowNode::ConcreteShadowNode; }; diff --git a/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h b/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h index c76eec7521e..de6e80d958e 100644 --- a/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h +++ b/ReactCommon/fabric/components/view/ConcreteViewShadowNode.h @@ -31,10 +31,10 @@ template < typename... Ts> class ConcreteViewShadowNode : public ConcreteShadowNode< concreteComponentName, + YogaLayoutableShadowNode, ViewPropsT, ViewEventEmitterT, - Ts...>, - public YogaLayoutableShadowNode { + Ts...> { static_assert( std::is_base_of::value, "ViewPropsT must be a descendant of ViewProps"); @@ -48,6 +48,7 @@ class ConcreteViewShadowNode : public ConcreteShadowNode< public: using BaseShadowNode = ConcreteShadowNode< concreteComponentName, + YogaLayoutableShadowNode, ViewPropsT, ViewEventEmitterT, Ts...>; @@ -57,21 +58,17 @@ class ConcreteViewShadowNode : public ConcreteShadowNode< ShadowNodeFragment const &fragment, ShadowNodeFamily::Shared const &family, ShadowNodeTraits traits) - : BaseShadowNode(fragment, family, traits), - YogaLayoutableShadowNode( - traits.check(ShadowNodeTraits::Trait::LeafYogaNode)) { + : BaseShadowNode(fragment, family, traits) { YogaLayoutableShadowNode::setProps( *std::static_pointer_cast(fragment.props)); YogaLayoutableShadowNode::setChildren( BaseShadowNode::template getChildrenSlice()); - }; + } ConcreteViewShadowNode( ShadowNode const &sourceShadowNode, ShadowNodeFragment const &fragment) - : BaseShadowNode(sourceShadowNode, fragment), - YogaLayoutableShadowNode( - static_cast(sourceShadowNode)) { + : BaseShadowNode(sourceShadowNode, fragment) { if (fragment.props) { YogaLayoutableShadowNode::setProps( *std::static_pointer_cast(fragment.props)); @@ -82,10 +79,18 @@ class ConcreteViewShadowNode : public ConcreteShadowNode< BaseShadowNode::template getChildrenSlice< YogaLayoutableShadowNode>()); } - }; + } + + static ShadowNodeTraits BaseTraits() { + auto traits = BaseShadowNode::BaseTraits(); + traits.set(ShadowNodeTraits::Trait::LayoutableKind); + traits.set(ShadowNodeTraits::Trait::YogaLayoutableKind); + traits.set(ShadowNodeTraits::Trait::ViewKind); + return traits; + } void appendChild(const ShadowNode::Shared &child) { - ensureUnsealed(); + BaseShadowNode::ensureUnsealed(); ShadowNode::appendChild(child); @@ -100,7 +105,7 @@ class ConcreteViewShadowNode : public ConcreteShadowNode< LayoutableShadowNode *cloneAndReplaceChild( LayoutableShadowNode *child, int suggestedIndex = -1) override { - ensureUnsealed(); + Sealable::ensureUnsealed(); auto childShadowNode = static_cast(child); auto clonedChildShadowNode = std::static_pointer_cast( diff --git a/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.cpp b/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.cpp index e9307b0a23f..d5dd7b14dd3 100644 --- a/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.cpp +++ b/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.cpp @@ -21,26 +21,32 @@ namespace facebook { namespace react { -YogaLayoutableShadowNode::YogaLayoutableShadowNode(bool isLeaf) - : yogaConfig_(nullptr), - yogaNode_(&initializeYogaConfig(yogaConfig_)), - isLeaf_(isLeaf) { +YogaLayoutableShadowNode::YogaLayoutableShadowNode( + ShadowNodeFragment const &fragment, + ShadowNodeFamily::Shared const &family, + ShadowNodeTraits traits) + : LayoutableShadowNode(fragment, family, traits), + yogaConfig_(nullptr), + yogaNode_(&initializeYogaConfig(yogaConfig_)) { yogaNode_.setContext(this); } YogaLayoutableShadowNode::YogaLayoutableShadowNode( - YogaLayoutableShadowNode const &layoutableShadowNode) - : LayoutableShadowNode(layoutableShadowNode), + ShadowNode const &sourceShadowNode, + ShadowNodeFragment const &fragment) + : LayoutableShadowNode(sourceShadowNode, fragment), yogaConfig_(nullptr), yogaNode_( - layoutableShadowNode.yogaNode_, - &initializeYogaConfig(yogaConfig_)), - isLeaf_(layoutableShadowNode.isLeaf_) { + static_cast(sourceShadowNode) + .yogaNode_, + &initializeYogaConfig(yogaConfig_)) { yogaNode_.setContext(this); yogaNode_.setOwner(nullptr); // Yoga node must inherit dirty flag. - assert(layoutableShadowNode.yogaNode_.isDirty() == yogaNode_.isDirty()); + assert( + static_cast(sourceShadowNode) + .yogaNode_.isDirty() == yogaNode_.isDirty()); } void YogaLayoutableShadowNode::cleanLayout() { @@ -73,7 +79,7 @@ void YogaLayoutableShadowNode::enableMeasurement() { } void YogaLayoutableShadowNode::appendChild(YogaLayoutableShadowNode *child) { - if (isLeaf_) { + if (getTraits().check(ShadowNodeTraits::Trait::LeafYogaNode)) { return; } @@ -102,7 +108,7 @@ void YogaLayoutableShadowNode::appendChild(YogaLayoutableShadowNode *child) { void YogaLayoutableShadowNode::setChildren( YogaLayoutableShadowNode::UnsharedList children) { - if (isLeaf_) { + if (getTraits().check(ShadowNodeTraits::Trait::LeafYogaNode)) { return; } diff --git a/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.h b/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.h index 026a3e9d5ba..68f603840ef 100644 --- a/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.h +++ b/ReactCommon/fabric/components/view/yoga/YogaLayoutableShadowNode.h @@ -22,9 +22,7 @@ namespace facebook { namespace react { -class YogaLayoutableShadowNode : public LayoutableShadowNode, - public virtual DebugStringConvertible, - public virtual Sealable { +class YogaLayoutableShadowNode : public LayoutableShadowNode { public: using UnsharedList = better::small_vector< YogaLayoutableShadowNode *, @@ -32,10 +30,14 @@ class YogaLayoutableShadowNode : public LayoutableShadowNode, #pragma mark - Constructors - YogaLayoutableShadowNode(bool isLeaf); + YogaLayoutableShadowNode( + ShadowNodeFragment const &fragment, + ShadowNodeFamily::Shared const &family, + ShadowNodeTraits traits); YogaLayoutableShadowNode( - YogaLayoutableShadowNode const &layoutableShadowNode); + ShadowNode const &sourceShadowNode, + ShadowNodeFragment const &fragment); #pragma mark - Mutating Methods @@ -108,13 +110,6 @@ class YogaLayoutableShadowNode : public LayoutableShadowNode, */ mutable YGNode yogaNode_; - /* - * Forces associated YGNode to be a leaf. - * Adding a child `ShadowNode` will not add `YGNode` associated with it as a - * child to the stored `YGNode`. - */ - bool const isLeaf_; - private: static YGConfig &initializeYogaConfig(YGConfig &config); static YGNode *yogaNodeCloneCallbackConnector( diff --git a/ReactCommon/fabric/core/layout/LayoutableShadowNode.cpp b/ReactCommon/fabric/core/layout/LayoutableShadowNode.cpp index b88582b458f..18033b32a5c 100644 --- a/ReactCommon/fabric/core/layout/LayoutableShadowNode.cpp +++ b/ReactCommon/fabric/core/layout/LayoutableShadowNode.cpp @@ -68,6 +68,19 @@ static LayoutMetrics calculateOffsetForLayoutMetrics( return layoutMetrics; } +LayoutableShadowNode::LayoutableShadowNode( + ShadowNodeFragment const &fragment, + ShadowNodeFamily::Shared const &family, + ShadowNodeTraits traits) + : ShadowNode(fragment, family, traits), layoutMetrics_({}) {} + +LayoutableShadowNode::LayoutableShadowNode( + ShadowNode const &sourceShadowNode, + ShadowNodeFragment const &fragment) + : ShadowNode(sourceShadowNode, fragment), + layoutMetrics_(static_cast(sourceShadowNode) + .layoutMetrics_) {} + LayoutMetrics LayoutableShadowNode::getLayoutMetrics() const { return layoutMetrics_; } diff --git a/ReactCommon/fabric/core/layout/LayoutableShadowNode.h b/ReactCommon/fabric/core/layout/LayoutableShadowNode.h index 88cd38ea3b2..ca946be4589 100644 --- a/ReactCommon/fabric/core/layout/LayoutableShadowNode.h +++ b/ReactCommon/fabric/core/layout/LayoutableShadowNode.h @@ -14,7 +14,6 @@ #include #include -#include #include #include #include @@ -30,8 +29,17 @@ struct LayoutContext; * Describes all sufficient layout API (in approach-agnostic way) * which makes a concurrent layout possible. */ -class LayoutableShadowNode : public virtual Sealable { +class LayoutableShadowNode : public ShadowNode { public: + LayoutableShadowNode( + ShadowNodeFragment const &fragment, + ShadowNodeFamily::Shared const &family, + ShadowNodeTraits traits); + + LayoutableShadowNode( + ShadowNode const &sourceShadowNode, + ShadowNodeFragment const &fragment); + class LayoutInspectingPolicy final { public: bool includeTransform{true}; @@ -41,8 +49,6 @@ class LayoutableShadowNode : public virtual Sealable { using UnsharedList = better:: small_vector; - virtual ~LayoutableShadowNode() noexcept = default; - /* * Measures the node (and node content, probably recursively) with * given constrains and relying on possible layout. @@ -151,7 +157,7 @@ class LayoutableShadowNode : public virtual Sealable { #endif private: - LayoutMetrics layoutMetrics_{}; + LayoutMetrics layoutMetrics_; }; } // namespace react diff --git a/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h b/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h index 92bad021c49..2ce67c279fa 100644 --- a/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h +++ b/ReactCommon/fabric/core/shadownode/ConcreteShadowNode.h @@ -23,16 +23,24 @@ namespace react { */ template < ComponentName concreteComponentName, + typename BaseShadowNodeT, typename PropsT, typename EventEmitterT = EventEmitter, typename StateDataT = StateData> -class ConcreteShadowNode : public ShadowNode { +class ConcreteShadowNode : public BaseShadowNodeT { + static_assert( + std::is_base_of::value, + "BaseShadowNodeT must be a descendant of ShadowNode"); static_assert( std::is_base_of::value, "PropsT must be a descendant of Props"); + protected: + using ShadowNode::props_; + using ShadowNode::state_; + public: - using ShadowNode::ShadowNode; + using BaseShadowNodeT::BaseShadowNodeT; using ConcreteProps = PropsT; using SharedConcreteProps = std::shared_ptr; @@ -84,7 +92,7 @@ class ConcreteShadowNode : public ShadowNode { * Thread-safe after the node is sealed. */ ConcreteProps const &getConcreteProps() const { - assert(props_ && "Props must not be `nullptr`."); + assert(BaseShadowNodeT::props_ && "Props must not be `nullptr`."); assert( std::dynamic_pointer_cast(props_) && "Props must be an instance of ConcreteProps class."); @@ -108,7 +116,7 @@ class ConcreteShadowNode : public ShadowNode { * Can be called only before the node is sealed (usually during construction). */ void setStateData(ConcreteStateData &&data) { - ensureUnsealed(); + Sealable::ensureUnsealed(); state_ = std::make_shared( std::make_shared(std::move(data)), *state_); } @@ -123,7 +131,7 @@ class ConcreteShadowNode : public ShadowNode { better:: small_vector children; - for (auto const &childShadowNode : getChildren()) { + for (auto const &childShadowNode : ShadowNode::getChildren()) { auto specificChildShadowNode = dynamic_cast(childShadowNode.get()); if (specificChildShadowNode) { diff --git a/ReactCommon/fabric/core/shadownode/ShadowNode.h b/ReactCommon/fabric/core/shadownode/ShadowNode.h index 223d189c282..63ccdfcd866 100644 --- a/ReactCommon/fabric/core/shadownode/ShadowNode.h +++ b/ReactCommon/fabric/core/shadownode/ShadowNode.h @@ -39,8 +39,7 @@ using SharedShadowNodeList = using SharedShadowNodeSharedList = std::shared_ptr; using SharedShadowNodeUnsharedList = std::shared_ptr; -class ShadowNode : public virtual Sealable, - public virtual DebugStringConvertible { +class ShadowNode : public Sealable, public DebugStringConvertible { public: using Shared = std::shared_ptr; using Weak = std::weak_ptr; diff --git a/ReactCommon/fabric/core/shadownode/ShadowNodeTraits.h b/ReactCommon/fabric/core/shadownode/ShadowNodeTraits.h index 08f9a6ac3b4..46ad563d712 100644 --- a/ReactCommon/fabric/core/shadownode/ShadowNodeTraits.h +++ b/ReactCommon/fabric/core/shadownode/ShadowNodeTraits.h @@ -47,6 +47,9 @@ class ShadowNodeTraits { // traverse beyond this node. See T61257516 for details. RootNodeKind = 1 << 4, + // `ViewShadowNode` (exact!) class. + View = 1 << 5, + // Inherits `YogaLayoutableShadowNode` and enforces that the `YGNode` is a // leaf. LeafYogaNode = 1 << 10, diff --git a/ReactCommon/fabric/debug/DebugStringConvertibleItem.cpp b/ReactCommon/fabric/debug/DebugStringConvertibleItem.cpp index d43fccbd40b..73606d87424 100644 --- a/ReactCommon/fabric/debug/DebugStringConvertibleItem.cpp +++ b/ReactCommon/fabric/debug/DebugStringConvertibleItem.cpp @@ -17,7 +17,7 @@ DebugStringConvertibleItem::DebugStringConvertibleItem( const std::string &value, const SharedDebugStringConvertibleList &props, const SharedDebugStringConvertibleList &children) - : name_(name), value_(value), props_(props), children_(children) {} + : name_(name), value_(value), debugProps_(props), children_(children) {} std::string DebugStringConvertibleItem::getDebugName() const { return name_; @@ -29,7 +29,7 @@ std::string DebugStringConvertibleItem::getDebugValue() const { SharedDebugStringConvertibleList DebugStringConvertibleItem::getDebugProps() const { - return props_; + return debugProps_; } SharedDebugStringConvertibleList DebugStringConvertibleItem::getDebugChildren() diff --git a/ReactCommon/fabric/debug/DebugStringConvertibleItem.h b/ReactCommon/fabric/debug/DebugStringConvertibleItem.h index ed362b03cf4..6769aa6b564 100644 --- a/ReactCommon/fabric/debug/DebugStringConvertibleItem.h +++ b/ReactCommon/fabric/debug/DebugStringConvertibleItem.h @@ -37,7 +37,7 @@ class DebugStringConvertibleItem : public DebugStringConvertible { private: std::string name_; std::string value_; - SharedDebugStringConvertibleList props_; + SharedDebugStringConvertibleList debugProps_; SharedDebugStringConvertibleList children_; };