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

Improve performance of context components re-rendering #3066

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Nov 7, 2024

Fix #3057

  • Refactor setProps reducer to partially updates the layout instead of returning a new layout object.
  • New rendering component, DashWrapper replace TreeContainer.
  • Remove DashContext, the state is now handled in react-redux selectors.

Gif showing only the clicked button is re-rendered:
single-update

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 7, 2024

The test_redraw shows failure with 5 redraw instead of 2.

There is two cases of additional redraw that need to be fixed in this pr:

  • the loading_state selector get triggered. Adding +2 redraw.
  • While the props is the same when returning, the equality check fails for props since it's a new object. Adding +1 redraw.
    • Checking all the props all the time is too expensive.
    • Shallow check will fail for objects that might be simple or complex.

{Array.isArray(layout) ? (
layout.map((c, i) =>
isSimpleComponent(c) ? (
c
) : (
<TreeContainer
<DashWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, new component - is this going to break any legacy code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it shouldn't as this is an internal component.

@@ -1,4 +1,4 @@
type Config = {
export type DashConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for the name change - why does it have to be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in the new DashWrapper.tsx.

@@ -26,6 +26,18 @@ export const apiRequests = [
'loginRequest'
];

function callbackNum(state = 0, action) {
// With the refactor of TreeContainer to DashWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the explanation

@@ -0,0 +1,425 @@
import React, {useMemo, useCallback} from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'm going to trust you on this - I don't know enough of Dash and TypeScript to review something this large. @emilykl can you please have a look? (who else might be a good reviewer?)

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 11, 2024

I think for the additional redraw might be coming from the new path, before it was JSON.stringify(path) and had a memo on that. Think it might need to be back like that or with an additional equalityFn on the memo for the path where it does JSON.stringify to compare the paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new performance something is slow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serious performance issues related to React context
2 participants