From 1953f6f02e9b1b15498dff33badcb4ed2bcf9ca0 Mon Sep 17 00:00:00 2001 From: Andrei Shikov Date: Tue, 22 Feb 2022 16:29:13 -0800 Subject: [PATCH] Exclude raw props from view shadow nodes Summary: With the `MapBuffer`-based props calculated from C++ props, there's no need to keep `rawProps` around for Android views. This change makes sure that the `rawProps` field is only initialized under the feature flag that is responsible for enabling `MapBuffer` for prop diffing, potentially decreasing memory footprint and speeding up node initialization as JS props don't have to be converted to `folly::dynamic` anymore. For layout animations, props rely on C++ values, so there's no need to update `rawProps` values either. Changelog: [Internal][Android] - Do not init `rawProps` when mapbuffer serialization is used for ViewProps. Reviewed By: mdvacca Differential Revision: D33793044 fbshipit-source-id: 35873b10d3ca8b152b25344ef2c27aff9641846f --- .../fabric/jni/FabricMountingManager.cpp | 3 ++ .../react/renderer/components/view/BUCK | 1 + .../renderer/components/view/ViewProps.cpp | 5 +-- .../renderer/components/view/ViewProps.h | 3 +- .../components/view/ViewPropsInterpolation.h | 8 +++-- .../components/view/ViewShadowNode.cpp | 32 +++++++++++++++++++ .../renderer/components/view/ViewShadowNode.h | 14 +++++++- .../components/view/YogaStylableProps.cpp | 5 +-- .../components/view/YogaStylableProps.h | 3 +- .../components/view/tests/LayoutTest.cpp | 10 +++--- .../components/view/tests/ViewTest.cpp | 6 ++-- ReactCommon/react/renderer/core/Props.cpp | 7 ++-- ReactCommon/react/renderer/core/Props.h | 3 +- .../core/tests/FindNodeAtPointTest.cpp | 22 ++++++------- .../core/tests/LayoutableShadowNodeTest.cpp | 6 ++-- .../renderer/element/tests/ElementTest.cpp | 6 ++-- .../mounting/tests/StackingContextTest.cpp | 2 +- 17 files changed, 96 insertions(+), 40 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp index f8a5920e48..a8b8357417 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/FabricMountingManager.cpp @@ -223,6 +223,9 @@ local_ref FabricMountingManager::getProps( ShadowView const &newShadowView) { if (useMapBufferForViewProps_ && newShadowView.traits.check(ShadowNodeTraits::Trait::View)) { + react_native_assert( + newShadowView.props->rawProps.empty() && + "Raw props must be empty when views are using mapbuffer"); auto oldProps = oldShadowView.props != nullptr ? static_cast(*oldShadowView.props) : ViewProps{}; diff --git a/ReactCommon/react/renderer/components/view/BUCK b/ReactCommon/react/renderer/components/view/BUCK index b82e163974..0f64263885 100644 --- a/ReactCommon/react/renderer/components/view/BUCK +++ b/ReactCommon/react/renderer/components/view/BUCK @@ -60,6 +60,7 @@ rn_xplat_cxx_library( react_native_xplat_target("react/renderer/core:core"), react_native_xplat_target("react/renderer/debug:debug"), react_native_xplat_target("react/renderer/graphics:graphics"), + react_native_xplat_target("react/config:config"), react_native_xplat_target("logger:logger"), ], ) diff --git a/ReactCommon/react/renderer/components/view/ViewProps.cpp b/ReactCommon/react/renderer/components/view/ViewProps.cpp index 5b9cd99e5f..aadc27692b 100644 --- a/ReactCommon/react/renderer/components/view/ViewProps.cpp +++ b/ReactCommon/react/renderer/components/view/ViewProps.cpp @@ -21,8 +21,9 @@ namespace react { ViewProps::ViewProps( const PropsParserContext &context, ViewProps const &sourceProps, - RawProps const &rawProps) - : YogaStylableProps(context, sourceProps, rawProps), + RawProps const &rawProps, + bool shouldSetRawProps) + : YogaStylableProps(context, sourceProps, rawProps, shouldSetRawProps), AccessibilityProps(context, sourceProps, rawProps), opacity(convertRawProp( context, diff --git a/ReactCommon/react/renderer/components/view/ViewProps.h b/ReactCommon/react/renderer/components/view/ViewProps.h index 497604809b..7e131604bd 100644 --- a/ReactCommon/react/renderer/components/view/ViewProps.h +++ b/ReactCommon/react/renderer/components/view/ViewProps.h @@ -30,7 +30,8 @@ class ViewProps : public YogaStylableProps, public AccessibilityProps { ViewProps( const PropsParserContext &context, ViewProps const &sourceProps, - RawProps const &rawProps); + RawProps const &rawProps, + bool shouldSetRawProps = true); #pragma mark - Props diff --git a/ReactCommon/react/renderer/components/view/ViewPropsInterpolation.h b/ReactCommon/react/renderer/components/view/ViewPropsInterpolation.h index 0d0665f61c..50957aad8f 100644 --- a/ReactCommon/react/renderer/components/view/ViewPropsInterpolation.h +++ b/ReactCommon/react/renderer/components/view/ViewPropsInterpolation.h @@ -44,10 +44,12 @@ static inline void interpolateViewProps( // mounting layer. Once we can remove this, we should change `rawProps` to // be const again. #ifdef ANDROID - interpolatedProps->rawProps["opacity"] = interpolatedProps->opacity; + if (!interpolatedProps->rawProps.isNull()) { + interpolatedProps->rawProps["opacity"] = interpolatedProps->opacity; - interpolatedProps->rawProps["transform"] = - (folly::dynamic)interpolatedProps->transform; + interpolatedProps->rawProps["transform"] = + (folly::dynamic)interpolatedProps->transform; + } #endif } diff --git a/ReactCommon/react/renderer/components/view/ViewShadowNode.cpp b/ReactCommon/react/renderer/components/view/ViewShadowNode.cpp index b23aac9d51..9ab95c3a3f 100644 --- a/ReactCommon/react/renderer/components/view/ViewShadowNode.cpp +++ b/ReactCommon/react/renderer/components/view/ViewShadowNode.cpp @@ -6,6 +6,7 @@ */ #include "ViewShadowNode.h" +#include #include namespace facebook { @@ -13,6 +14,37 @@ namespace react { char const ViewComponentName[] = "View"; +static inline bool keepRawValuesInViewProps(PropsParserContext const &context) { + static bool shouldUseRawProps = true; + +#ifdef ANDROID + static bool initialized = false; + + if (!initialized) { + auto config = + context.contextContainer.find>( + "ReactNativeConfig"); + if (config.has_value()) { + initialized = true; + shouldUseRawProps = !config.value()->getBool( + "react_native_new_architecture:use_mapbuffer_for_viewprops"); + } + } +#endif + + return shouldUseRawProps; +} + +ViewShadowNodeProps::ViewShadowNodeProps( + PropsParserContext const &context, + ViewShadowNodeProps const &sourceProps, + RawProps const &rawProps) + : ViewProps( + context, + sourceProps, + rawProps, + keepRawValuesInViewProps(context)){}; + ViewShadowNode::ViewShadowNode( ShadowNodeFragment const &fragment, ShadowNodeFamily::Shared const &family, diff --git a/ReactCommon/react/renderer/components/view/ViewShadowNode.h b/ReactCommon/react/renderer/components/view/ViewShadowNode.h index 0264a7ab41..d62f63ed6b 100644 --- a/ReactCommon/react/renderer/components/view/ViewShadowNode.h +++ b/ReactCommon/react/renderer/components/view/ViewShadowNode.h @@ -15,12 +15,24 @@ namespace react { extern const char ViewComponentName[]; +/** + * Implementation of the ViewProps that propagates feature flag. + */ +class ViewShadowNodeProps final : public ViewProps { + public: + ViewShadowNodeProps() = default; + ViewShadowNodeProps( + const PropsParserContext &context, + ViewShadowNodeProps const &sourceProps, + RawProps const &rawProps); +}; + /* * `ShadowNode` for component. */ class ViewShadowNode final : public ConcreteViewShadowNode< ViewComponentName, - ViewProps, + ViewShadowNodeProps, ViewEventEmitter> { public: static ShadowNodeTraits BaseTraits() { diff --git a/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp b/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp index 5b2d5be6ba..2a5023f2b3 100644 --- a/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp +++ b/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp @@ -22,8 +22,9 @@ namespace react { YogaStylableProps::YogaStylableProps( const PropsParserContext &context, YogaStylableProps const &sourceProps, - RawProps const &rawProps) - : Props(context, sourceProps, rawProps), + RawProps const &rawProps, + bool shouldSetRawProps) + : Props(context, sourceProps, rawProps, shouldSetRawProps), yogaStyle(convertRawProp(context, rawProps, sourceProps.yogaStyle)){}; #pragma mark - DebugStringConvertible diff --git a/ReactCommon/react/renderer/components/view/YogaStylableProps.h b/ReactCommon/react/renderer/components/view/YogaStylableProps.h index 07d3d3fc84..b8f6118cd2 100644 --- a/ReactCommon/react/renderer/components/view/YogaStylableProps.h +++ b/ReactCommon/react/renderer/components/view/YogaStylableProps.h @@ -22,7 +22,8 @@ class YogaStylableProps : public Props { YogaStylableProps( const PropsParserContext &context, YogaStylableProps const &sourceProps, - RawProps const &rawProps); + RawProps const &rawProps, + bool shouldSetRawProps = true); #pragma mark - Props diff --git a/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp b/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp index f9890c32ff..6d907a3dcb 100644 --- a/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp +++ b/ReactCommon/react/renderer/components/view/tests/LayoutTest.cpp @@ -74,7 +74,7 @@ class LayoutTest : public ::testing::Test { Element() .reference(viewShadowNodeA_) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; yogaStyle.positionType() = YGPositionTypeAbsolute; @@ -86,7 +86,7 @@ class LayoutTest : public ::testing::Test { Element() .reference(viewShadowNodeAB_) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; yogaStyle.positionType() = YGPositionTypeAbsolute; @@ -100,7 +100,7 @@ class LayoutTest : public ::testing::Test { Element() .reference(viewShadowNodeABC_) .props([=] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; @@ -119,7 +119,7 @@ class LayoutTest : public ::testing::Test { Element() .reference(viewShadowNodeABCD_) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; yogaStyle.positionType() = YGPositionTypeAbsolute; @@ -133,7 +133,7 @@ class LayoutTest : public ::testing::Test { Element() .reference(viewShadowNodeABE_) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); auto &props = *sharedProps; auto &yogaStyle = props.yogaStyle; yogaStyle.positionType() = YGPositionTypeAbsolute; diff --git a/ReactCommon/react/renderer/components/view/tests/ViewTest.cpp b/ReactCommon/react/renderer/components/view/tests/ViewTest.cpp index 05b9008b8e..67c7eda979 100644 --- a/ReactCommon/react/renderer/components/view/tests/ViewTest.cpp +++ b/ReactCommon/react/renderer/components/view/tests/ViewTest.cpp @@ -49,7 +49,7 @@ class YogaDirtyFlagTest : public ::testing::Test { /* * Some non-default props. */ - auto mutableViewProps = std::make_shared(); + auto mutableViewProps = std::make_shared(); auto &props = *mutableViewProps; props.nativeId = "native Id"; props.opacity = 0.5; @@ -111,7 +111,7 @@ TEST_F(YogaDirtyFlagTest, changingNonLayoutSubPropsMustNotDirtyYogaNode) { */ auto newRootShadowNode = rootShadowNode_->cloneTree( innerShadowNode_->getFamily(), [](ShadowNode const &oldShadowNode) { - auto viewProps = std::make_shared(); + auto viewProps = std::make_shared(); auto &props = *viewProps; props.nativeId = "some new native Id"; @@ -135,7 +135,7 @@ TEST_F(YogaDirtyFlagTest, changingLayoutSubPropsMustDirtyYogaNode) { */ auto newRootShadowNode = rootShadowNode_->cloneTree( innerShadowNode_->getFamily(), [](ShadowNode const &oldShadowNode) { - auto viewProps = std::make_shared(); + auto viewProps = std::make_shared(); auto &props = *viewProps; props.yogaStyle.alignContent() = YGAlignBaseline; diff --git a/ReactCommon/react/renderer/core/Props.cpp b/ReactCommon/react/renderer/core/Props.cpp index 81e316ff84..d1b5ff7e15 100644 --- a/ReactCommon/react/renderer/core/Props.cpp +++ b/ReactCommon/react/renderer/core/Props.cpp @@ -16,7 +16,8 @@ namespace react { Props::Props( const PropsParserContext &context, const Props &sourceProps, - const RawProps &rawProps) + const RawProps &rawProps, + const bool shouldSetRawProps) : nativeId(convertRawProp( context, rawProps, @@ -26,7 +27,9 @@ Props::Props( revision(sourceProps.revision + 1) #ifdef ANDROID , - rawProps((folly::dynamic)rawProps) + rawProps( + shouldSetRawProps ? (folly::dynamic)rawProps + : /* null */ folly::dynamic()) #endif {}; diff --git a/ReactCommon/react/renderer/core/Props.h b/ReactCommon/react/renderer/core/Props.h index 56e9187941..987c4228a7 100644 --- a/ReactCommon/react/renderer/core/Props.h +++ b/ReactCommon/react/renderer/core/Props.h @@ -33,7 +33,8 @@ class Props : public virtual Sealable, public virtual DebugStringConvertible { Props( const PropsParserContext &context, const Props &sourceProps, - RawProps const &rawProps); + RawProps const &rawProps, + bool shouldSetRawProps = true); virtual ~Props() = default; std::string nativeId; diff --git a/ReactCommon/react/renderer/core/tests/FindNodeAtPointTest.cpp b/ReactCommon/react/renderer/core/tests/FindNodeAtPointTest.cpp index 6a9effc6e5..dc53b11ab0 100644 --- a/ReactCommon/react/renderer/core/tests/FindNodeAtPointTest.cpp +++ b/ReactCommon/react/renderer/core/tests/FindNodeAtPointTest.cpp @@ -45,9 +45,9 @@ TEST(FindNodeAtPointTest, withoutTransform) { }) }) }); - + auto parentShadowNode = builder.build(element); - + EXPECT_EQ( LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {115, 115})->getTag(), 3); EXPECT_EQ(LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {105, 105})->getTag(), 2); @@ -91,7 +91,7 @@ TEST(FindNodeAtPointTest, viewIsTranslated) { }) }) }); - + auto parentShadowNode = builder.build(element); EXPECT_EQ( @@ -125,7 +125,7 @@ TEST(FindNodeAtPointTest, viewIsScaled) { Element() .tag(3) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); sharedProps->transform = Transform::Scale(0.5, 0.5, 0); return sharedProps; }) @@ -137,7 +137,7 @@ TEST(FindNodeAtPointTest, viewIsScaled) { }) }) }); - + auto parentShadowNode = builder.build(element); EXPECT_EQ( @@ -175,9 +175,9 @@ TEST(FindNodeAtPointTest, overlappingViews) { shadowNode.setLayoutMetrics(layoutMetrics); }) }); - + auto parentShadowNode = builder.build(element); - + EXPECT_EQ( LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {50, 50})->getTag(), 3); } @@ -198,7 +198,7 @@ TEST(FindNodeAtPointTest, overlappingViewsWithZIndex) { Element() .tag(2) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); sharedProps->zIndex = 1; auto &yogaStyle = sharedProps->yogaStyle; yogaStyle.positionType() = YGPositionTypeAbsolute; @@ -219,11 +219,9 @@ TEST(FindNodeAtPointTest, overlappingViewsWithZIndex) { shadowNode.setLayoutMetrics(layoutMetrics); }) }); - + auto parentShadowNode = builder.build(element); - + EXPECT_EQ( LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {50, 50})->getTag(), 2); } - - diff --git a/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp b/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp index bf98fb317e..55c761c954 100644 --- a/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp +++ b/ReactCommon/react/renderer/core/tests/LayoutableShadowNodeTest.cpp @@ -148,7 +148,7 @@ TEST(LayoutableShadowNodeTest, relativeLayoutMetricsOnTransformedNode) { shadowNode.setLayoutMetrics(layoutMetrics); }) .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); sharedProps->transform = Transform::Scale(0.5, 0.5, 1); return sharedProps; }) @@ -199,7 +199,7 @@ TEST(LayoutableShadowNodeTest, relativeLayoutMetricsOnTransformedParent) { .children({ Element() .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); sharedProps->transform = Transform::Scale(0.5, 0.5, 1); return sharedProps; }) @@ -280,7 +280,7 @@ TEST(LayoutableShadowNodeTest, relativeLayoutMetricsOnSameTransformedNode) { auto element = Element() .props([] { - auto sharedProps = std::make_shared(); + auto sharedProps = std::make_shared(); sharedProps->transform = Transform::Scale(2, 2, 1); return sharedProps; }) diff --git a/ReactCommon/react/renderer/element/tests/ElementTest.cpp b/ReactCommon/react/renderer/element/tests/ElementTest.cpp index 519047b8e1..85860fcc2d 100644 --- a/ReactCommon/react/renderer/element/tests/ElementTest.cpp +++ b/ReactCommon/react/renderer/element/tests/ElementTest.cpp @@ -26,7 +26,7 @@ TEST(ElementTest, testNormalCases) { auto shadowNodeAB = std::shared_ptr{}; auto shadowNodeABA = std::shared_ptr{}; - auto propsAA = std::make_shared(); + auto propsAA = std::make_shared(); propsAA->nativeId = "node AA"; // clang-format off @@ -51,7 +51,7 @@ TEST(ElementTest, testNormalCases) { .reference(shadowNodeAB) .tag(3) .props([]() { - auto props = std::make_shared(); + auto props = std::make_shared(); props->nativeId = "node AB"; return props; }) @@ -60,7 +60,7 @@ TEST(ElementTest, testNormalCases) { .reference(shadowNodeABA) .tag(4) .props([]() { - auto props = std::make_shared(); + auto props = std::make_shared(); props->nativeId = "node ABA"; return props; }) diff --git a/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp b/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp index dc315be427..6d2cc0a190 100644 --- a/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp +++ b/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp @@ -158,7 +158,7 @@ class StackingContextTest : public ::testing::Test { rootShadowNode_ = std::static_pointer_cast(rootShadowNode_->cloneTree( node->getFamily(), [&](ShadowNode const &oldShadowNode) { - auto viewProps = std::make_shared(); + auto viewProps = std::make_shared(); callback(*viewProps); return oldShadowNode.clone(ShadowNodeFragment{viewProps}); }));