[RN] Set up test to create public instances lazily in Fabric #32363
Conversation
rubennorte
requested review from
jackpope,
javache,
rickhanlonii and
sammy-SC
rubennorte
force-pushed
the
fabric-lazy-public-instance
branch
from
96beacb to
e3c24c3
Compare
|
Comparing: 192555b...e0e5a64 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
rubennorte
force-pushed
the
fabric-lazy-public-instance
branch
from
e3c24c3 to
31fa126
Compare
| publicInstance: PublicInstance, | ||
| // Exposed through refs. Potentially lazily created. | ||
| publicInstance: PublicInstance | null, | ||
| // This is only necessary to lazily create `publicInstance`. |
There was a problem hiding this comment.
Add docs that this will be nulled out once publicInstance is created?
| // Exposed through refs. Potentially lazily created. | ||
| publicInstance: PublicInstance | null, | ||
| // This is only necessary to lazily create `publicInstance`. | ||
| publicRootInstance?: PublicRootInstance | null, |
There was a problem hiding this comment.
Is there anyway we could retrieve this lazily from internalInstanceHandle for example?
This is an extra slot for every single host component, which we should avoid if we can.
There was a problem hiding this comment.
Unfortunately internalInstanceHandle is pretty much an opaque object here, so we can't tap into the internals. Also, even if we did, the Fiber itself doesn't have a reference to the root, and we'd have to traverse the fiber tree up to it to access this, which would be inefficient.
The benchmarks I used took this field into account already, and even with this it should be a net win.
There was a problem hiding this comment.
I wouldn't expect this to show on benchmarks, but rather as memory overhead (increased overall heap size).
There was a problem hiding this comment.
Oh, I only meant runtime performance when I was referencing the benchmarks. What I mean is that this change will overall reduce memory usage, even with that new field, as we'll avoid a large amount of allocations for unused refs.
rubennorte
force-pushed
the
fabric-lazy-public-instance
branch
from
31fa126 to
a5faff6
Compare
## Summary In React Native, public instances and internal host nodes are not represented by the same object (ReactNativeElement & shadow nodes vs. just DOM elements), and the only one that's required for rendering is the shadow node. Public instances are generally only necessary when accessed via refs or events, and that usually happens for a small amount of components in the tree. This implements an optimization to create the public instance on demand, instead of eagerly creating it when creating the host node. We expect this to improve performance by reducing the logic we do per node and the number of object allocations. ## How did you test this change? Manually synced the changes to React Native and run Fantom tests and benchmarks, with the flag enabled and disabled. All tests pass in both cases, and benchmarks show a slight but consistent performance improvement. DiffTrain build for [f83903b](f83903b)
4 participants
Summary
In React Native, public instances and internal host nodes are not represented by the same object (ReactNativeElement & shadow nodes vs. just DOM elements), and the only one that's required for rendering is the shadow node. Public instances are generally only necessary when accessed via refs or events, and that usually happens for a small amount of components in the tree.
This implements an optimization to create the public instance on demand, instead of eagerly creating it when creating the host node. We expect this to improve performance by reducing the logic we do per node and the number of object allocations.
How did you test this change?
Manually synced the changes to React Native and run Fantom tests and benchmarks, with the flag enabled and disabled. All tests pass in both cases, and benchmarks show a slight but consistent performance improvement.