-
Notifications
You must be signed in to change notification settings - Fork 174
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
There was a problem hiding this 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.
{openDialog && ( | ||
<EditorDialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this logic to the EditorDialog component? So it's optimized whenever used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we should, but let's handle it in a different PR
there are other places already that use the same pattern as here (EditButton.tsx:117 for example)
for now this is the simplest fix but if we're going to modify the component itself then I'd want to make EditorDialog lazy loaded as well
@@ -112,7 +112,7 @@ function SidebarToggleButton() { | |||
); | |||
} | |||
|
|||
function DefaultLinkArea(props: { sidebarName: string; isOpen: boolean }) { | |||
const DefaultLinkArea = memo((props: { sidebarName: string; isOpen: boolean }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? I thought React already only rendered components only if the props change, and there's no complex work in the body of the component AFAICS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React rerenders component if the parent updates, even if the props are the same. https://react.dev/reference/react/memo#skipping-re-rendering-when-props-are-unchanged
Since the props here are super simple, comparing if props are changed is not an expensive operation
const linkArea = useMemo( | ||
() => <DefaultLinkArea sidebarName={sidebar.selected.sidebar || ''} isOpen={isOpen} />, | ||
[sidebar.selected.sidebar, isOpen] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are using a memo here, do we need the memo around the DefaultLinkArea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 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. 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 Style recalculation is taking a big chunk here because certain mui components use functions like So this is not React's fault. Aggresive memoization will help us deal with runtime style generation and heavy mui components. |
This PR adds some useMemo, memo, useCallbacks to avoid unnecessary rerenders. Also hides a create resource popup when it's not open