Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag to test fast jsx #28816

Merged
merged 1 commit into from
May 3, 2024
Merged

Add flag to test fast jsx #28816

merged 1 commit into from
May 3, 2024

Conversation

jackpope
Copy link
Contributor

@jackpope jackpope commented Apr 10, 2024

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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 10, 2024
@react-sizebot
Copy link

react-sizebot commented Apr 10, 2024

Comparing: 1d618a9...c40ba1a

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 🚫 dangerJS against c40ba1a

@sebmarkbage
Copy link
Collaborator

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.

@acdlite
Copy link
Collaborator

acdlite commented Apr 10, 2024

Alternatively we could publish separate fast-jsx-runtime and slow-jsx-runtime for Meta and then you can run the test with a requireCond.

@kassens
Copy link
Member

kassens commented Apr 10, 2024

Is 1 additional boolean check really that critical here? It seems like compared to all the allocations going on that would be minimal?

yungsters referenced this pull request Apr 22, 2024
(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.
@yungsters
Copy link
Contributor

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.

@jackpope
Copy link
Contributor Author

@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 enableFastJSX flag 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

@jackpope jackpope force-pushed the fastjsx-www branch 2 times, most recently from add49d5 to d9deaab Compare April 30, 2024 19:26
@jackpope jackpope marked this pull request as ready for review April 30, 2024 20:05
@jackpope jackpope merged commit 1beb73d into facebook:main May 3, 2024
38 checks passed
@jackpope jackpope deleted the fastjsx-www branch May 3, 2024 14:47
github-actions bot pushed a commit that referenced this pull request May 3, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants