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

Serious performance issues related to React context #3057

Open
CNFeffery opened this issue Nov 2, 2024 · 3 comments · May be fixed by #3066
Open

Serious performance issues related to React context #3057

CNFeffery opened this issue Nov 2, 2024 · 3 comments · May be fixed by #3066
Assignees
Labels
P1 needed for current cycle performance something is slow

Comments

@CNFeffery
Copy link
Contributor

When using components associated with the XxxProvider, severe performance issues can arise when there is a large amount of page content. Here are some examples related to well-known component libraries in the Dash ecosystem:

  • with dmc

In dmc, it is required that the application be wrapped inside the MantineProvider. With the React Developer Tools, you can see that any interaction with an internal component will trigger a re-render of all components on the current page.
Image

import dash_mantine_components as dmc
from dash import Dash, _dash_renderer

_dash_renderer._set_react_version("18.2.0")

app = Dash(external_stylesheets=dmc.styles.ALL)

app.layout = dmc.MantineProvider([dmc.Button("test", style={"margin": 5})] * 200)

if __name__ == "__main__":
    app.run(debug=True)

Even placing components from dcc under the MantineProvider will cause the same issue:
Image

import dash_mantine_components as dmc
from dash import Dash, _dash_renderer, dcc

_dash_renderer._set_react_version("18.2.0")

app = Dash(external_stylesheets=dmc.styles.ALL)

app.layout = dmc.MantineProvider([dcc.Input(style={"margin": 5})] * 200)

if __name__ == "__main__":
    app.run(debug=True)
  • with fac

In fac, the similar component AntdConfigProvider is not a must-use, but the same issue will also occur:
Image

import dash
from dash import html
import feffery_antd_components as fac

app = dash.Dash(__name__)

app.layout = html.Div(
    fac.AntdConfigProvider(
        [fac.AntdButton("test", type="primary", style={"margin": 5})] * 100
    )
)

if __name__ == "__main__":
    app.run(debug=True)

However, the issue of global re-rendering does not occur with components within html, such as for html.Div (which has the functionality to update the click event to the component's n_clicks property):

  • with dmc
import dash_mantine_components as dmc
from dash import Dash, _dash_renderer, html

_dash_renderer._set_react_version("18.2.0")

app = Dash(external_stylesheets=dmc.styles.ALL)

app.layout = dmc.MantineProvider(
    [html.Div(style={"height": 25, "border": "1px solid black", "marginBottom": 5})]
    * 100
)

if __name__ == "__main__":
    app.run(debug=True)
  • with fac
import dash
from dash import html
import feffery_antd_components as fac

app = dash.Dash(__name__)

app.layout = html.Div(
    fac.AntdConfigProvider(
        [html.Div(style={"height": 25, "border": "1px solid black", "marginBottom": 5})]
        * 100
    )
)

if __name__ == "__main__":
    app.run(debug=True)

I hope to receive more help on this issue, to explore the deeper reasons and possible solutions.

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 4, 2024

However, the issue of global re-rendering does not occur with components within html, such as for html.Div (which has the functionality to update the click event to the component's n_clicks property):

The onClick is only added if there is an id, if you add an id it trigger the setProps all the time and the context get changed.

I think the culprit is here:

return assocPath(propPath, mergedProps, state);

It's a new layout object on every prop change, the topmost component is thus always updated.

@T4rk1n T4rk1n self-assigned this Nov 4, 2024
@T4rk1n T4rk1n added the P1 needed for current cycle label Nov 4, 2024
@BSd3v
Copy link
Contributor

BSd3v commented Nov 4, 2024

I noticed this back when using dmc 0.12, however, I utilized a workaround that will no longer work due to the dependence of MantineProvider.

Here is an example that is even slower for me:

import dash_mantine_components as dmc
from dash import Dash, _dash_renderer

_dash_renderer._set_react_version("18.2.0")

app = Dash(external_stylesheets=dmc.styles.ALL)

app.layout = dmc.MantineProvider([dmc.Select(style={"margin": 5}, data=['rawr', 'testing'])] * 200)

if __name__ == "__main__":
    app.run(debug=True)

Any selection change takes about 1 second to render.

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 4, 2024

If you change here:

const propPath = append('props', action.payload.itempath);
const existingProps = view(lensPath(propPath), state);
const mergedProps = mergeRight(existingProps, action.payload.props);
return assocPath(propPath, mergedProps, state);

For

const component = path(action.payload.itempath, state);
component.props = mergeRight(component.props, action.payload.props);

Then only the component props will be updated in the redux store. But then the component doesn't update the props since they are passed down from the top components to the bottom, this is how it got the new props.

We'll need to change the way the components get their props by using a selector on the redux state instead of the props being passed down. This can be done like:

const componentProps = useSelector(state => path(concat(componentPath, ["props"]), state.layout));

When the props is updated it trigger the re-render in that selector only for that component. Just need to find a way (or refactor TreeContainer.js entirely) to use those props instead of the props being passed from the top of the tree.

@gvwilson gvwilson added the performance something is slow label Nov 5, 2024
@T4rk1n T4rk1n linked a pull request Nov 7, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 needed for current cycle performance something is slow
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants