filter out yoga style props during folly::dynamic conversion (#41607)

Summary: Pull Request resolved: #41607 X-link: facebook/hermes#1198 changelog: [internal] Reviewed By: javache Differential Revision: D51471665 fbshipit-source-id: 0633422d356c2f2419ea87b2a2ad6b5ea64c99f6
facebook · Nov 24, 2023 · 67c852e · 67c852e
1 parent 0707bbc
commit 67c852e
Show file tree

Hide file tree

Showing 7 changed files with 169 additions and 125 deletions.
diff --git a/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.cpp b/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.cpp
@@ -150,7 +150,10 @@ void dynamicFromValueShallow(
 
 } // namespace
 
-folly::dynamic dynamicFromValue(Runtime& runtime, const Value& valueInput) {
+folly::dynamic dynamicFromValue(
+    Runtime& runtime,
+    const Value& valueInput,
+    std::function<bool(const std::string&)> filterObjectKeys) {
   std::vector<FromValue> stack;
   folly::dynamic ret;
 
@@ -182,13 +185,17 @@ folly::dynamic dynamicFromValue(Runtime& runtime, const Value& valueInput) {
         if (prop.isUndefined()) {
           continue;
         }
+        auto nameStr = name.utf8(runtime);
+        if (filterObjectKeys && filterObjectKeys(nameStr)) {
+          continue;
+        }
         // The JSC conversion uses JSON.stringify, which substitutes
         // null for a function, so we do the same here.  Just dropping
         // the pair might also work, but would require more testing.
         if (prop.isObject() && prop.getObject(runtime).isFunction(runtime)) {
           prop = Value::null();
         }
-        props.emplace_back(name.utf8(runtime), std::move(prop));
+        props.emplace_back(std::move(nameStr), std::move(prop));
         top.dyn->insert(props.back().first, nullptr);
       }
       for (const auto& prop : props) {

diff --git a/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h b/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h
@@ -19,7 +19,8 @@ facebook::jsi::Value valueFromDynamic(
 
 folly::dynamic dynamicFromValue(
     facebook::jsi::Runtime& runtime,
-    const facebook::jsi::Value& value);
+    const facebook::jsi::Value& value,
+    std::function<bool(const std::string&)> filterObjectKeys = nullptr);
 
 } // namespace jsi
 } // namespace facebook
diff --git a/packages/react-native/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp b/packages/react-native/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp
@@ -19,134 +19,19 @@
 
 namespace facebook::react {
 
-namespace {
-inline RawProps filterYogaProps(const RawProps& rawProps) {
-  const static std::unordered_set<std::string> yogaStylePropNames = {
-      {"direction",
-       "flexDirection",
-       "justifyContent",
-       "alignContent",
-       "alignItems",
-       "alignSelf",
-       "position",
-       "flexWrap",
-       "display",
-       "flex",
-       "flexGrow",
-       "flexShrink",
-       "flexBasis",
-       "margin",
-       "padding",
-       "rowGap",
-       "columnGap",
-       "gap",
-       // TODO: T163711275 also filter out width/height when SVG no longer read
-       // them from RawProps
-       "minWidth",
-       "maxWidth",
-       "minHeight",
-       "maxHeight",
-       "aspectRatio",
-
-       // edges
-       "left",
-       "right",
-       "top",
-       "bottom",
-       "start",
-       "end",
-
-       // variants of inset
-       "inset",
-       "insetStart",
-       "insetEnd",
-       "insetInline",
-       "insetInlineStart",
-       "insetInlineEnd",
-       "insetBlock",
-       "insetBlockEnd",
-       "insetBlockStart",
-       "insetVertical",
-       "insetHorizontal",
-       "insetTop",
-       "insetBottom",
-       "insetLeft",
-       "insetRight",
-
-       // variants of margin
-       "marginStart",
-       "marginEnd",
-       "marginInline",
-       "marginInlineStart",
-       "marginInlineEnd",
-       "marginBlock",
-       "marginBlockStart",
-       "marginBlockEnd",
-       "marginVertical",
-       "marginHorizontal",
-       "marginTop",
-       "marginBottom",
-       "marginLeft",
-       "marginRight",
-
-       // variants of padding
-       "paddingStart",
-       "paddingEnd",
-       "paddingInline",
-       "paddingInlineStart",
-       "paddingInlineEnd",
-       "paddingBlock",
-       "paddingBlockStart",
-       "paddingBlockEnd",
-       "paddingVertical",
-       "paddingHorizontal",
-       "paddingTop",
-       "paddingBottom",
-       "paddingLeft",
-       "paddingRight"}};
-
-  auto filteredRawProps = (folly::dynamic)rawProps;
-
-  auto it = filteredRawProps.items().begin();
-  while (it != filteredRawProps.items().end()) {
-    auto key = it->first.asString();
-    if (yogaStylePropNames.find(key) != yogaStylePropNames.end()) {
-      it = filteredRawProps.erase(it);
-    } else {
-      ++it;
-    }
-  }
-
-  return RawProps(std::move(filteredRawProps));
-}
-} // namespace
-
 YogaStylableProps::YogaStylableProps(
     const PropsParserContext& context,
     const YogaStylableProps& sourceProps,
     const RawProps& rawProps)
     : Props() {
-  if (CoreFeatures::excludeYogaFromRawProps) {
-    const auto filteredRawProps = filterYogaProps(rawProps);
-    initialize(context, sourceProps, filteredRawProps);
-
-    yogaStyle = CoreFeatures::enablePropIteratorSetter
-        ? sourceProps.yogaStyle
-        : convertRawProp(context, filteredRawProps, sourceProps.yogaStyle);
-
-    if (!CoreFeatures::enablePropIteratorSetter) {
-      convertRawPropAliases(context, sourceProps, filteredRawProps);
-    }
-  } else {
-    initialize(context, sourceProps, rawProps);
+  initialize(context, sourceProps, rawProps);
 
-    yogaStyle = CoreFeatures::enablePropIteratorSetter
-        ? sourceProps.yogaStyle
-        : convertRawProp(context, rawProps, sourceProps.yogaStyle);
+  yogaStyle = CoreFeatures::enablePropIteratorSetter
+      ? sourceProps.yogaStyle
+      : convertRawProp(context, rawProps, sourceProps.yogaStyle);
 
-    if (!CoreFeatures::enablePropIteratorSetter) {
-      convertRawPropAliases(context, sourceProps, rawProps);
-    }
+  if (!CoreFeatures::enablePropIteratorSetter) {
+    convertRawPropAliases(context, sourceProps, rawProps);
   }
 };
 

diff --git a/packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h b/packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h
@@ -105,6 +105,13 @@ class ConcreteComponentDescriptor : public ComponentDescriptor {
       return ShadowNodeT::defaultSharedProps();
     }
 
+    if (CoreFeatures::excludeYogaFromRawProps) {
+      if (ShadowNodeT::IdentifierTrait() ==
+          ShadowNodeTraits::Trait::YogaLayoutableKind) {
+        rawProps.filterYogaStylePropsInDynamicConversion();
+      }
+    }
+
     rawProps.parse(rawPropsParser_);
 
     // Call old-style constructor

diff --git a/packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp b/packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp
@@ -13,6 +13,96 @@
 
 namespace facebook::react {
 
+namespace {
+inline bool isYogaStyleProp(const std::string& prop) {
+  const static std::unordered_set<std::string> yogaStylePropNames = {
+      {"direction",
+       "flexDirection",
+       "justifyContent",
+       "alignContent",
+       "alignItems",
+       "alignSelf",
+       "position",
+       "flexWrap",
+       "display",
+       "flex",
+       "flexGrow",
+       "flexShrink",
+       "flexBasis",
+       "margin",
+       "padding",
+       "rowGap",
+       "columnGap",
+       "gap",
+       // TODO: T163711275 also filter out width/height when SVG no longer read
+       // them from RawProps
+       "minWidth",
+       "maxWidth",
+       "minHeight",
+       "maxHeight",
+       "aspectRatio",
+
+       // edges
+       "left",
+       "right",
+       "top",
+       "bottom",
+       "start",
+       "end",
+
+       // variants of inset
+       "inset",
+       "insetStart",
+       "insetEnd",
+       "insetInline",
+       "insetInlineStart",
+       "insetInlineEnd",
+       "insetBlock",
+       "insetBlockEnd",
+       "insetBlockStart",
+       "insetVertical",
+       "insetHorizontal",
+       "insetTop",
+       "insetBottom",
+       "insetLeft",
+       "insetRight",
+
+       // variants of margin
+       "marginStart",
+       "marginEnd",
+       "marginInline",
+       "marginInlineStart",
+       "marginInlineEnd",
+       "marginBlock",
+       "marginBlockStart",
+       "marginBlockEnd",
+       "marginVertical",
+       "marginHorizontal",
+       "marginTop",
+       "marginBottom",
+       "marginLeft",
+       "marginRight",
+
+       // variants of padding
+       "paddingStart",
+       "paddingEnd",
+       "paddingInline",
+       "paddingInlineStart",
+       "paddingInlineEnd",
+       "paddingBlock",
+       "paddingBlockStart",
+       "paddingBlockEnd",
+       "paddingVertical",
+       "paddingHorizontal",
+       "paddingTop",
+       "paddingBottom",
+       "paddingLeft",
+       "paddingRight"}};
+
+  return yogaStylePropNames.find(prop) != yogaStylePropNames.end();
+}
+} // namespace
+
 RawProps::RawProps() {
   mode_ = Mode::Empty;
 }
@@ -55,6 +145,7 @@ RawProps::RawProps(const RawProps& other) noexcept {
   } else if (mode_ == Mode::Dynamic) {
     dynamic_ = other.dynamic_;
   }
+  ignoreYogaStyleProps_ = other.ignoreYogaStyleProps_;
 }
 
 RawProps& RawProps::operator=(const RawProps& other) noexcept {
@@ -65,6 +156,7 @@ RawProps& RawProps::operator=(const RawProps& other) noexcept {
   } else if (mode_ == Mode::Dynamic) {
     dynamic_ = other.dynamic_;
   }
+  ignoreYogaStyleProps_ = other.ignoreYogaStyleProps_;
   return *this;
 }
 
@@ -84,12 +176,17 @@ RawProps::operator folly::dynamic() const noexcept {
     case Mode::Empty:
       return folly::dynamic::object();
     case Mode::JSI:
-      return jsi::dynamicFromValue(*runtime_, value_);
+      return jsi::dynamicFromValue(
+          *runtime_, value_, ignoreYogaStyleProps_ ? isYogaStyleProp : nullptr);
     case Mode::Dynamic:
       return dynamic_;
   }
 }
 
+void RawProps::filterYogaStylePropsInDynamicConversion() noexcept {
+  ignoreYogaStyleProps_ = true;
+}
+
 /*
  * Returns `true` if the object is empty.
  * Empty `RawProps` does not have any stored data.

diff --git a/packages/react-native/ReactCommon/react/renderer/core/RawProps.h b/packages/react-native/ReactCommon/react/renderer/core/RawProps.h
@@ -73,6 +73,15 @@ class RawProps final {
    */
   explicit operator folly::dynamic() const noexcept;
 
+  /*
+   * Once called, Yoga style props will be filtered out during conversion to
+   * folly::dynamic. folly::dynamic conversion is only used on Android and props
+   * specific to Yoga do not need to be send over JNI to Android.
+   * This is a performance optimisation to minimise traffic between C++ and
+   * Java.
+   */
+  void filterYogaStylePropsInDynamicConversion() noexcept;
+
   /*
    * Returns `true` if the object is empty.
    * Empty `RawProps` does not have any stored data.
@@ -124,6 +133,8 @@ class RawProps final {
    */
   mutable std::vector<RawPropsValueIndex> keyIndexToValueIndex_;
   mutable std::vector<RawValue> values_;
+
+  bool ignoreYogaStyleProps_{false};
 };
 
 } // namespace facebook::react
diff --git a/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp b/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp
@@ -467,3 +467,39 @@ TEST(RawPropsTest, copyJSIRawProps) {
   EXPECT_NEAR(
       copyProps->derivedFloatValue, originalProps->derivedFloatValue, 0.00001);
 }
+
+TEST(RawPropsTest, filterYogaRawProps) {
+  auto runtime = facebook::hermes::makeHermesRuntime();
+
+  ContextContainer contextContainer{};
+  PropsParserContext parserContext{-1, contextContainer};
+
+  auto object = jsi::Object(*runtime);
+  object.setProperty(*runtime, "floatValue", 10.0);
+  object.setProperty(*runtime, "flex", 1);
+
+  auto rawProps = RawProps(*runtime, jsi::Value(*runtime, object));
+
+  EXPECT_FALSE(rawProps.isEmpty());
+
+  auto dynamicProps = (folly::dynamic)rawProps;
+
+  EXPECT_EQ(dynamicProps["floatValue"], 10.0);
+  EXPECT_EQ(dynamicProps["flex"], 1);
+
+  rawProps.filterYogaStylePropsInDynamicConversion();
+
+  dynamicProps = (folly::dynamic)rawProps;
+
+  EXPECT_EQ(dynamicProps["floatValue"], 10.0);
+  EXPECT_EQ(dynamicProps["flex"], nullptr);
+
+  // The fact that filterYogaStylePropsInDynamicConversion should
+  // must apply to a copy as well.
+  auto copy = RawProps(rawProps);
+
+  auto dynamicPropsFromCopy = (folly::dynamic)copy;
+
+  EXPECT_EQ(dynamicPropsFromCopy["floatValue"], 10.0);
+  EXPECT_EQ(dynamicPropsFromCopy["flex"], nullptr);
+}