Repository navigation
Apply setNativeProps props to a cloned node only when they changed - #59624
hryhoriiK97 wants to merge 5 commits into
Conversation
Once a component's props were set with setNativeProps, every clone of its shadow nodes with unchanged props parsed nativeProps_DEPRECATED into new props again, including the clones Yoga makes of every child of a node it lays out. New props mark a Text's Yoga node dirty, so with JS-driven Animated (which uses setNativeProps on Fabric) every sibling Text was measured again in every frame. The family now counts changes to nativeProps_DEPRECATED, and each node records the count its props include. A clone applies nativeProps_DEPRECATED only when the node it clones is behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@javache could you take a look at this one when you have time? It stops re-parsing |
javache
left a comment
There was a problem hiding this comment.
Not a huge fan of this approach and doing the bookkeeping across shadownode and shadownodefamily. Could we reuse the existing ShadowNode revision_ metadata for this?
| * Incremented whenever `nativeProps_DEPRECATED` changes, so a cloned node | ||
| * can tell whether its props already include them. | ||
| */ | ||
| mutable uint32_t nativePropsVersion_DEPRECATED{0}; |
There was a problem hiding this comment.
Instead of adding a new field here, should we make nativeProps_DEPRECATED a ptr to a struct with the props field and the version number?
There was a problem hiding this comment.
@javache thanks for the review! I looked at revision_, but it only exists in debug builds, and it counts clones, not props changes, so it can't tell if a node's props already include the native props.
So I went with your struct idea: nativeProps_DEPRECATED now keeps the native props together with the props object they were last applied to. All the bookkeeping is in the family now, and ShadowNode has no new field. Done in 2e01b7b, could you take another look?
I measured it again on Android and the result is the same as before: about 87% fewer text measure calls.
Review feedback: instead of a version counter on both ShadowNodeFamily and ShadowNode, `nativeProps_DEPRECATED` is now a pointer to a struct with the props and the props object they were last applied to. `ShadowNode::clone` skips parsing them when the node's `props_` is that object, so ShadowNode no longer needs a field of its own. `setNativeProps_DEPRECATED` and `UIManager::cloneNode` replace the struct, so the next clone applies the new native props. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| *this, | ||
| {.props = componentDescriptor.cloneProps( | ||
| propsParserContext, props_, std::move(*propsOverride)), | ||
| {.props = props, |
There was a problem hiding this comment.
| {.props = props, | |
| {.props = std::move(props), |
| auto props = componentDescriptor.cloneProps( | ||
| propsParserContext, props_, RawProps(nativeProps->props)); | ||
| { | ||
| std::lock_guard<std::mutex> lock(family.nativePropsMutex); |
There was a problem hiding this comment.
Can we make this a single block / lock? Could all of this logic live inside of if (family.nativeProps_DEPRECATED != nullptr)?
The risk is that we'd hold the lock during the cloneProps call, is that a concern?
There was a problem hiding this comment.
@javache done in 82c4383. I don't think holding the lock during cloneProps is a problem: the mutex is per family, so it only blocks another thread working on the same node at the same time, and cloneProps never takes this lock again. UIManager::cloneNode already does mergeDynamicProps under it too. I kept cloneShadowNode outside the lock, though.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Visual regressionNo visual differences at |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The |
|
@javache has imported this pull request. If you are a Meta employee, you can view this in D131796458. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary:
Once a component's props are set with
setNativeProps,ShadowNode::cloneappliesfamily.nativeProps_DEPRECATEDagain every time one of its shadow nodes is cloned with unchanged props: it parses them into new props withcloneProps. That includes the clones Yoga makes of every child of a node it lays out (YogaLayoutableShadowNode::adoptYogaChild), so each commit that touches a parent re-parses the native props of all its children that ever hadsetNativeProps, andnativeProps_DEPRECATEDstays set for the life of the component.On Fabric, JS-driven
Animated(useNativeDriver: false) updates props withsetNativePropson every frame (shouldUseSetNativePropsInFabric, on by default). With several animated components under one parent, every frame re-parses the props of all of them, not just the one being updated. For aText, new props also mark its Yoga node dirty (ParagraphShadowNode::shouldNewRevisionDirtyMeasurement), so all of them are measured again. In a list of 30 JS-animated texts, on Android that was ~16,000measurePreparedLayoutcalls per second (~40 per frame), and on iOS props parsing was 27–28% of the JS thread.With this change,
ShadowNodeFamilycounts changes tonativeProps_DEPRECATED(setNativeProps_DEPRECATED, andUIManager::cloneNodewhen a React render updates them), and each shadow node records the count its props include. A clone with unchanged props keeps the source node's count;ShadowNode::cloneappliesnativeProps_DEPRECATEDonly when the node it clones is behind, exactly as before in every other case. So native props are still applied aftersetNativePropsand after a React render replaces a node's props; they are only no longer parsed again into props that already include them.Changelog:
[GENERAL] [CHANGED] - Fabric no longer parses the props set by setNativeProps again on every clone of a component's shadow nodes
Test Plan:
FabricUIManagerTestcases: aftersetNativeProps_DEPRECATED, a clone of a node whose props include them keeps the same props object, and a latersetNativeProps_DEPRECATEDis applied to the next clone; after a React render (cloneNode) replaces a node's props, the next clone applies the native props again. These C++ tests run internally; locally I compiled the test file against the real headers with a stub gtest (clang++ -fsyntax-only), as OSS CI doesn't run ReactCommon gtests.yarn fantomwith thesetNativePropsreconciliation tests (Mounting-itest.js,ReactNativeElement-itest.js) and the Animated suites that drive JS animations throughsetNativeProps(AnimatedComponents,AnimatedComposition,AnimatedValue,TimingAnimation): 6 suites, 285 tests pass.node scripts/clang-format.js --checkreports no changes for the touched files.ShadowNodeFamilyfield is excluded by_DEPRECATEDand the newShadowNodefield is private.Animated.Textrows with JS-driven loops (even rows animatecolor, odd rowsfontSize), RNTester release builds:mainat f7de203 with local benchmark screens and call counters), installed alternately, 20 s per run, three rounds. "Before" is a build with this check behind a runtime switch, turned off, soclonere-appliesnativeProps_DEPRECATEDevery time as onmain; "this PR" is this change as is:measurePreparedLayoutJNI calls per secondTextdraws per second (animation throughput)TextdrawA control screen without
setNativeProps(200 single-style numbers changing every 100 ms) is unchanged: JS time per text layout 0.45 → 0.44 ms, main thread 2.30 → 2.33 s.clonePropsand its callees) fell from 27–28% of the JS thread (1,608 / 2,708 ms) to 4.4–4.5% (158 / 208 ms). Total JS and main-thread times varied too much between runs on this phone to compare.🤖 Generated with Claude Code