Add flag to test fast jsx #28816
Add flag to test fast jsx #28816
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 1d618a9...c40ba1a 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
|
|
Dynamic flags in JSX is a bad idea since it'll slow down the default case. You can maybe try it for bugs but you'll slow down both the test and control so perf testing isn't good and also you slow down everyone. |
|
Alternatively we could publish separate |
|
Is 1 additional boolean check really that critical here? It seems like compared to all the allocations going on that would be minimal? |
(Unless "key" is spread onto the element.) Historically, the JSX runtime clones the props object that is passed in. We've done this for two reasons. One reason is that there are certain prop names that are reserved by React, like `key` and (before React 19) `ref`. These are not actual props and are not observable by the target component; React uses them internally but removes them from the props object before passing them to userspace. The second reason is that the classic JSX runtime, `createElement`, is both a compiler target _and_ a public API that can be called manually. Therefore, we can't assume that the props object that is passed into `createElement` won't be mutated by userspace code after it is passed in. However, the new JSX runtime, `jsx`, is not a public API — it's solely a compiler target, and the compiler _will_ always pass a fresh, inline object. So the only reason to clone the props is if a reserved prop name is used. In React 19, `ref` is no longer a reserved prop name, and `key` will only appear in the props object if it is spread onto the element. (Because if `key` is statically defined, the compiler will pass it as a separate argument to the `jsx` function.) So the only remaining reason to clone the props object is if `key` is spread onto the element, which is a rare case, and also triggers a warning in development. In a future release, we will not remove a spread key from the props object. (But we'll still warn.) We'll always pass the object straight through. The expected impact is much faster JSX element creation, which in many apps is a significant slice of the overall runtime cost of rendering.
|
At some point, we're going to start rolling out each of the feature flags affecting I think there's consensus that they'll introduce non-zero overhead, but maybe there's disagreement about how much and the magnitude of regression. As long as we're not regressing open source bundles, I think that the regression can be a calculated risk for Meta-specific experimentation. For what it's worth, my expectation is that as long as both the control and test groups have the additional overhead, our experimentation results should still be sufficiently meaningful. Knowing that these code paths are hot, we can also minimize the time that the feature flag is configured to be dynamic. In conclusion, I don't think we need to block on forking the entire build artifact before trying this out in production. That all said, I would prefer to see an |
|
@yungsters My intention here was to both have a path to testing this sooner that we expect the two blocking flags to roll out internally, and also to generally create a stable experiment to understand the impact of fast jsx. The built in assumption in the initial implementation was that we had a while before the blocking flags were enabled but I agree its better to be explicit, especially since the story there might be different on the RN side. Plus now we have the string ref codemod. My next steps here will be:
|
jackpope
force-pushed
the
fastjsx-www
branch
2 times, most recently
from
add49d5
to
d9deaab
Compare
Following #28768, add a path to testing Fast JSX on www. We want to measure the impact of Fast JSX and enable a path to testing before string refs are completely removed in www (which is a work in progress). Without `disableStringRefs`, we need to copy any object with a `ref` key so we can pass it through `coerceStringRef()` and copy it into the object. This de-opt path is what is gated behind `enableFastJSXWithStringRefs`. The additional checks should have no perf impact in OSS as the flags remain true there and the build output is not changed. For www, I've benchmarked the addition of the boolean checks with values cached at module scope. There is no significant change observed from our benchmarks and any latency will apply to test and control branches evenly. This added experiment complexity is temporary. We should be able to clean it up, along with the flag checks for `enableRefAsProp` and `disableStringRefs` shortly. DiffTrain build for [1beb73d](1beb73d)
Following #28768, add a path to testing Fast JSX on www.
We want to measure the impact of Fast JSX and enable a path to testing before string refs are completely removed in www (which is a work in progress).
Without
disableStringRefs, we need to copy any object with arefkey so we can pass it throughcoerceStringRef()and copy it into the object. This de-opt path is what is gated behindenableFastJSXWithStringRefs.The additional checks should have no perf impact in OSS as the flags remain true there and the build output is not changed. For www, I've benchmarked the addition of the boolean checks with values cached at module scope. There is no significant change observed from our benchmarks and any latency will apply to test and control branches evenly. This added experiment complexity is temporary. We should be able to clean it up, along with the flag checks for
enableRefAsPropanddisableStringRefsshortly.