Dashboard Performance: Memoize widgets #4734
Conversation
| @@ -33,6 +33,58 @@ const WidgetType = PropTypes.shape({ | |||
| const SINGLE = "single-column"; | |||
| const MULTI = "multi-column"; | |||
|
|
|||
| function DashboardWidget({ | |||
There was a problem hiding this comment.
Wouldn't it be simpler to just use React.memo here?
| @@ -33,6 +33,45 @@ const WidgetType = PropTypes.shape({ | |||
| const SINGLE = "single-column"; | |||
| const MULTI = "multi-column"; | |||
|
|
|||
| const DashboardWidget = React.memo(function DashboardWidget({ | |||
There was a problem hiding this comment.
I assume the performance issue isn't with the RestrictedWidget or TextboxWidget (correct me if I wrong), but with visualization rendering. Maybe we should actually memoize one of the more basic components down the stream? This will later be useful in other places we render visualizations (query page).
There was a problem hiding this comment.
Why not both? 😁
I actually wanted to memoize as high as possible in the tree, thus shaving off more time.
There was a problem hiding this comment.
Because memoization can have unwanted side effects that we rather avoid. The assumptions I'm working with are: 1) for the most part re-rendering is a cheap operation in React; 2) in many cases even when you re-render React won't in fact do anything because the DOM stays the same.
For this reason I would rather keep memoization for places it's really needed.
If you prefer, we can merge this as is and explore memoization of a different component as a follow up.
There was a problem hiding this comment.
That's kinda of why I wanted to have more testing time to it, but if it ended up being safe to memoize in an Upper level, there should be no problem to memoize in a lower one as well.
From my understanding it's a matter of being able to determine a component from its props/state, so if the tree was somehow composed by immutable objects (or basic variable props), it should be fine to use React.memo.
But I do understand your concern and also share it, that's the reason I was targeting #4724 and #4726 to v9 and left away this one. Since this PR is targeted to #4724 and not master, and I am using it for results comparison (mainly for alternatives we are discussing in #4724), let's leave this PR here and I'll compare how it differs from memoizing the visualization.
There was a problem hiding this comment.
As I imagined, memoizing the Visualization instead of the whole widget helps, but not quite as much:
| Dashboard | master | Memoized Widgets | Memoized Visualizations |
|---|---|---|---|
| Dashboard for testing React migration | 38s (~) | 20s (-47%) | 30s (-21%) |
What happens is that it doesn't really seem that are the visualizations that are slowing down the dashboards, but it rather seem a combination of:
- re-render numbers; and
- number of components down the stream.
The re-render process is mostly a cheap operation, as you said (<1ms), but it currently scales.
In brief, #4724 is an attempt to solve 1 and this PR is an attempt to solve 2.
A few screenshots to illustrate how memoizing visualizations differs from memoizing widgets:
Basically re-rendering a Visualization seems not that expensive as well, it mostly helped here because we reduced part of 2.
gabrieldutra
force-pushed
the
dashboard-performance--memoize-widgets
branch
from
4e608d9 to
8469458
Compare
gabrieldutra
changed the base branch from
dashboard-performance--debounce-load-widgets
to
master
| (prevProps, nextProps) => | ||
| prevProps.widget === nextProps.widget && | ||
| prevProps.canEdit === nextProps.canEdit && | ||
| prevProps.isPublic === nextProps.isPublic && | ||
| prevProps.isLoading === nextProps.isLoading && | ||
| prevProps.filters === nextProps.filters | ||
| ); |
There was a problem hiding this comment.
I didn't directly removed the dashboard prop from the widget (didn't seem that easy), but I selected what props will trigger the rerender instead.
There was a problem hiding this comment.
Is filters prop immutable?
Why move isLoading out of widget?
There was a problem hiding this comment.
Is filters prop immutable?
From my understanding, yes, it's defined in a state every time loadDashboard and the dashboard doesn't mutate it. I quickly opened a dashboard with filters and looks like it's working.
I think I'll push a PR with some Cypress tests for filters. It should be a good thing , since we don't test Filters all the time.
Why move isLoading out of widget?
This leads to the one that most concerns me and the reason to that: the widget isn't an immutable prop, hence why I moved isLoading out of widget. We can have a mutable prop, but we need to make sure that all render dependent props trigger a change when they should. And that's mostly what concerns me about testing this PR. From my tests and from existing Cypress coverage, the isLoading looks like the only one.
What type of PR is this? (check all applicable)
Description
The idea of this PR is to handle this:
Basically, whenever a widget updates it updates the Dashboard and all children components get re-rendered. This PR memoizes widgets and only update them when one of their props change (need to test a bit more to make sure it does not blow anything). While I also wanted to memoize the base child from ReactGridLayout and avoid the whole grid item to re-render, I couldn't find a working way to do that.
For smaller dashboards I didn't see a relevant difference in the scripting time, but for the React Migration dashboard it did shave off extra 2 seconds in the Scripting Time (14s to 12s avg, tested with #4724, #4726 and this PR changes together).
Related Tickets & Documents
#4714
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
--