Ensure React object shape is monomorphic #14267
Conversation
| this.actualDuration = 0; | ||
| this.actualStartTime = -1; | ||
| this.selfBaseDuration = 0; | ||
| this.treeBaseDuration = 0; |
There was a problem hiding this comment.
I thought this conditional was stripped from production and production+profiling builds (as part of uglification).
Removing it will cause non-profiling production builds to increase memory size, no?
There was a problem hiding this comment.
Looking at the CJS bundles in NPM, it looks to me like the condition is being removed from all but the dev builds.
There was a problem hiding this comment.
FWIW, my above comment only applies to builds where this flag is static ^
There was a problem hiding this comment.
I was testing this on the FB bundle, sorry, I should have stated. We need to somehow make this static for FB too otherwise it seems most, if not all of our fibers on FB code end up getting deoptimized on V8 :(
There was a problem hiding this comment.
It is static for production bundles– just not the dev bundle because we don't dead code eliminate it.
We choose which bundle to serve based on a gatekeeper. (We switch at the level of the react-dom bundle.) Within the bundles– profiling or production+profiling– we've already DCE'd so the condition has been removed.
There was a problem hiding this comment.
I remove the change, as it's not the fix. Something else must be touching those properties. Thanks for the help, I'll keep digging.
|
React: size: 🔺+0.4%, gzip: 🔺+0.3% Details of bundled changes.Comparing: d204747...ddf954e react
Generated by 🚫 dangerJS |
trueadm
changed the title
packages/react/src/React.js
Outdated
| this.useState = null; | ||
| } | ||
|
|
||
| const React = new ReactObject(); |
There was a problem hiding this comment.
The same could be accomplished, without requiring a constructor or making this more difficult to convert to ES modules, by just using conditionals for each assignment.
React.ConcurrentMode = enableStableConcurrentModeAPIs ? REACT_CONCURRENT_MODE_TYPE : undefined;
// et alThere was a problem hiding this comment.
I tried that and the object still depots, likely because it has quite a few properties.
There was a problem hiding this comment.
😢
(tangentially, it'd be great if an ES modules export was made available in time for 16.7, especially with all the additional commonly used exports exposed by hooks)
packages/react/src/React.js
Outdated
| this.useState = null; | ||
| } | ||
|
|
||
| const React = new ReactObject(); | ||
|
|
||
| if (enableStableConcurrentModeAPIs) { | ||
| React.ConcurrentMode = REACT_CONCURRENT_MODE_TYPE; |
There was a problem hiding this comment.
Do these lines not affect anything? (Any reason we shouldn't move them and the hooks assignments into the function too?)
packages/react/src/React.js
Outdated
| // This uses a function constructor to enforce a monomorphic object. Using an object literal | ||
| // along with the conditional branches causes this object to deoptimize frequently otherwise. | ||
| function ReactObject() { | ||
| this.Children = { |
There was a problem hiding this comment.
I guess React.Children.* isn't used often enough to matter?
|
cc @bmeurer |
|
I don't fully understand if it's meant to be for internal or external usage. Externally I think this wouldn't solve the problem because most people use React via webpack which already has its own object indirection in the middle. It's like |
|
@bmeurer The property callsites on the React object show the V8 deopt message "wrong map". Moving them all into a constructor seems to stop the message from showing up. I found that this is a common message that appears when accessing properties on object literals with large amounts of properties. Why aren't object literals treated the same as function constructors in terms of how they optimize? |
|
In other words we're trying to optimize property access on the React object. |
|
If we switch to ES modules exports in our syntax, then Rollup will convert this to an object for CJS anyway. Most likely packagers that consume ES module directly will convert it to an object when it is imported as |
|
Can we switch to use ES exports in our code and fix this in Rollup? |
|
@sebmarkbage What kind of change you'd like to see for this in rollup? Their output (module.exports with properties) seems to be sane for common use case |
|
If you really want to avoid having undefined exports you could have something like However, it'd make importing symbols that are not exported by the current build of React a compile error instead of a runtime one. I think this would actually be good for the most part, but it'd require code compatible with both versions (with or without the export) to use PS: even if webpack has to reify a module namespace object, it doesn't actually have to include all exports in it as long as the namespace object doesn't escape. Unfortunately, as long as there are libraries published using babelmodules, the namespace object will escape to the babel helper functions. |
|
I'm really having a hard time understanding this change and why it makes things better. On a high level this seems like we have a bug in V8 somewhere, since wrt. shape changes there shouldn't be any difference in the two approaches. I guess what I'm missing is the link to bundlers and what this ends up doing in the end. @trueadm Would it be possible to provide a standalone .js file (ideally runnable in |
|
After speaking to @bmeurer about this, I think I've identified the deopt areas in the case of the React object. We're changing the shape of React object after creating it, both in the above (where we assign the hooks and the ConcurrentMode and profiler properties) and internally at FB where we change the shape yet again. If I make the conditional properties part of the object literal and change the internal areas where we change the shape of the React object, I no longer see the same "wrong map` deopt message for the object literal approach. So I'll update the PR to reflect my findings and make a diff to change the internal shape change deopt. |
undo fiber change move around conditionals Change approach to object literal again Change approach to object literal again
trueadm
force-pushed
the
make-react-more-monomoprhic
branch
from
352c8f2
to
6116253
Compare
Create two React objects with the condition of hooks enabled foo
trueadm
force-pushed
the
make-react-more-monomoprhic
branch
from
b3d5af0
to
ddf954e
Compare
| useMutationEffect, | ||
| useReducer, | ||
| useRef, | ||
| useState, |
There was a problem hiding this comment.
Curious why we forked for enableHooks rather than using ternaries like for enableStableConcurrentModeAPIs and __DEV__? e.g.
React = {
Children: {
map,
forEach,
count,
toArray,
only,
},
createRef,
Component,
PureComponent,
createContext,
forwardRef,
lazy,
memo,
Fragment: REACT_FRAGMENT_TYPE,
StrictMode: REACT_STRICT_MODE_TYPE,
Suspense: REACT_SUSPENSE_TYPE,
createElement: __DEV__ ? createElementWithValidation : createElement,
cloneElement: __DEV__ ? cloneElementWithValidation : cloneElement,
createFactory: __DEV__ ? createFactoryWithValidation : createFactory,
isValidElement: isValidElement,
version: ReactVersion,
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: ReactSharedInternals,
ConcurrentMode: enableStableConcurrentModeAPIs
? REACT_CONCURRENT_MODE_TYPE
: null,
Profiler: enableStableConcurrentModeAPIs ? REACT_PROFILER_TYPE : null,
unstable_ConcurrentMode: !enableStableConcurrentModeAPIs
? REACT_CONCURRENT_MODE_TYPE
: null,
unstable_Profiler: !enableStableConcurrentModeAPIs
? REACT_PROFILER_TYPE
: null,
useCallback: enableHooks ? useCallback : null,
useContext: enableHooks ? useContext : null,
useEffect: enableHooks ? useEffect : null,
useImperativeMethods: enableHooks ? useImperativeMethods : null,
useLayoutEffect: enableHooks ? useLayoutEffect : null,
useMemo: enableHooks ? useMemo : null,
useMutationEffect: enableHooks ? useMutationEffect : null,
useReducer: enableHooks ? useReducer : null,
useRef: enableHooks ? useRef : null,
useState: enableHooks ? useState : null,
};No big deal I guess, just curious.
There was a problem hiding this comment.
I did that originally, but @gaearon suggested that it unnecessarily exposed the hooks APIs as properties.
There was a problem hiding this comment.
True, although...we do the same for the unstable/stable ConcurrentMode and Profiler properties. I"m not sure why that would actually matter. Seems preferable to forking but...I don't feel strongly 😄
There was a problem hiding this comment.
I had to draw the line somewhere and they seemed less bad to expose. Forking the object 4 times would have been pretty horrible. :D
There was a problem hiding this comment.
Yeah I agree completely. I wasn't suggesting that 😅 I don't like even this fork but I think we've already talked about it more than it's worth so I'll 🤐
There was a problem hiding this comment.
Hmm to be clear I don't think it's great to expose ConcurrentMode = null either.
There was a problem hiding this comment.
Would you feel any differently about undefined?
There was a problem hiding this comment.
Honestly I think we should compromise and choose one build as "primary". That's the one that should not deopt. It would make sense to me if it's the stable one. So my proposal is to make it like
// Non-alpha React API is one object
const React = {
Children: ...,
createRef,
Component,
// blabla
// don't care about undefined in alpha builds
unstable_ConcurrentMode: enableStableCMAPIs ? undefined : CM,
unstable_Profiler: enableStableCMAPIs ? undefined : P,
}
// Different feature flags can tweak it
if (enableHooks) {
React.useState = ...
}
if (enableCMAPIs) {
React.ConcurrentMode = React.unstable_ConcurrentMode;
React.Profiler = React.unstable_Profiler;
React.unstable_ConcurrentMode = undefined;
React.unstable_Profiler = undefined;
}That would make the default open source build optimized, and deopt the alphas. I don't think deopting the alphas is a huge deal since it's already deopted (status quo).
At FB internally, we can either keep it deopted until we lose the feature flags. Or we can re-export all the necessary exports manually in the internal React entry point in www. Which we need to do to solve the PropTypes and createClass deopts there anyway.
|
I disagree that that proposal is preferable to having a few Sorry to be difficult. I think we should have an objective reason for why Dominic's original approach is actually worse than an intentional de-opt approach for alphas. |
They've always been deopted so it's not a new issue. You shouldn't ideally be using them in production anyway.
It's not objective but if it appears on the React object, it will likely appear in autocomplete (at least in some editors, and in Chrome). It will appear in tools that display APIs based on |
|
These look like unreasonable expectations for an ES module, if you're ever
planning to migrate to that.
|
Yeah I guess that's fair, but since people compare e.g. performance of hooks vs classes, it sucks if we're knowingly continuing to deopt. |
|
Not sure I agree with hooks "looking confirmed", or at least not that it's more important than deopting. The |
I don't understand what this comment refers to.
Deopting affects only alpha. Shipping an API name (even without implementation) affects a stable release. |
Hm, potentially. Maybe. 😄 Anyway, you feel more strongly about this than I do so~ 🙇 |
|
I don't understand why adding expando properties would make the existing fast properties slower. |
I think what @Kovensky has meant is that with ES modules you won't be able to export things conditionally because of static nature of ES modules. |
|
I merged #14309 which does this. |
The main
Reactobject's property call-sites suffer from deoptimizing because we assign properties to the object after its creation, thus changing the object's shape (hidden class). This moves all properties back into the object literal.