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.

Sign up for GitHub

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

Dashboard Performance: Memoize widgets #4734

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Dashboard Performance: Memoize widgets #4734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 70 additions & 32 deletions client/app/components/dashboards/DashboardGrid.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,53 @@ const WidgetType = PropTypes.shape({
const SINGLE = "single-column";
const MULTI = "multi-column";

const DashboardWidget = React.memo(
function DashboardWidget({
widget,
dashboard,
onLoadWidget,
onRefreshWidget,
onRemoveWidget,
onParameterMappingsChange,
canEdit,
isPublic,
isLoading,
filters,
}) {
const { type } = widget;
const onLoad = () => onLoadWidget(widget);
const onRefresh = () => onRefreshWidget(widget);
const onDelete = () => onRemoveWidget(widget.id);

if (type === WidgetTypeEnum.VISUALIZATION) {
return (
<VisualizationWidget
widget={widget}
dashboard={dashboard}
filters={filters}
canEdit={canEdit}
isPublic={isPublic}
isLoading={isLoading}
onLoad={onLoad}
onRefresh={onRefresh}
onDelete={onDelete}
onParameterMappingsChange={onParameterMappingsChange}
/>
);
}
if (type === WidgetTypeEnum.TEXTBOX) {
return <TextboxWidget widget={widget} canEdit={canEdit} isPublic={isPublic} onDelete={onDelete} />;
}
return <RestrictedWidget widget={widget} />;
},
(prevProps, nextProps) =>
prevProps.widget === nextProps.widget &&
prevProps.canEdit === nextProps.canEdit &&
prevProps.isPublic === nextProps.isPublic &&
prevProps.isLoading === nextProps.isLoading &&
prevProps.filters === nextProps.filters
);
Comment on lines +75 to +81
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is filters prop immutable?
Why move isLoading out of widget?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class DashboardGrid extends React.Component {
static propTypes = {
isEditing: PropTypes.bool.isRequired,
Expand Down Expand Up @@ -203,38 +250,29 @@ class DashboardGrid extends React.Component {
onLayoutChange={this.onLayoutChange}
onBreakpointChange={this.onBreakpointChange}
breakpoints={{ [MULTI]: cfg.mobileBreakPoint, [SINGLE]: 0 }}>
{widgets.map(widget => {
const widgetProps = {
widget,
filters,
isPublic,
canEdit: dashboard.canEdit(),
onDelete: () => onRemoveWidget(widget.id),
};
const { type } = widget;
return (
<div
key={widget.id}
data-grid={DashboardGrid.normalizeFrom(widget)}
data-widgetid={widget.id}
data-test={`WidgetId${widget.id}`}
className={cx("dashboard-widget-wrapper", {
"widget-auto-height-enabled": this.autoHeightCtrl.exists(widget.id),
})}>
{type === WidgetTypeEnum.VISUALIZATION && (
<VisualizationWidget
{...widgetProps}
dashboard={dashboard}
onLoad={() => onLoadWidget(widget)}
onRefresh={() => onRefreshWidget(widget)}
onParameterMappingsChange={onParameterMappingsChange}
/>
)}
{type === WidgetTypeEnum.TEXTBOX && <TextboxWidget {...widgetProps} />}
{type === WidgetTypeEnum.RESTRICTED && <RestrictedWidget widget={widget} />}
</div>
);
})}
{widgets.map(widget => (
<div
key={widget.id}
data-grid={DashboardGrid.normalizeFrom(widget)}
data-widgetid={widget.id}
data-test={`WidgetId${widget.id}`}
className={cx("dashboard-widget-wrapper", {
"widget-auto-height-enabled": this.autoHeightCtrl.exists(widget.id),
})}>
<DashboardWidget
dashboard={dashboard}
widget={widget}
filters={filters}
isPublic={isPublic}
isLoading={widget.loading}
canEdit={dashboard.canEdit()}
onLoadWidget={onLoadWidget}
onRefreshWidget={onRefreshWidget}
onRemoveWidget={onRemoveWidget}
onParameterMappingsChange={onParameterMappingsChange}
/>
</div>
))}
</ResponsiveGridLayout>
</div>
);
Expand Down
10 changes: 6 additions & 4 deletions client/app/components/dashboards/dashboard-widget/VisualizationWidget.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ class VisualizationWidget extends React.Component {
dashboard: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types
filters: FiltersType,
isPublic: PropTypes.bool,
isLoading: PropTypes.bool,
canEdit: PropTypes.bool,
onLoad: PropTypes.func,
onRefresh: PropTypes.func,
Expand All @@ -197,6 +198,7 @@ class VisualizationWidget extends React.Component {
static defaultProps = {
filters: [],
isPublic: false,
isLoading: false,
canEdit: false,
onLoad: () => {},
onRefresh: () => {},
Expand Down Expand Up @@ -236,8 +238,8 @@ class VisualizationWidget extends React.Component {
};

renderVisualization() {
const { widget, filters } = this.props;
const widgetQueryResult = widget.getQueryResult();
const { isLoading, widget, filters } = this.props;
const widgetQueryResult = !isLoading && widget.getQueryResult();
const widgetStatus = widgetQueryResult && widgetQueryResult.getStatus();
switch (widgetStatus) {
case "failed":
Expand Down Expand Up @@ -273,10 +275,10 @@ class VisualizationWidget extends React.Component {
}

render() {
const { widget, isPublic, canEdit, onRefresh } = this.props;
const { widget, isLoading, isPublic, canEdit, onRefresh } = this.props;
const { localParameters } = this.state;
const widgetQueryResult = widget.getQueryResult();
const isRefreshing = widget.loading && !!(widgetQueryResult && widgetQueryResult.getStatus());
const isRefreshing = isLoading && !!(widgetQueryResult && widgetQueryResult.getStatus());

return (
<Widget
Expand Down
27 changes: 15 additions & 12 deletions client/app/pages/dashboards/hooks/useDashboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useState, useEffect, useMemo, useCallback } from "react";
import { useState, useEffect, useMemo, useCallback, useRef } from "react";
import { isEmpty, includes, compact, map, has, pick, keys, extend, every, get } from "lodash";
import notification from "@/services/notification";
import location from "@/services/location";
Expand Down Expand Up @@ -100,28 +100,31 @@ function useDashboard(dashboardData) {

const refreshWidget = useCallback(widget => loadWidget(widget, true), [loadWidget]);

const removeWidget = useCallback(
widgetId => {
dashboard.widgets = dashboard.widgets.filter(widget => widget.id !== undefined && widget.id !== widgetId);
setDashboard(currentDashboard => extend({}, currentDashboard));
},
[dashboard]
);
const removeWidget = useCallback(widgetId => {
setDashboard(currentDashboard =>
extend({}, currentDashboard, {
widgets: currentDashboard.widgets.filter(widget => widget.id !== undefined && widget.id !== widgetId),
})
);
}, []);

const dashboardRef = useRef();
dashboardRef.current = dashboard;

const loadDashboard = useCallback(
(forceRefresh = false, updatedParameters = []) => {
const affectedWidgets = getAffectedWidgets(dashboard.widgets, updatedParameters);
const affectedWidgets = getAffectedWidgets(dashboardRef.current.widgets, updatedParameters);
const loadWidgetPromises = compact(
affectedWidgets.map(widget => loadWidget(widget, forceRefresh).catch(error => error))
);

return Promise.all(loadWidgetPromises).then(() => {
const queryResults = compact(map(dashboard.widgets, widget => widget.getQueryResult()));
const updatedFilters = collectDashboardFilters(dashboard, queryResults, location.search);
const queryResults = compact(map(dashboardRef.current.widgets, widget => widget.getQueryResult()));
const updatedFilters = collectDashboardFilters(dashboardRef.current, queryResults, location.search);
setFilters(updatedFilters);
});
},
[dashboard, loadWidget]
[loadWidget]
);

const refreshDashboard = useCallback(
Expand Down