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

[Box][styled] Cache makeStyles() per JSON of props for style functions #16858

Closed
wants to merge 1 commit into from

Conversation

ypresto
Copy link
Contributor

@ypresto ypresto commented Aug 2, 2019

Fixes #16704

Make styled() use filterProps value to generate cache key.
You can use _useStylesCache: null option to disable this behavior.

❯ yarn box
yarn run v1.17.3
$ cd ../../ && NODE_ENV=production BABEL_ENV=benchmark babel-node packages/material-ui-benchmark/src/box.js --inspect=0.0.0.0:9229
Debugger listening on ws://0.0.0.0:9229/4e1ad776-8951-4f0b-9f50-61de494bfd7f
For help, see: https://nodejs.org/en/docs/inspector
Box x 2,165 ops/sec ±1.19% (187 runs sampled)
Box with disable props cache x 413 ops/sec ±1.61% (183 runs sampled)
useStyles x 4,782 ops/sec ±1.40% (187 runs sampled)
Inline style x 9,856 ops/sec ±1.30% (190 runs sampled)
✨  Done in 45.98s.

It's still 2x slower than useStyles(), but 5x faster than original implementation..! (This benchmark creates 100 Boxes at once)

Limitations:

  • Alias (m={2}) and non-alias (margin={2}) props will create different cache.
  • Different props order will create different cache.
  • Updating props is slower than original.
    • Maybe using link: true and updating existing style if refs === 1 can solve this.

@ypresto ypresto force-pushed the cache-box-style branch 2 times, most recently from d191188 to e3e4c4d Compare August 2, 2019 13:08
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 2, 2019

@material-ui/core: parsed: +0.29% , gzip: +0.39%
@material-ui/styles: parsed: +1.86% , gzip: +2.25%

Details of bundle changes.

Comparing: 78db8a0...6a07098

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.29% 🔺 +0.39% 🔺 329,663 330,616 90,121 90,468
@material-ui/core/Paper +1.38% 🔺 +1.69% 🔺 68,684 69,634 20,469 20,814
@material-ui/core/Paper.esm +0.42% 🔺 +0.29% 🔺 62,058 62,321 19,209 19,264
@material-ui/core/Popper 0.00% -0.03% 29,185 29,185 10,434 10,431
@material-ui/core/Textarea 0.00% +0.04% 🔺 5,759 5,759 2,368 2,369
@material-ui/core/TrapFocus 0.00% -0.06% 3,834 3,834 1,617 1,616
@material-ui/core/styles/createMuiTheme +0.02% 🔺 +0.05% 🔺 16,389 16,392 5,824 5,827
@material-ui/core/useMediaQuery 0.00% +0.23% 🔺 3,213 3,213 1,311 1,314
@material-ui/lab +0.17% 🔺 +0.13% 🔺 152,607 152,870 46,520 46,581
@material-ui/styles +1.86% 🔺 +2.25% 🔺 51,401 52,356 15,285 15,629
@material-ui/system +0.04% 🔺 +0.21% 🔺 15,666 15,672 4,350 4,359
Button +0.33% 🔺 +0.21% 🔺 79,421 79,683 24,279 24,331
Modal 0.00% -0.10% 15,050 15,050 5,233 5,228
Portal 0.00% 0.00% 3,579 3,579 1,568 1,568
Rating +0.37% 🔺 +0.21% 🔺 70,735 70,997 22,079 22,126
Slider +0.35% 🔺 +0.20% 🔺 75,066 75,329 23,271 23,317
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 52,124 52,124 13,837 13,837
docs.main +0.05% 🔺 +0.07% 🔺 590,550 590,845 188,602 188,736
packages/material-ui/build/umd/material-ui.production.min.js +0.22% 🔺 +0.31% 🔺 299,964 300,625 86,137 86,408

Generated by 🚫 dangerJS against 6a07098

@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2019

Please include benchmark results using the production build once this is ready for review.

@ypresto
Copy link
Contributor Author

ypresto commented Aug 7, 2019

I decided to implement cache on styled().
Ready for review!

@ypresto ypresto changed the title [Box] Cache makeStyles result per props JSON [Box][styled] Cache makeStyles() per JSON of props for style functions Aug 7, 2019
@ypresto
Copy link
Contributor Author

ypresto commented Aug 7, 2019

oops, something wrong.

if (
name &&
name.indexOf('Mui') === 0 &&
!styleSheet.options.muiDynamic &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was important to use different class name for every Box instance (otherwise class name of all elements will be "MuiBox-root").

@ypresto ypresto force-pushed the cache-box-style branch 2 times, most recently from 0ce6450 to 75d179d Compare August 7, 2019 19:30
@ypresto
Copy link
Contributor Author

ypresto commented Aug 7, 2019

Sorry for bothering with force-pushes. Finally fixed issues!

@oliviertassinari
Copy link
Member

@ypresto Thank you for working on the problem! It's important. I will soon have a look at it :)

@joacub
Copy link

joacub commented Aug 17, 2019

@ypresto Thank you for working on the problem! It's important. I will soon have a look at it :)

oliviertassinari this its important, when useStyles its called with props its called two times per render, so the first render don't match with the server any time, and its completely inefficient called more times the same thing.

but more important never match with server classes when props its present because the counter its called two times on the client.

@bartlomiejzuber
Copy link

@oliviertassinari what's the status with it ? It's going to be merged some day ?

@oliviertassinari
Copy link
Member

@ypresto As we are moving to a unstyled core and adapters for styled-components/emotion/jss, it's probably not worth pushing further. Thanks for the exploration! It could be interesting to move the concern to jss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Box The React component. performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[system] Boxes with identical style props use the same class
6 participants