facebook
Add flag to test fast jsx
Following #28768, add a path to testing Fast JSX on www.
Cleaning up string ref usage on www will be a longer term effort and given the expected win here, we need a way to experiment on the JSX changes in the meantime. When enableRefAsProp is rolled out, we'll be able to compare performance data using enableFastJSXWithStringRefs.
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.
Comparing: 1d618a9cf36948dbf94aa6a6f2eb9e6a64b52eb0...c40ba1a3c3a556b997561ada43b3344e1a5586c1
Critical size changes
Includes critical production bundles, as well as any change greater than 2%:
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| oss-stable/react-dom/cjs/react-dom.production.js | = | 6.66 kB | 6.66 kB | = | 1.82 kB | 1.82 kB |
| oss-stable/react-dom/cjs/react-dom-client.production.js | = | 492.61 kB | 492.61 kB | = | 87.88 kB | 87.88 kB |
| oss-experimental/react-dom/cjs/react-dom.production.js | = | 6.67 kB | 6.67 kB | = | 1.83 kB | 1.83 kB |
| oss-experimental/react-dom/cjs/react-dom-client.production.js | = | 498.86 kB | 498.86 kB | = | 88.93 kB | 88.93 kB |
| facebook-www/ReactDOM-prod.classic.js | = | 591.22 kB | 591.22 kB | = | 103.96 kB | 103.96 kB |
| facebook-www/ReactDOM-prod.modern.js | = | 567.44 kB | 567.44 kB | = | 100.36 kB | 100.36 kB |
| test_utils/ReactAllWarnings.js | Deleted | 64.26 kB | 0.00 kB | Deleted | 16.02 kB | 0.00 kB |
Significant size changes
Includes any change greater than 0.2%:
Expand to show
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| facebook-www/React-prod.modern.js | +1.61% | 22.73 kB | 23.10 kB | +1.25% | 5.86 kB | 5.93 kB |
| facebook-www/React-prod.classic.js | +1.61% | 22.73 kB | 23.10 kB | +1.26% | 5.86 kB | 5.93 kB |
| facebook-www/React-profiling.modern.js | +1.58% | 23.17 kB | 23.53 kB | +1.25% | 5.93 kB | 6.01 kB |
| facebook-www/React-profiling.classic.js | +1.58% | 23.17 kB | 23.53 kB | +1.25% | 5.94 kB | 6.01 kB |
| facebook-www/JSXDEVRuntime-dev.classic.js | +0.50% | 52.06 kB | 52.32 kB | +0.40% | 14.63 kB | 14.69 kB |
| facebook-www/JSXDEVRuntime-dev.modern.js | +0.50% | 52.09 kB | 52.35 kB | +0.40% | 14.64 kB | 14.70 kB |
| facebook-www/React-dev.modern.js | +0.22% | 116.17 kB | 116.43 kB | +0.20% | 30.12 kB | 30.18 kB |
| facebook-www/React-dev.classic.js | +0.22% | 116.66 kB | 116.92 kB | +0.20% | 30.22 kB | 30.28 kB |
| test_utils/ReactAllWarnings.js | Deleted | 64.26 kB | 0.00 kB | Deleted | 16.02 kB | 0.00 kB |
Generated by :no_entry_sign: dangerJS against c40ba1a3c3a556b997561ada43b3344e1a5586c1
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 fast-jsx-runtime and slow-jsx-runtime for Meta and then you can run the test with a requireCond.
Is 1 additional boolean check really that critical here? It seems like compared to all the allocations going on that would be minimal?
At some point, we're going to start rolling out each of the feature flags affecting jsx-runtime, such as enableRefAsProp and disableStringRefs. When that happens, these flags will have to become dynamic for at least a short period of time for stabilization in production.
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 enableFastJSX feature flag instead of an enableFastJSXWithStringRefs feature flag.
@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:
- Update this implementation to have a more general
enableFastJSXflag like you suggest. On in OSS, dynamic for our tests - Run a benchmark with the extra boolean checks so we have an understanding (for this and for the future) of what kind of overhead will occur from the check
- Clean up whatever tests are failing here and get this in a landable state so we can start testing assuming said benchmark doesn't majorly regress control