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

Reduce React load time by 3-7ms in Node #7493

Merged
merged 1 commit into from
Aug 14, 2016
Merged

Reduce React load time by 3-7ms in Node #7493

merged 1 commit into from
Aug 14, 2016

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Aug 14, 2016

The creation of the DOM factories with mapObject is a relatively expensive operation in Node. It's accounting for ~10% of the require load time across the various builds. I saw some improvements from only unwrapping createDOMFactory, but the bulk of the gains are from removing mapObject.

I ran a few benchmarks that only focused on load times using [email protected] and [email protected] (source). Each test was run 100 times:

engine build before: avg / p90 (ms) after: avg / p90 (ms)
node react.js (development) 29.14 / 30 22.07 / 23
node react.js (production) 23.30 / 24 19.99 / 21
node dist/react-with-addons.js 29.61 / 30 25.95 / 27
node dist/react-with-addons.min.js 20.06 / 21 17.55 / 18
node dist/react.js 21.83 / 22 17.91 / 18
node dist/react.min.js 15.08 / 15 12.33 / 13
jsc dist/react-with-addons.js 8.48 / 9 8.35 / 9
jsc dist/react-with-addons.min.js 6.04 / 6 5.93 / 6
jsc dist/react.js 5.75 / 6 5.46 / 6
jsc dist/react.min.js 4.07 / 4 3.96 / 4

This change also reduced the gziped size of the bundles:

   raw     gz Compared to master @ cba2b19b84b339e937b4bbc258cd09b2b8303391
     =      = build/react-dom-fiber.js
     =      = build/react-dom-fiber.min.js
     =      = build/react-dom-server.js
     =      = build/react-dom-server.min.js
     =      = build/react-dom.js
     =      = build/react-dom.min.js
  +746   -222 build/react-with-addons.js
  +159    -53 build/react-with-addons.min.js
  +746   -244 build/react.js
  +159    -29 build/react.min.js

Raw size can also be reduced by renaming createDOMFactory to something shorter.

This change is GCC safe (see discussion in #5759).

I'm also investigating adding a babel transform to inline the result of keyMirror and keyOf.

@vjeux
Copy link
Contributor

vjeux commented Aug 14, 2016

This is insane to me that this takes ~10% of the startup time! cc @gaearon and @spicyj for accepting.

@sophiebits sophiebits added this to the 15-next milestone Aug 14, 2016
@sophiebits sophiebits merged commit e5f9ae2 into facebook:master Aug 14, 2016
@sophiebits
Copy link
Collaborator

Works for me, thanks!

@vjeux
Copy link
Contributor

vjeux commented Aug 14, 2016

Nuclide is not even using React.DOM :( https://fburl.com/407417521

@vjeux
Copy link
Contributor

vjeux commented Aug 14, 2016

If you remove mapObject from this test, you can also kill the entire module from React codebase fyi:

return mapObject(statusDisplays, function(statusDisplay, key) {

@zertosh
Copy link
Member Author

zertosh commented Aug 14, 2016

Thanks! @vjeux we do use React.DOM but it's called ReactDOM since we get it through react-for-atom https://github.com/jgebhardt/react-for-atom/blob/746b715/react-for-atom.js#L22

EDIT: I misread as react-dom. Nuclide does not use React.DOM, we already write jsx.

@bolinfest
Copy link

Would we be willing to use memoizing lazy getters instead? I suspect very few of these tags are used, in practice.

@zertosh
Copy link
Member Author

zertosh commented Aug 15, 2016

I just ran a quick unscientific test, and it takes ~0.63ms to create ReactDOMFactories (since you're calling 130 functions, plus whatever work that does). However, if you make them getters, so that it looks like this:

var ReactDOMFactories = {
  get a() { return createDOMFactory('a'); },
  get abbr() { return createDOMFactory('abbr'); },
 //...
};

It takes ~0.13ms to create that object. I'm not sure if ~0.5ms is worth it.

@zpao
Copy link
Member

zpao commented Aug 15, 2016

Yea not worth it. We're 100% more likely to do #6169 and split these off from React entirely.

@cpojer
Copy link
Contributor

cpojer commented Aug 15, 2016

Awesome work.

@zpao zpao modified the milestones: 15.3.1, 15-next Aug 15, 2016
zpao pushed a commit that referenced this pull request Aug 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants