Change renderer `setProps` assignation logic

[Marc-Andre-Rivet]

Fixes #40. (Supersedes) Fixes https://github.com/plotly/dash-docs/issues/352. (Supersedes) Fixes https://github.com/plotly/dash-docs/issues/435

Currently setProps is passed to the components only if a component property is used as an input or a state in a callback. This causes two problems:

• components do not update as expected as the props inside the component itself are only updated if setProps exists: here we set the own-props https://github.com/plotly/dash-renderer/blob/91084ce9a86c960e310979ae8a572f83fe21d80b/src/components/core/NotifyObservers.react.js#L39 here we decide if there's a setProps https://github.com/plotly/dash-renderer/blob/91084ce9a86c960e310979ae8a572f83fe21d80b/src/components/core/NotifyObservers.react.js#L86

• the table is in its own special land as it loops back on itself if it doesn't have a setProps (uses its own internal state to update itself) -- which is fine until the following happens: a callback updates the table AND no callback requires a table prop as input / state -- the table now updates itself into its state and receives updates into its props -- for reasons not detailed here, the table merges states unto props, overriding props in the process -- the net result is that the props update from the callbacks are overwritten by the table's state as in https://github.com/plotly/dash-table/issues/386

(the table will still need its loopback with this fix for standalone purposes)

Additionally, the current renderer logic is very permissive as to which props will be sent. If a prop is used as input / state, each time a prop is updated, all updated props will be sent to Dash. For most components the overhead is a minor nuisance but for the table, updating data and sending it back to Dash, if not required, is a major overhead, possibly orders of magnitude larger than the useful data.

This PR proposes a solution to both problems:

• always pass setProps to the component
• on setProps: (1) always update the component itself -- no more stale updates that are impossible to understand for users -- this is seriously probably the most frequent question I answer with "create a bogus callback on your prop as a temporary solution", (2) filter out the props that are not asked for by Dash

Problems that arise from this seem to mostly be related to the way we test..

• [x] since we now update the components correctly all the time, the layout contains additional props, some of which are time sensitive (e.g. n_click_timestamp) -- I've adjusted the tests so as to be less dependent on the layout details
• [x] this needs to be used in tests from dcc, html, dash, table before going forward
• [x] check performance impact (see Chris' comment)

Performance: https://github.com/plotly/dash-renderer/pull/126#issuecomment-469037201

Companion PRs https://github.com/plotly/dash-core-components/pull/478 https://github.com/plotly/dash-html-components/pull/99 https://github.com/plotly/dash-docs/pull/439 https://github.com/plotly/dash-component-boilerplate/pull/65

[alexcjohnson]

This looks like a good idea to me, but I'm not sure I'm aware of all the possible implications. @T4rk1n would you mind taking a look?

[chriddyp]

Only dispatch to Dash if at least one watched prop is updated

I think this implementation makes sense. I originally made "setProps" conditional for dcc.Graph hoverData - if someone was just displaying a graph but not listening to its updates, I didn't want to subscribe the event handler for the graph and have it cause the entire tree to re-render on every hover: https://github.com/plotly/dash-core-components/blob/83a3ea07fd8af9ee3ca57c8da8ed32483e96f87c/src/components/Graph.react.js#L134-L141

In this PR, if you're only re-rendering the component if it is part of a callback, then there shouldn't be any adverse performance effects 👌

---

On another note, from a documentation point of view, we'll want to update our 'react for python devs' guide (https://dash.plot.ly/react-for-python-developers) as well as the boilerplate. Should mostly just be a lot of 🔪 though 😸

[Marc-Andre-Rivet]

setProps being always present triggers a significant number of additional updates for many scenarios, also setProps and loading_states being evaluated on each render creates a new function and new object, forcing re-renders -- modified the PR with a very experimental attempt at limiting the re-renders at the source and simplifying the tree structure / logic -- I'll retag reviewers once I feel I've (1) addressed the performance concerns, (2) verified it works in known scenarios

[Marc-Andre-Rivet]

@chriddyp Using the first example of https://dash.plot.ly/interactive-graphing

Concerning the re-renders, I get the following results for the current 0.19.0 renderer implementation and the implementation in this PR

Version	Hover	Initial renders	Hover renders
0.19.0	Yes	180	+45
0.19.0	No	150	+15
PR	Yes	37	+6
PR	No	33	+2

For the table, using v_be_page.py test implementation, the number of renders per cell modification drops from 4 to 1.

The renders are counted in the NotifyObservers component in 0.19.0 and in the TreeContainer in the PR as with the modifications they play the same role.

[Marc-Andre-Rivet]

Simplified the changes -- since the renders are mostly driven through changes to immutable values in the store, PureComponents + memoization actually provides no benefit.

Prepended renderer centric props with _dashprivate_.

[Marc-Andre-Rivet]

@alexcjohnson I think this is ready for another look. @chriddyp Did you have a chance to try it out against an existing / larger app?

[chriddyp]

did you still want to try it on a big app?

I shared the source of a private large app with @Marc-Andre-Rivet to test things out with. I'm 💃 either way.

Also note that there are a few places in the documentation that we should update once this is released:

• dash component boilerplate repo - the sample example
• the react for python devs tutorial on dash-docs
• we might want to make some issues in our other component repos to remove setProps just to be internally consistent
• also this could be closed https://github.com/plotly/dash-docs/issues/352
• and this is partially addressed https://github.com/plotly/dash-docs/issues/435

[Marc-Andre-Rivet]

@alexcjohnson @chriddyp

Did some actual performance tests, mostly against the dash-docs. Got the demo app working partially but against the latest version of dash but there were still multiple issues, fixed at lot of them and got the code running but in a weird state -- and not sure how significant the results really are.

In all cases, opening a new browser window for each version, the runs are all in the same tab. Version ordering changes randomly. One warm up run prior to taking results.

In the docs' dcc page, simply reloading the page gave the following results in milliseconds for 5 reloads: Dash 0.39: 6046, 6228, 6135 Modified: 4667, 4653,4635 The sample is small but ~25% difference on 15 runs is probably significant.

Table paging page, clicking prev/nex 10 times each, in milliseconds: Dash 0.39: 3398, 3421, 3223 Modified: 637, 640, 604 5-to-1 difference... this might be partially due to no callback requiring data

Table editing, modifying 10 cells per run, in milliseconds: Dash 0.39: 2647, 2418, 2439 Modified: 1059, 1109, 1113 2-to-1 difference

The Graph example w/ hover, 20 hovers, in milliseconds: Dash 0.39: 833, 385, 512 Modified: 671, 750, 588 Nothing significant with this set -- the variability is high.. could go either way I guess -- could do more runs to make sure.

The Graph example w/ hover, 5 page loads, in milliseconds: Dash 0.39: 1996, 2006, 2104 Modified: 2037, 2073, 2009 Nothing significant with this set

It seems that at worse this has no impact for certain components (e.g. Graph) and that at best, for certain interactions the performance impact can be very significant.

[chriddyp]

Nice, those look encouraging. Another page I just thought of is https://dash.plot.ly/all. It's a "hidden" page that we used to generate a PDF version of the docs. It loads all of the pages in the docs at once (takes like 40 seconds to load), probably the most render intensive page that I can think of.

[Marc-Andre-Rivet]

Dry run from a new tab of /all gave:

Dash 0.39: "Updating..." disappeared after ~ 229 seconds Dev tools crashed when trying to load the performance analysis 😁

Modified: "Updating..." disappeared after 17.2, 17.4 and 18.1 seconds

[Marc-Andre-Rivet]

2 step merge -- will remove the refs for the feature branches of dcc and html right after tests pass everywhere