Dashboard Performance: Debounce loadWidgets

[gabrieldutra]

What type of PR is this? (check all applicable)

• Other

Description

This PR is basically how I decided to incorporate this:

Also removing the state change on the loadWidget function improved timing substantially (~60% less), so I'll see how to actually incorporate this on code.

From my experiments this was the biggest individual improvement so far.

A few data from Chrome's performance Scripting Time for dashboard loading (you should see a noticeable improvement on dashboards manually too):

Dashboard	master	This PR	% of improvement
Sales Dashboard	4056 ms	1239 ms	69%
Saas Metrics Dashboard	1588 ms	973 ms	38%
Dashboard for testing React migration	42505 ms	16142 ms	62%

(The performance analysis were run with the same time for each dashboard to make sure results more reliable)

Related Tickets & Documents

#4714

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

--

[arikfr]

I can't put my finger on it, but this feels wrong. I feel like we're adding complexity to the implementation that might be avoided with different architecture. Did you understand why debouncing helped here?

Also, it will be interesting to compare scripting time with v7. In one of the other PRs, you mentioned you tested with v7 -- was it using Netlify?

[gabrieldutra]

Did you understand why debouncing helped here?

Yes, because currently the widgets loading triggers a dashboard state change, which makes React recheck all widgets for re-renders. That basically creates a quadratic rise on those recheck numbers when you add new widgets. Debouncing helps because it groups widget state changes, it does not seem ideal, but was the best I thought when having the DashboardPage managing widgets loading.

If each Widget managed its own loading, it would solve the problem as well. I tried to come with solutions on this direction, but it ended up on even worse code as I'd need to access individual rendered widgets to request their loading.

The different architecture that I thought of was, as I said above, having each widget to manage more their own state, and relieve the Dashboard from that. Just didn't continue on it because of such component communication issues. Is this the general lines of the different architecture you were thinking, or was it sth else?

---

Also, it will be interesting to compare scripting time with v7. In one of the other PRs, you mentioned you tested with v7 -- was it using Netlify?

I'm actually running tests locally with webpack-dev, but pointing backend to preview-backend.

[arikfr]

Re V7: here's its Netlify preview: https://deploy-preview-3674--redash-preview.netlify.com/ for easier comparisons in the future.

Re. architecture:

• We need to make sure we're passing relevant state between the components, but beyond that, trying to optimize number of state updates feels wrong.
• If rendering is expensive, we need to make sure it only happens when needed. In theory React takes care of this, but I guess that in the case of visualizations React isn't very effective as it only provides the container and the different libraries (Plotly, D3, Leaflet) take care of rendering.
• If re-check is expensive, we need to understand why and make sure it isn't.

[gabrieldutra]

Re V7: here's its Netlify preview: https://deploy-preview-3674--redash-preview.netlify.com/ for easier comparisons in the future.

👍 The advantage of running locally is to have access to the components and function names for debugging. But in that case I didn't need any of those anyway.

---

I made a few more tests with #4734 with and without the debouncing (since the widgets are memoized, the pain of rerendering reduced considerably):
(Dashboard for testing React Migration)

Even witth that, the scripting time I got was 10s vs 16s (with vs without deboucing). When I open the Profiler to see what could be the reason to that:

The screenshots show the result rendering for a loading widget state change, overall it's not that painful (takes ~20ms), but it scales up with the number of widgets.

Most of those ~20ms are now coming from ReactGridLayout and its components (GridItems). That was a lot worse without memoized widgets, but I don't have a solid idea on how to reduce more the impact of those renderings, hence why I attempted to reduce the number of renders.

[arikfr]

Most of those ~20ms are now coming from ReactGridLayout and its components (GridItems).

RGL suggests to memoize the children. I'm not sure how easy it is given the current implementation.

Another quick-win to consider is to stop passing around dashboard: it seems that we're only using it to open EditParameterMappingsDialog:

redash/client/app/components/dashboards/dashboard-widget/VisualizationWidget.jsx

Lines 223 to 236 in 33131c1

	editParameterMappings = () => {
	const { widget, dashboard, onRefresh, onParameterMappingsChange } = this.props;
	EditParameterMappingsDialog.showModal({
	dashboard,
	widget,
	}).onClose(valuesChanged => {
	// refresh widget if any parameter value has been updated
	if (valuesChanged) {
	onRefresh();
	}
	onParameterMappingsChange();
	this.setState({ localParameters: widget.getLocalParameters() });
	});
	};

If it helps avoid passing dashboard, we can change the pattern here so the callback the widget receives will actually open the dialog (and it will have the dashboard reference) instead of having the open dialog code in the widget implementation.

You can quickly test if this will help by passing null instead of a dashboard object.

Beyond this change which feels quick & easy, let's table this and see how the work you already did affects less busy dashboards.

[gabrieldutra]

RGL suggests to memoize the children. I'm not sure how easy it is given the current implementation.

This was one of my first attempts (#4714 (comment)) 😅, it was actually resulting in more time. Eventually I realized that memoizing the children wouldn't be that relevant (and could indeed add time) as whenever one child element changed, that would trigger it to memoize all over again. Basically, their example is quite simple, ours hold quite a few variables.

Another quick-win to consider is to stop passing around dashboard: it seems that we're only using it to open EditParameterMappingsDialog

I had considered and tested this one too, but the dashboard element actually stopped updating that much when I switched the loadWidget to write in a separate loadingWidgets state instead.

[arikfr]

I had considered and tested this one too, but the dashboard element actually stopped updating that much when I switched the loadWidget to write in a separate loadingWidgets state instead.

I propose this as an alternative to this change (of separate loadingWidget state).

[gabrieldutra]

I propose this as an alternative to this change (of separate loadingWidget state).

Any reason why you want to centralize it in the dashboard state or it's basically for code simplicity?

I changed it because I felt there was no reason to trigger an update for all dashboard dependent code only based on a widget state.

[gabrieldutra]

BTW I tested here, none of those changes alone (remove dashboard attr or separating the loadingWidgets state) actually help on any significant time 😅.

In the context of this PR the loadingWidgets state is necessary. As if we want to deliver #4734 outside of this PR, we will need one of these solutions (dashboard prop removed or loadingWidgets state), otherwise its improvements won't work.

[arikfr]

In the context of this PR the loadingWidgets state is necessary. As if we want to deliver #4734 outside of this PR, we will need one of these solutions (dashboard prop removed or loadingWidgets state), otherwise its improvements won't work.

Just to make sure I understand, in terms of performance loadingWidgets + Memoize == remove dashboard prop + Memoize?

If this is correct, then let's implement removing dashboard prop and memoize. It's a much simpler change.

[gabrieldutra]

If this is correct, then let's implement removing dashboard prop and memoize. It's a much simpler change.

👍. I'll decouple #4734 from this PR

[guidopetri]

@gabrieldutra , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

[gabrieldutra]

@gabrieldutra , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

Assume mostly automated message 😂. But I'll close this PR, it wasn't relevant back then, as I had merged a separate, but equivalent PR. And if it was today, there would be other alternatives I'd invest in for Dashboard frontend performance, which should be better.

[guidopetri]

Indeed :) but I still wanted to be nice!