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

[docs] Remove section about modal performance #17284

Merged
merged 4 commits into from
Sep 2, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Sep 2, 2019

Testing claim of https://material-ui.com/components/modal/#performance in production bundle. Edit: Results in #17284 (comment)

The section is talking about createElement performance impact in general. It only becomes an issue for very large trees in render functions at which point virtualization/memoization of the component in question is more appropriate.

Closes: #17271

@eps1lon eps1lon added docs Improvements or additions to the documentation performance labels Sep 2, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 2, 2019

No bundle size changes comparing d81e0a7...b320a85

Generated by 🚫 dangerJS against b320a85

@eps1lon
Copy link
Member Author

eps1lon commented Sep 2, 2019

Between https://5d6d0ecb670755000c993149--material-ui.netlify.com/performance/modal-many-elements/ and https://5d6d0ecb670755000c993149--material-ui.netlify.com/performance/modal-single-element/ we have .5 - .8ms difference on an update.

This is negligible. The section only applies to owners of very large element trees and even then is the createElement call likely not the bottleneck. If it becomes a bottleneck common refactoring practices (large funtions are hard to reason about etc.) apply as well.

improvement is negligible and causes confusion or rather
is applied too eagerly
@eps1lon eps1lon changed the title [docs] Test section about modal perf [docs] Remove section about modal performance Sep 2, 2019
@eps1lon eps1lon marked this pull request as ready for review September 2, 2019 13:21
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I don't fully remember the context for this section in the documentation. It originates to when I was using Material-UI at Onepixel. I have no objection to removing it, if it's truly useful, the developers will let us know and we can simplify revert.

@mbrookes mbrookes merged commit 5b6262b into mui:master Sep 2, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Sep 2, 2019

I don't fully remember the context for this section in the documentation. It originates to when I was using Material-UI at Onepixel. I have no objection to removing it, if it's truly useful, the developers will let us know and we can simplify revert.

It's easy to forget some expensive function calls (or many moderately expensive calls) in large JSX trees which might explain a larger impact than just createElement calls. But then again you have more of a problem with a component that's just too big to fully understand and not necessarily a performance problem.

That's also why I prefer to use intermediate variables instead of inlining the function calls. Makes it easier to spot what's actually happening.

+ const className = clsx(a, b, c, [d, e], { f, g });
+ return <div className={className} />;
- return <div className={clsx(a, b, c, [d, e], { f, g })} />;

Size concerns are either negligible or can be optimized by a compiler (I really need to look into closure compiler. React core is using this as well and they are even more liberal with intermediate variables.)

@eps1lon eps1lon deleted the docs/modal-perf branch September 2, 2019 19:14
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2019

The limitation with the intermediary className variable is that it allows the definition of the class names and the application of them to be decorrelated, I have seen components where there were a gap of dozens of line of codes.

I think that the code is easier to read when the style is as close as possible to the element. I think that it's part of the reason why the styled-components API is popular.

Lately, I was going crazy having to go back en forth between my large render method a the bottom of the page and my large style sheet declaration at the top. (This problem can be solved with two tabs in the editor)

@mbrookes
Copy link
Member

mbrookes commented Sep 2, 2019

This problem can be solved with two tabs in the editor

Facepalms for not doing that in the past. (scroll up, scroll down, scroll up, scroll down... )

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2019

@mbrookes I have never used this feature yet, I remember looking at it 5 years ago (a C teacher wanted the students to learn and use this feature of vim), and thought, Nah, let's hope I never need that 😆.

@joshwooding
Copy link
Member

@oliviertasinnari

Nah, let's hope I never need that 😆.

You just summed up most of my experience with learning vim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Modal] Overhead of having unmounted modals
5 participants