Improve performance of context components re-rendering #3066

T4rk1n · 2024-11-07T20:48:08Z

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:

T4rk1n · 2024-11-07T22:07:59Z

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.

gvwilson · 2024-11-11T14:56:15Z

dash/dash-renderer/src/APIController.react.js

                {Array.isArray(layout) ? (
                    layout.map((c, i) =>
                        isSimpleComponent(c) ? (
                            c
                        ) : (
-                            <TreeContainer
+                            <DashWrapper


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

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

gvwilson · 2024-11-11T14:57:10Z

dash/dash-renderer/src/config.ts

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


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

It is used in the new DashWrapper.tsx.

gvwilson · 2024-11-11T14:58:37Z

dash/dash-renderer/src/reducers/reducer.js

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

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


thank you for the explanation

gvwilson · 2024-11-11T14:59:37Z

dash/dash-renderer/src/wrapper/DashWrapper.tsx

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


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 · 2024-11-11T15:29:56Z

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.

NOTMEE12 · 2024-11-28T17:56:09Z

Hi. Are there any updates on this?

AnnMarieW · 2024-12-09T17:33:58Z

@T4rk1n

I ran the dmc-docs using this branch, and it's much faster! Thanks so much for doing this PR. This will make large apps using DMC perform much better.

On the dev branch this app works fine, but on this branch, a component sent to the icon prop throws errors.

import dash_mantine_components as dmc
from dash import Dash, _dash_renderer
_dash_renderer._set_react_version("18.2.0")
from dash_iconify import DashIconify
app = Dash(external_stylesheets=dmc.styles.ALL)



app.layout = dmc.MantineProvider(
    dmc.Checkbox(
            label="Custom checked icon",
            checked=True,
            icon=DashIconify(icon="ion:bag-check-sharp"),
            size="lg",
        )
)

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

Here's one of the error messages (There are a few more as well)

Uncaught TypeError: Cannot read properties of undefined (reading 'props')
    at n.47474.s.default.createElement.r.icon (dash_mantine_components.v0_15_1m1733503561.js:2:1076461)
    at renderWithHooks ([email protected]_18_2m1733760928.2.0.js:16315:20)
    at mountIndeterminateComponent ([email protected]_18_2m1733760928.2.0.js:20084:15)
    at beginWork ([email protected]_18_2m1733760928.2.0.js:21597:18)
    at HTMLUnknownElement.callCallback ([email protected]_18_2m1733760928.2.0.js:4151:16)
    at Object.invokeGuardedCallbackDev ([email protected]_18_2m1733760928.2.0.js:4200:18)
    at invokeGuardedCallback ([email protected]_18_2m1733760928.2.0.js:4264:33)
    at beginWork$1 ([email protected]_18_2m1733760928.2.0.js:27461:9)
    at performUnitOfWork ([email protected]_18_2m1733760928.2.0.js:26567:14)
    at workLoopSync ([email protected]_18_2m1733760928.2.0.js:26476:7)

Update

It's not just the DashIconify library. It's not possible to pass any components to the icon prop:

import dash_mantine_components as dmc
from dash import Dash, _dash_renderer, html
_dash_renderer._set_react_version("18.2.0")

FONT_AWESOME = "https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.0/css/all.min.css"

app = Dash(external_stylesheets=[FONT_AWESOME])

app.layout = dmc.MantineProvider([
    html.I(className="fa-solid fa-bag-shopping fa-3x"),
    dmc.Checkbox(
            label="Custom checked icon",
            checked=True,
            icon=html.I(className="fa-solid fa-bag-shopping fa-3x"),
            size="lg",
        )
])


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

T4rk1n · 2024-12-09T18:54:28Z

@AnnMarieW I removed the _dashprivate_layout from the given props, it's a "private" props and shouldn't have been used outside of the renderer.

https://github.com/snehilvj/dash-mantine-components/blob/529231820bc2ca25db396cb35d637618b6df62cc/src/ts/components/core/checkbox/Checkbox.tsx#L66

AnnMarieW · 2024-12-09T19:13:10Z

@T4rk1n Thanks for your speedy response. I updated to use React.cloneElement instead and it works fine 🎉

Update: Actually it doesn't work 😢 It doesn't throw errors, but it doesn't render correctly 🤔

alexcjohnson · 2024-12-10T06:26:08Z

There's one other user of _dashprivate_layout in this repo (dcc.ConfirmDialogProvider), and it's also used by both DDK and DBE https://github.com/search?q=org%3Aplotly+_dashprivate_layout&type=code, in addition to DBC and as @AnnMarieW points out DMC. @T4rk1n is removing this necessary to the performance improvements? If not, it seems like we really shouldn't tamper with it. But if it is necessary, we'll need to come up with an alternate pattern that will work for our own uses of it, and then give these other libraries time and assistance to adopt the pattern as well.

T4rk1n · 2024-12-10T14:01:23Z

I removed the _dashprivate props because it was not necessary anymore in the wrapper props, still think it was a mistake to include them in given props with a name like that when it should be part of the API. While the performance impact isn't that much to add it back, it does contains the whole layout starting from the component, duplicated for each components the memory footprint is non-negligible for just a few usecases.

The proper way to handle this kind of pattern (send data up/down stream the component tree) in react is with context components, which this PR fixes their performance.

AnnMarieW · 2024-12-10T16:10:25Z

@T4rk1n

Thanks for the suggestion of using context instead. We are using the dashprivate props in multiple components in DMC, so this will take a while to update.

You make a good point here:

contains the whole layout starting from the component, duplicated for each components the memory footprint is non-negligible for just a few usecases.

Given that this PR substantially increases performance, would you consider keeping the dashprivate props for now, and removing in a future update?

T4rk1n · 2024-12-10T17:39:31Z

Given that this PR substantially increases performance, would you consider keeping the dashprivate props for now, and removing in a future update?

Yes, I'll put it back since it's used everywhere. There could be an alternative get_props on the clientside that would return the raw props in a future update before deprecating and removing this dashprivate api.

AnnMarieW · 2024-12-10T18:34:24Z

This would be awesome:

There could be an alternative get_props on the clientside that would return the raw props in a future update before deprecating and removing this dashprivate api.

❤️ ❤️

T4rk1n · 2024-12-11T16:25:29Z

Not sure I can add back those props in the way it was, there was another component in between the TreeContainer and the library component that is not longer the case. Might need to refactor the new Wrapper to have a middle component but that changes the order of renders and is not as optimal as a single component since it add overhead.

T4rk1n · 2024-12-11T16:37:20Z

And I think now with this to access the child props the new path would be just without a level and the _dashprivate_layout.props.
So instead of child.props._dashprivate_layout.props you would use child.props directly.

T4rk1n · 2024-12-11T17:08:40Z

And I think now with this to access the child props the new path would be just without a level and the _dashprivate_layout.props. So instead of child.props._dashprivate_layout.props you would use child.props directly.

Actually, the props are not there anymore in the wrapper, they are now gotten from the selector, so the old hack cannot work anymore with this solution.

AnnMarieW · 2024-12-11T17:19:44Z

so the old hack cannot work anymore with this solution.

Is there a "new hack" that might be easier than using context?

T4rk1n · 2024-12-11T18:24:56Z

so the old hack cannot work anymore with this solution.

Is there a "new hack" that might be easier than using context?

A Context solution might only be valid in some cases, for the mantine checkbox case, it want to add extra props to the component.

I see a couple solution for those cases:

Could add extraProps to DashWrapper props that would be merged with the component props from the store, this would make React.cloneElement(child, {extraProps: {selected: true}}) to work.
change the clientside set_props to accept a path instead of an id. Then you can use children[0]._dashprivate_path to add new props to the components.

For accessing the props (dcc.Tabs cases), a new get_props(path) could be added or refactor the components to use a context.

T4rk1n · 2024-12-12T20:40:05Z

@AnnMarieW I added all three solutions from the last post.

Added extras prop to DashWrapper component, think this one could replace dmc checkbox:

const iconFunc = ({ indeterminate, ...others }) => {
    const selected: any = indeterminate ? indeterminateIcon : icon;
    return React.cloneElement(selected, {extras: others});
};

set_props can take a dash path instead of id.
Added get_props(pathOrId), to get the props of a component. The path is available in children[0].props.componentPath.

Let me know if that work.

AnnMarieW · 2024-12-12T20:52:29Z

Thanks for the update @T4rk1n I'll try to figure out how to use these new features.

Will it handle this pattern too? https://github.com/snehilvj/dash-mantine-components/blob/83a1cc12e5e6b210b7a1e27c7a83068abe2830c5/src/ts/components/core/timeline/Timeline.tsx#L46

T4rk1n · 2024-12-12T21:37:21Z

Thanks for the update @T4rk1n I'll try to figure out how to use these new features.

Will it handle this pattern too? https://github.com/snehilvj/dash-mantine-components/blob/83a1cc12e5e6b210b7a1e27c7a83068abe2830c5/src/ts/components/core/timeline/Timeline.tsx#L46

For that one can use window.dash_clientside.get_props(child.props.componentProps)

AnnMarieW · 2024-12-13T17:03:03Z

@T4rk1n

Do you know how to get the component type as used here in in this Stepper component

 const childType = child.props._dashprivate_layout.type;
 if (childType === "StepperCompleted") {
      ....

BSd3v · 2024-12-13T17:50:30Z

@T4rk1n,

Hmm... would it be possible to change it from get_props to get_attributes? Where it could return type, namespace, props and if you wanted to, you could target props just like how you are targeting the value in the one component?

T4rk1n · 2024-12-13T19:03:23Z

I changed get_props to get_layout to match the previous _dashprivate_layout it returns type, namespace, props. Can also concat the path to get props values, eg: [...child.props.componentPath, 'props', 'value']

AnnMarieW · 2024-12-14T21:29:11Z

@T4rk1n

It looks like dcc.Loading is not working with this update .

Here's the first example from the docs - loading spinners don't show up

from dash import Dash, dcc, html, Input, Output, callback
import time

app = Dash()

app.layout = html.Div([
    html.H3("Edit text input to see loading state"),
    html.Div("Input triggers local spinner"),
    dcc.Input(id="loading-input-1"),
    dcc.Loading(
        id="loading-1",
        type="default",
        children=html.Div(id="loading-output-1")
    ),
    html.Div([
        html.Div('Input triggers nested spinner'),
        dcc.Input(id="loading-input-2"),
        dcc.Loading(
            id="loading-2",
            children=[html.Div([html.Div(id="loading-output-2")])],
            type="circle",
        )
    ]),
])


@callback(Output("loading-output-1", "children"), Input("loading-input-1", "value"))
def input_triggers_spinner(value):
    time.sleep(1)
    return value


@callback(Output("loading-output-2", "children"), Input("loading-input-2", "value"))
def input_triggers_nested(value):
    time.sleep(1)
    return value


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

emilhe · 2024-12-16T06:41:02Z

@T4rk1n In some cases, it is desireable (necessary?) to render other Dash components beyond what is passed to the children property. A example is icon propety in dmc, where a component is to be rendered (including callback bindings) similar to the children property. As I see it, we need two pieces of functionality,

To be able to lookup a component type
To be able to render a component (given type and props), including callback bindings

It seems that the proposed get_layout function satisfies (1). I wrote renderDashComponents to provide (2), but I am not sure if there are any other utility functions available providing this functionality?

As a last resort, I guess I could re-write the renderDashComponents function in dash-extensions-js to adopt the new syntax, but I would of course prefer an official, non-private API based solution if possible :)

T4rk1n · 2024-12-16T14:18:53Z

It looks like dcc.Loading is not working with this update .

Yes, the loading state stuff incurred two extra renders so I had merged the loading_state selector with the props but the equality function still need to be adapted or something, working on that.

T4rk1n · 2024-12-16T14:25:26Z

To be able to render a component (given type and props), including callback bindings

I don't think this is a good pattern, the components are already hydrated, they only need to be mounted for the rendering. Can now add props set_props(pathOrId, payload) or React.cloneElement(dashComponent, {extras: {style: {background: 'red'}}). The path would now be available at component.props.componentPath.

The renderDashComponent does not return a Wrapper or previously TreeContainer and may be disconnected from the store.

T4rk1n · 2024-12-16T14:51:15Z

I guess we could add a window.dash_clientside.render(path: DashPath, component: DashComponent): DashWrapper to be able to add children from components. But it shouldn't be used to replace the already rendered components that comes in the props.

I guess this is a limitation of the layout stuff returning raw components, I'll see if it can return hydrated component instead.

AnnMarieW · 2024-12-16T15:57:29Z

@T4rk1n

In DMC PR # 458 I've used the new features you added to eliminate all the _dashprivate_layout from DMC. Works great - Thanks! 🎉

DMC is using Emil's renderDashComponets function in these 4 components in case you wanted to see a use-case.

Is it OK to continue to use _dashprivate_pushstate ? We use it in dmc.Anchor so it works like dcc.Link. It's also used in these components in the Dash Bootstrap Components library.

T4rk1n · 2024-12-16T16:25:22Z

Is it OK to continue to use _dashprivate_pushstate ?

@AnnMarieW Yea that is just for the location component, it shouldn't have been prefixed with _dashprivate and a better name but it's unlikely to change for now.

AnnMarieW · 2024-12-19T21:07:36Z

@T4rk1n
Does this mean that every component that adds data-dash-is-loading needs to be updated too?
https://github.com/plotly/dash/blob/dev/components/dash-core-components/src/fragments/Graph.react.js#L523

T4rk1n · 2024-12-19T21:26:29Z

@T4rk1n Does this mean that every component that adds data-dash-is-loading needs to be updated too? https://github.com/plotly/dash/blob/dev/components/dash-core-components/src/fragments/Graph.react.js#L523

Yes, if it's really needed, there will be ctx.isLoading and ctx.useLoading() as replacement.

AnnMarieW · 2024-12-19T21:40:37Z

Yes, if it's really needed, there will be ctx.isLoading and ctx.useLoading() as replacement.

Could you show an example of how to update the dcc.Graph component? It would be good to still enabling styling a component that's loading via CSS as described on at the end of this page: https://dash.plotly.com/loading-states

T4rk1n · 2024-12-20T14:50:52Z

@AnnMarieW For class component we can replace the div with data-dash-is-loading with a component like this one:

import React from 'react';
import PropTypes from 'prop-types';

export default function LoadingDiv({children, loading_state, ...props}) {
    let loading;
    if (window.dash_component_api) {
        const ctx = window.dash_component_api.useDashContext();
        loading = ctx.useLoading();
    } else {
        loading = loading_state?.is_loading;
    }
    
    return (
        <div {...props} data-dash-is-loading={loading || undefined}>
            {children}
        </div>
    );
}

This would also be backward compatible with the old loading_state.

AnnMarieW · 2024-12-20T17:44:15Z

Hi @T4rk1n

I see the new LoadingElement - that looks great! Will it work with elements like a Mantine component rather than a standard jsx component? It causes some problems if a Mantine component is wrapped in an extra div as described in issue 463

T4rk1n · 2024-12-20T19:58:54Z

Hi @T4rk1n

I see the new LoadingElement - that looks great! Will it work with elements like a Mantine component rather than a standard jsx component? It causes some problems if a Mantine component is wrapped in an extra div as described in issue 463

If the component accept data-dash-is-loading and pass it to it's dom node can use const loading = ctx.useLoading() in function components.

Improve performance of context components re-rendering

8a5ab9a

T4rk1n requested review from alexcjohnson, ndrezn, gvwilson and emilykl as code owners November 7, 2024 20:48

Fix list as layout

378fec3

AnnMarieW mentioned this pull request Nov 8, 2024

Performance issues snehilvj/dash-mantine-components#390

Open

gvwilson added performance something is slow feature something new labels Nov 11, 2024

gvwilson assigned T4rk1n Nov 11, 2024

gvwilson approved these changes Nov 11, 2024

View reviewed changes

AnnMarieW mentioned this pull request Dec 10, 2024

Stop using dashprivate props snehilvj/dash-mantine-components#453

Open

memo wrapper, merge loading state selector, add back _dashprivate_layout

65ffabd

Add get_props, extras to wrapper, set_props takes path

011ded9

AnnMarieW mentioned this pull request Dec 12, 2024

Remove dashprivate props snehilvj/dash-mantine-components#458

Draft

8 tasks

get_props -> get_layout

603cbce

T4rk1n added 2 commits December 16, 2024 15:02

take loading_state out of selector

e1cbc68

Rework loading component

5424fe0

T4rk1n added 3 commits December 19, 2024 16:43

Add useLoading

1a56d4f

useLoading in html components

1f56612

Add LoadingDiv

9741a69

replace loading for data-dash-is-loading

14c5028

T4rk1n force-pushed the fix/wrapper branch from ac1103a to 14c5028 Compare December 20, 2024 17:20

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

Improve performance of context components re-rendering #3066

Are you sure you want to change the base?

Improve performance of context components re-rendering #3066

Conversation

T4rk1n commented Nov 7, 2024

T4rk1n commented Nov 7, 2024

gvwilson Nov 11, 2024

Choose a reason for hiding this comment

T4rk1n Nov 11, 2024

Choose a reason for hiding this comment

gvwilson Nov 11, 2024

Choose a reason for hiding this comment

T4rk1n Nov 11, 2024

Choose a reason for hiding this comment

gvwilson Nov 11, 2024

Choose a reason for hiding this comment

gvwilson Nov 11, 2024

Choose a reason for hiding this comment

T4rk1n commented Nov 11, 2024

NOTMEE12 commented Nov 28, 2024

AnnMarieW commented Dec 9, 2024 • edited Loading

Update

T4rk1n commented Dec 9, 2024

AnnMarieW commented Dec 9, 2024 • edited Loading

alexcjohnson commented Dec 10, 2024

T4rk1n commented Dec 10, 2024

AnnMarieW commented Dec 10, 2024

T4rk1n commented Dec 10, 2024

AnnMarieW commented Dec 10, 2024

T4rk1n commented Dec 11, 2024 • edited Loading

T4rk1n commented Dec 11, 2024 • edited Loading

T4rk1n commented Dec 11, 2024

AnnMarieW commented Dec 11, 2024

T4rk1n commented Dec 11, 2024 • edited Loading

T4rk1n commented Dec 12, 2024

AnnMarieW commented Dec 12, 2024

T4rk1n commented Dec 12, 2024

AnnMarieW commented Dec 13, 2024 • edited Loading

BSd3v commented Dec 13, 2024 • edited Loading

T4rk1n commented Dec 13, 2024

AnnMarieW commented Dec 14, 2024

emilhe commented Dec 16, 2024 • edited Loading

T4rk1n commented Dec 16, 2024

T4rk1n commented Dec 16, 2024

T4rk1n commented Dec 16, 2024 • edited Loading

AnnMarieW commented Dec 16, 2024

T4rk1n commented Dec 16, 2024

AnnMarieW commented Dec 19, 2024

T4rk1n commented Dec 19, 2024

AnnMarieW commented Dec 19, 2024

T4rk1n commented Dec 20, 2024 • edited Loading

AnnMarieW commented Dec 20, 2024

T4rk1n commented Dec 20, 2024

AnnMarieW commented Dec 9, 2024 •

edited

Loading

AnnMarieW commented Dec 9, 2024 •

edited

Loading

T4rk1n commented Dec 11, 2024 •

edited

Loading

T4rk1n commented Dec 11, 2024 •

edited

Loading

T4rk1n commented Dec 11, 2024 •

edited

Loading

AnnMarieW commented Dec 13, 2024 •

edited

Loading

BSd3v commented Dec 13, 2024 •

edited

Loading

emilhe commented Dec 16, 2024 •

edited

Loading

T4rk1n commented Dec 16, 2024 •

edited

Loading

T4rk1n commented Dec 20, 2024 •

edited

Loading