[Modal] performance

[priscilawebdev]

Firstly, I would like to thank you for all your efforts in building this amazing library. We love using it.

• The issue is present in the latest release.

It is related to this issue: #10778, but as @oliviertassinari said this "I'm closing this issue. We need a dedicated performance report for each potential area of improvement, not an umbrella thread." I decided to open a separate issue. So..

• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

We are currently experiencing performance issues rendering the Dialog component as a child of other components rendered inside a map() method.

The Dialog components open with considerable delay. For instance:

The Dialog takes longer than 3s to open.😬

After reading more about the Dialog API documentation, I decided to try the "keepMounted" prop. I noticed a improvement performance, but pages takes long to finally render.. Please see:

Page loads - before keepMounded:

Page loaded - after keepMounted:

Dialog component - with keepMounted:

So after read about the modal performance issue and how to solve it "You can speed up the rendering by moving the modal body into its own component.". I decided to try it out, but even after moving the dialog body into its own component the problem persisted.

Expected Behavior 🤔

The Dialog component should not open with a delay. The performance should be improved.

Context 🔦

Desktop version

Your Environment 🌎

Tech	Version
Material-UI	v4.0.0
React	v16.8.6
Browser	Chrome
TypeScript	v3.5.3

[oliviertassinari]

@priscilawebdev Thanks for the detailed report. There is one aspect that I think we miss. Why is the dialog component concerned with the issue? It seems that most of the time is spend outside Material-UI.

[eps1lon]

Thank you for opening this detailed performance report!

From what I can tell 1s alone is spent rendering the DynamicComponent. Is this a generic name from react devtools or where does this component come from?

There are some possible improvements for this scenario but they all involve unstable react features.

If DynamicComponent is within your codebase you would have to find a way to improve the performance.

What would be possible is to not render the DialogContent until you decide you have enough render resources to spare. You would render the dialog initially with keepMounted={false} and once some heuristic determines you have time you could switch keepMounted to true. After that opening the dialog should feel instant. This is basically a very crude implementation of offscreen rendering. How good it feels depends on how good your heuristic is.

[eps1lon]

For example something like this: https://codesandbox.io/s/crude-offscreen-render-5tfxu

This is what keepMounted is for. But we cannot tell you if this is appropriate for your use case. You have to carefully measure what your users are doing and how it impacts their perceived performance.

[priscilawebdev]

@eps1lon, @oliviertassinari thank you for your prompt reply!

@eps1lon Yes the DynamicComponent is within our codebase. I will analyse the component and see if it's performance can be improved.

Hmmm interesting 'possible solution' with the heuristic and keepMounted. Let's try it out and see if it solves our problem 😉🙂 Btw thanks for the example.

My findings will be posted here 😉

[ZYinMD]

May or may not be relevant to this issue, but I have a question regarding the Doc talking about the performance of modals. Basically it says A is bad, and B is good:

// A:
const [open, setOpen] = useState(false);
return <Modal open={open}>{/* lots of JSX here */}</Modal>;
// B:
const [open, setOpen] = useState(false);
return <Modal open={open}><ReturnLotsOfJSX /></Modal>;

A is bad because the modal is always rendered even when (!open). Although it's not mounted, there's still a cost.

Which raises some questions:

1. If I use A, but add a condition in this way below, does it solve the problem?

const [open, setOpen] = useState(false);
return open && <Modal open={open}>{/* lots of JSX here */}</Modal>;

2. In the source code of Modal, does it have something like if (!open) return null;? Would that solve the problem?

3. Should I worry about the same problem when using Menu?

const [anchorEl, setAnchorEl] = useState(null);
return (
    <Menu open={!!anchorEl} anchorEl={anchorEl}>
      {/* lots of JSX here */}
    </Menu>
)

[eps1lon]

@ZYinMD It's not "bad". Just because something isn't needed doesn't mean it's bad or needs optimization. The first rule of performance is that you measure. Don't look at the code and find the lines that aren't needed. Measure what is happening. If you see issues then and only then should you take action.

The issue described in the section is not about rendering but createElement. IMO we shouldn't mention this in the section. I'm not aware of bottlenecks that a createElement call causes and as seen here causes more confusion than it helps. It was just about moving many createElement calls into a single on.

I'll do a benchmark and compare the approaches. If it's insignificant we can cut the section. Though this isn't relevant to this issue. @ZYinMD If you have any more questions please open a separate issue.

[ZYinMD]

Thanks @eps1lon , I'll create a separate issue, since I really want to learn about the best practice for Menu.

[oliviertassinari]

@priscilawebdev Do you have a reproduction for this issue with the Modal?

[priscilawebdev]

@oliviertassinari I am sorry, but I don't have it. It is very difficult to create an example similar of what we have here...we render many components that make use of hooks that create and inject 1) empty styles 2) synchronously. This is very expensive.

The possible solution with the heuristic + the keepMounted suggested by @eps1lon unfortunately, for some reasons, it is not ideal to solve our problem. I also noticed that even using keepMounted 'correctly', some actions freeze. Ex: I can't click on any button for a few seconds.

After analyzing the Dynamic component and giving a proper name to the anonymous functions we have. I concluded that this performance issue is related to this issue created by my co-worker: #16756

[oliviertassinari]

After analyzing the Dynamic component and giving a proper name to the anonymous functions we have. I concluded that this performance issue is related to this issue created by my co-worker: #16756

Great, we can close this issue then :)