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

frontend: Small performance improvements #2590

Merged
merged 6 commits into from
Dec 11, 2024
Merged

frontend: Small performance improvements #2590

merged 6 commits into from
Dec 11, 2024

Conversation

sniok
Copy link
Contributor

@sniok sniok commented Nov 21, 2024

This PR adds some useMemo, memo, useCallbacks to avoid unnecessary rerenders. Also hides a create resource popup when it's not open

@sniok sniok marked this pull request as ready for review November 25, 2024 12:40
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Nov 25, 2024
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

I am a bit surprised we need memo(...) surrounding a bunch of components. I thought this was only needed if we were doing a lot of complex work in components' bodies, and even those could be abstracted into useCallback + useEffect to avoid this.

frontend/src/components/common/Resource/CreateButton.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sidebar/Sidebar.tsx Show resolved Hide resolved
frontend/src/components/Sidebar/Sidebar.tsx Show resolved Hide resolved
@sniok
Copy link
Contributor Author

sniok commented Nov 28, 2024

I am a bit surprised we need memo(...) surrounding a bunch of components. I thought this was only needed if we were doing a lot of complex work in components' bodies, and even those could be abstracted into useCallback + useEffect to avoid this.

Let's look at what is happening on the main branch, without optimizations. This PR is mostly focused on the Sidebar and navigating between pages.

The test case is clicking on Pods from Deployments page. I've commented out the Table so it's clearer.

image

What we see here is the first rerender that happens after a click. The trigger is the change of current route and the whole sidebar rerenders. Then there's another identical rerender when sidebar selected item in a slice updates. (Ideally it should derive that from the url but that's a different issue) What user sees is Pods button turns yellow and Deployment button turns back to black but we're rerendering every item.

You might think that rerendering the same thing in react is not a big deal and in most cases it is not. But what we see is that single rerender takes 70ms (to maintain 60 fps you need to have less than 16ms per frame)

Let's see what code actually runs during this without the react profiler overhead. Here I'm using the Chrome Profiler.

image

On the top is the flame graph that represents CPU work over time and on the bottom is the list of functions that take the most time.

6 out of 10 slowest functions are from mui/emotion styling packages dealing with generating styles in runtime. This can be somewhat improved by getting rid of sx usage everywhere and using separate styled(Box) component for example.

Style recalculation is taking a big chunk here because certain mui components use functions like getBoundingClientRect during render which makes it slow.

So this is not React's fault. Aggresive memoization will help us deal with runtime style generation and heavy mui components.

@joaquimrocha
Copy link
Collaborator

Thanks a lot for the explanation. Appreciate that. I am willing to merge once the conflicts are solved.

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Dec 6, 2024
@sniok
Copy link
Contributor Author

sniok commented Dec 9, 2024

Merge conflicts were fixed

@joaquimrocha
Copy link
Collaborator

@sniok I have rebased these changes but noticed there's a bug related to my previous concerns: if you add content to the creation dialog (from the create button), then you apply and it fails to apply, or you close and reopen that dialog, it will no longer retain the contents that the user has added. This is a UX problem.

So to fix this while keeping your goal of not rendering unless necessary, I added a new commit which only renders the dialog after it's first opened (unless the keepMounted prop is passed, as maybe some plugins do want to pre-render it).
I have reverted your commit for the CreateButton, so please test and if it makes sense we just remove the original commit.

@joaquimrocha joaquimrocha added this to the v0.27.0 milestone Dec 11, 2024
@sniok
Copy link
Contributor Author

sniok commented Dec 11, 2024

I have rebased these changes but noticed there's a bug related to my previous concerns: if you add content to the creation dialog (from the create button), then you apply and it fails to apply, or you close and reopen that dialog, it will no longer retain the contents that the user has added. This is a UX problem.

So to fix this while keeping your goal of not rendering unless necessary, I added a new commit which only renders the dialog after it's first opened (unless the keepMounted prop is passed, as maybe some plugins do want to pre-render it). I have reverted your commit for the CreateButton, so please test and if it makes sense we just remove the original commit.

There's a simpler fix for that, we can preserve the state of the EditorDialog but also avoid rendering the Dialog by reusing the open prop and returning only inside EditorDialog (previously it would not render EditorDialog completely which would cause the state to be lost). I've tried the scenario you've described and it keeps the content of that popup.

I've dropped the initial commit and added a new commit with this fix.

@joaquimrocha joaquimrocha merged commit d0eea72 into main Dec 11, 2024
17 of 18 checks passed
@joaquimrocha joaquimrocha deleted the page-perf branch December 11, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues related to the frontend performance size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants