Drop per-field enableCppPropsIteratorSetter ternaries from Props ctors#57418
Open
javache wants to merge 3 commits into
Open
Drop per-field enableCppPropsIteratorSetter ternaries from Props ctors#57418javache wants to merge 3 commits into
enableCppPropsIteratorSetter ternaries from Props ctors#57418javache wants to merge 3 commits into
Conversation
…:forEachItem` (react#57328) Summary: Pull Request resolved: react#57328 Today the iterator-setter path in `ConcreteComponentDescriptor::cloneProps` runs three sequential walks over the input — `RawProps::parse(parser)` (builds `keyIndexToValueIndex_` for `convertRawProp`), `static_cast<folly::dynamic>(rawProps)` (materializes a `folly::dynamic` via `jsi::dynamicFromValue` in JSI mode), and then `dynamic.items()` to dispatch `setProp`. Only the third is actually used: `convertRawProp` is never called on the iterator-setter branch, and the `folly::dynamic` materialization exists only as iteration scaffolding. Restructure so the runtime flag picks one of two construction paths up front: - **Iterator-setter** — copy-construct from `sourceProps` via the (re-enabled) `Props` copy ctor, then walk `rawProps` in-place via the new `RawProps::forEachItem` helper and route each entry through `setProp`. `parse()` is skipped entirely; the `folly::dynamic` materialization is skipped in `Mode::JSI`. - **Classic** — unchanged: `parse()` + 3-arg `convertRawProp`-driven ctor. `forEachItem` switches on `RawProps::Mode`: - `Mode::JSI` — walks `value_.asObject(*runtime_).getPropertyNames(...)` and constructs `RawValue` from each `jsi::Value` directly, no `folly::dynamic` in between. - `Mode::Dynamic` — iterates `dynamic_.items()` (same as today). - `Mode::Empty` — no-op. A new `HasIteratorSetterCtor<T>` concept (`std::copy_constructible<T>`) documents the contract and feeds a `static_assert` in `cloneProps`, so a future Props type that deletes its copy ctor fails at compile time rather than silently diverging at runtime between the two flag states. The `RN_SERIALIZABLE_STATE` Props 2.0 accumulation branch keeps its existing dynamic-iteration shape — when `fallbackToDynamicRawPropsAccumulation` is true, `initializeDynamicProps` has already merged the source's rawProps with the input onto `shadowNodeProps->rawProps`, so we iterate that merged dynamic rather than the raw input. The per-field `flag ? sourceProps.X : convertRawProp(...)` ternaries across every Props .cpp file become dead in the flag-on path (the copy ctor handles those fields) but are still functional in the flag-off path. They get removed in a follow-up cleanup; this diff is structurally non-breaking on either flag state. Changelog: [Internal] Differential Revision: D109568749
…active (react#57329) Summary: Pull Request resolved: react#57329 `RawPropsParser::prepare<ConcreteProps>()` runs in `ConcreteComponentDescriptor`'s constructor and builds the parser's `keys_` vector + `nameToIndex_` length-bucketed map by walking every `convertRawProp` call in a probe construction of `ConcreteProps`. The cost is O(n²) in the number of props (the comment in `RawPropsParser::at` notes 4950 lookups for a 100-prop class) and is paid once per component class at app startup. Those data structures are consumed **only** by `RawProps::at()`, which is reached exclusively through the classic per-field `convertRawProp` path. The iterator-setter path skips `parse()` entirely (see the prior diff in the stack), so the prepared parser is dead weight when both: 1. `HasIteratorSetterCtor<ConcreteProps>` is satisfied (i.e. the type opts into the iterator-setter path), AND 2. `enableCppPropsIteratorSetter()` is on at runtime. Guard the call accordingly. Classes that don't satisfy the concept always run `prepare<T>()` (they can only use the classic path); when the runtime flag is off, all classes run it (since they all fall back to classic). Changelog: [Internal] Differential Revision: D109569571 Reviewed By: christophpurrer, lenaic
Summary:
With `ConcreteComponentDescriptor::cloneProps` now dispatching to the iterator-setter path via the copy ctor (the prior diffs in the stack), the per-field `flag ? sourceProps.X : convertRawProp(...)` ternaries scattered across every Props copy ctor are dead in the flag-on path (copy ctor handles those fields directly) and redundant in the flag-off path (the runtime check inside the ctor is always false, since cloneProps already filtered the iterator-setter path out before invoking the 3-arg ctor).
Strip the ternary wrapper from every Props .cpp file. Each initializer collapses from
```cpp
field(ReactNativeFeatureFlags::enableCppPropsIteratorSetter()
? sourceProps.field
: convertRawProp(ctx, rawProps, "field", sourceProps.field, default))
```
to just
```cpp
field(convertRawProp(ctx, rawProps, "field", sourceProps.field, default))
```
Also strip:
- `YogaStylableProps`: the inverted `if (!flag) { convertRawPropAliases(...); }` wrapper becomes unconditional.
- `Props::initialize` (`Props.cpp`): the `nativeId` ternary collapses to the `convertRawProp` form.
- `AccessibilityProps` (xplat + react-native-macos): the `if (flag) { copy } else { at()-based resolution }` block in the constructor body collapses to just the `at()`-based resolution — the same reasoning applies (flag-on path never reaches the 3-arg ctor).
- Drop `<react/featureflags/ReactNativeFeatureFlags.h>` includes that are no longer referenced.
311 ternaries removed across 20 Props .cpp files (incl. the `third-party/react-native-macos/` mirror and `xplat/instagram/airwave/components/airwavescrollview/cpp/AirwaveScrollViewProps.cpp`).
Side benefit: the classic path is now meaningfully cheaper because we no longer do `~30 × ReactNativeFeatureFlags::enableCppPropsIteratorSetter()` calls (each goes through `std::atomic<std::optional<bool>>::load()`) per `cloneProps` invocation per Props class.
Changelog:
[Internal]
Differential Revision: D109583086
|
@javache has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109583086. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
With
ConcreteComponentDescriptor::clonePropsnow dispatching to the iterator-setter path via the copy ctor (the prior diffs in the stack), the per-fieldflag ? sourceProps.X : convertRawProp(...)ternaries scattered across every Props copy ctor are dead in the flag-on path (copy ctor handles those fields directly) and redundant in the flag-off path (the runtime check inside the ctor is always false, since cloneProps already filtered the iterator-setter path out before invoking the 3-arg ctor).Strip the ternary wrapper from every Props .cpp file. Each initializer collapses from
to just
Also strip:
YogaStylableProps: the invertedif (!flag) { convertRawPropAliases(...); }wrapper becomes unconditional.Props::initialize(Props.cpp): thenativeIdternary collapses to theconvertRawPropform.AccessibilityProps(xplat + react-native-macos): theif (flag) { copy } else { at()-based resolution }block in the constructor body collapses to just theat()-based resolution — the same reasoning applies (flag-on path never reaches the 3-arg ctor).<react/featureflags/ReactNativeFeatureFlags.h>includes that are no longer referenced.311 ternaries removed across 20 Props .cpp files (incl. the
third-party/react-native-macos/mirror andxplat/instagram/airwave/components/airwavescrollview/cpp/AirwaveScrollViewProps.cpp).Side benefit: the classic path is now meaningfully cheaper because we no longer do
~30 × ReactNativeFeatureFlags::enableCppPropsIteratorSetter()calls (each goes throughstd::atomic<std::optional<bool>>::load()) perclonePropsinvocation per Props class.Changelog:
[Internal]
Differential Revision: D109583086