Commit d0078c2
Avoid copying the children list twice when cloning a path to the root (#58690)
Summary:
`ShadowNode::cloneTree` copies the parent's children into a local vector, swaps in the new child, then copies that local again into the `std::make_shared` passed to `clone()`. The second copy bumps and drops the refcount of every sibling at each level on the path to the root. The local is dead after the call, so this moves it instead. `UIManager::updateShadowTree` has the same double copy, and `addAncestorsToUpdateList` copied the whole children vector just to read one element. Output is unchanged.
This is a small cleanup rather than a meaningful perf win, since the parent clone itself does O(N) Yoga work per child. Host benchmark (clang -O3, Apple M4 Pro), `cloneTree` on one leaf of a View with N View siblings, median of 7 runs:
| Children | Before | After | Delta |
|---|---|---|---|
| 10 | 1.68 µs | 1.65 µs | -1.7% |
| 100 | 16.02 µs | 15.76 µs | -1.6% |
| 1000 | 164.23 µs | 160.84 µs | -2.1% |
At 10–100 children the difference is within noise.
<details><summary>Benchmark</summary>
```cpp
#include <benchmark/benchmark.h>
#include <react/renderer/components/view/ViewComponentDescriptor.h>
#include <react/renderer/core/EventDispatcher.h>
#include <react/renderer/core/ShadowNode.h>
#include <react/utils/ContextContainer.h>
#include <memory>
#include <vector>
namespace facebook::react {
auto contextContainer = std::make_shared<const ContextContainer>();
auto eventDispatcher = std::shared_ptr<EventDispatcher>{nullptr};
auto viewComponentDescriptor =
ViewComponentDescriptor{ComponentDescriptorParameters{
.eventDispatcher = eventDispatcher,
.contextContainer = contextContainer}};
std::shared_ptr<ShadowNode> createViewShadowNode(
Tag tag,
std::shared_ptr<const std::vector<std::shared_ptr<const ShadowNode>>>
children) {
auto family = viewComponentDescriptor.createFamily(
{.tag = tag, .surfaceId = 1, .instanceHandle = nullptr});
return viewComponentDescriptor.createShadowNode(
{.props = ViewShadowNode::defaultSharedProps(),
.children = std::move(children)},
family);
}
// Clones the path from the root to a single leaf of a parent with
// `state.range(0)` children, as a state update on that leaf would.
static void cloneTreeWithSiblings(benchmark::State& state) {
auto childCount = static_cast<Tag>(state.range(0));
auto children =
std::make_shared<std::vector<std::shared_ptr<const ShadowNode>>>();
for (Tag tag = 0; tag < childCount; tag++) {
children->push_back(createViewShadowNode(
tag + 2, ShadowNode::emptySharedShadowNodeSharedList()));
}
auto leaf = children->at(childCount / 2);
auto root = createViewShadowNode(1, children);
for (auto _ : state) {
auto newRoot = root->cloneTree(
leaf->getFamily(),
[](const ShadowNode& oldShadowNode) { return oldShadowNode.clone({}); });
benchmark::DoNotOptimize(newRoot);
}
}
BENCHMARK(cloneTreeWithSiblings)->Arg(10)->Arg(100)->Arg(1000);
} // namespace facebook::react
BENCHMARK_MAIN();
```
</details>
## Changelog:
[GENERAL] [CHANGED] - Avoid a redundant copy of the children list in `ShadowNode::cloneTree` and `UIManager::updateShadowTree`
Pull Request resolved: #58690
Test Plan:
Added `cloneTree` coverage to `ShadowNodeTest`. Built the renderer gtests on the host with CMake (Debug):
- `core/tests`: 65/65 pass (the 4 Hermes-dependent files were not built)
- `mounting/tests` StateReconciliationTest, StackingContextTest, OrderIndexTest: 21/21 pass
Not built for Android/iOS and not measured on a device.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Reviewed By: javache
Differential Revision: D122059304
Pulled By: fabriziocucci
fbshipit-source-id: 699574af73217ad569ba840c834a55de9c5dd0191 parent 25041be commit d0078c2
3 files changed
Lines changed: 38 additions & 3 deletions
File tree
- packages/react-native/ReactCommon/react/renderer
- core
- tests
- uimanager
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
411 | 411 | | |
412 | 412 | | |
413 | 413 | | |
414 | | - | |
| 414 | + | |
415 | 415 | | |
416 | 416 | | |
417 | 417 | | |
| |||
Lines changed: 34 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
429 | 429 | | |
430 | 430 | | |
431 | 431 | | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
Lines changed: 3 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
47 | | - | |
| 47 | + | |
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
| |||
201 | 201 | | |
202 | 202 | | |
203 | 203 | | |
204 | | - | |
| 204 | + | |
| 205 | + | |
205 | 206 | | |
206 | 207 | | |
207 | 208 | | |
| |||
0 commit comments