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.

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

perf(dashboard): reduce number of rerenders of Charts #16444

Merged
merged 3 commits into from
Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions superset-frontend/src/dashboard/actions/hydrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,8 @@ export const hydrateDashboard = (dashboardData, chartData) => (
// only persistent refreshFrequency will be saved to backend
shouldPersistRefreshFrequency: false,
css: dashboardData.css || '',
colorNamespace: metadata?.color_namespace || null,
colorScheme: metadata?.color_scheme || null,
colorNamespace: metadata?.color_namespace,
colorScheme: metadata?.color_scheme,
editMode: canEdit && editMode,
isPublished: dashboardData.published,
hasUnsavedChanges: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ const defaultProps = {
// resizing across all slices on a dashboard on every update
const RESIZE_TIMEOUT = 350;
const SHOULD_UPDATE_ON_PROP_CHANGES = Object.keys(propTypes).filter(
prop => prop !== 'width' && prop !== 'height',
prop =>
prop !== 'width' && prop !== 'height' && prop !== 'isComponentVisible',

Choose a reason for hiding this comment

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

Thanks for the work! this perf improvement work is great!
Before: chart component not update when width or height changed,
after: chart component not update width or height or visibility change.
But when chart component hide and show when tab changes, why chart component not need update?

Copy link
Member Author

Choose a reason for hiding this comment

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

In shouldComponentUpdate we have a if(isComponentVisible) statement followed by additional checks if component should render. The point is that we run those only when component is visible.
In other words, isComponentVisible should be used to decide if we should run checks to determine if component should be rerendered. However, the change of isComponentVisible alone shouldn't trigger a rerender, because it's already rendered. You can check out that behaviour by switching tabs back and forth. On the first change of tab, isComponentVisible changes, but so do other props, so the chart is rerendered. But if you go back to previous tab, only isComponentVisible changes, so there's no point in rerendering a chart because there are no changes that would affect that chart.

Sorry for rambling, I hope that it makes sense 😆

);
const OVERFLOWABLE_VIZ_TYPES = new Set(['filter_box']);
const DEFAULT_HEADER_HEIGHT = 22;
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/dashboard/containers/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import getFormDataWithExtraFilters from '../util/charts/getFormDataWithExtraFilt
import Chart from '../components/gridComponents/Chart';
import { PLACEHOLDER_DATASOURCE } from '../constants';

const EMPTY_FILTERS = {};
const EMPTY_OBJECT = {};

function mapStateToProps(
{
Expand All @@ -54,7 +54,7 @@ function mapStateToProps(
ownProps,
) {
const { id } = ownProps;
const chart = chartQueries[id] || {};
const chart = chartQueries[id] || EMPTY_OBJECT;
const datasource =
(chart && chart.form_data && datasources[chart.form_data.datasource]) ||
PLACEHOLDER_DATASOURCE;
Expand Down Expand Up @@ -82,7 +82,7 @@ function mapStateToProps(
datasource,
slice: sliceEntities.slices[id],
timeout: dashboardInfo.common.conf.SUPERSET_WEBSERVER_TIMEOUT,
filters: getActiveFilters() || EMPTY_FILTERS,
filters: getActiveFilters() || EMPTY_OBJECT,
formData,
editMode: dashboardState.editMode,
isExpanded: !!dashboardState.expandedSlices[id],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
import { ChartQueryPayload, Charts, LayoutItem } from 'src/dashboard/types';
import { getExtraFormData } from 'src/dashboard/components/nativeFilters/utils';
import { DataMaskStateWithId } from 'src/dataMask/types';
import { areObjectsEqual } from 'src/reduxUtils';
import getEffectiveExtraFilters from './getEffectiveExtraFilters';
import { ChartConfiguration, NativeFiltersState } from '../../reducers/types';
import { getAllActiveFilters } from '../activeAllDashboardFilters';
Expand Down Expand Up @@ -67,16 +68,18 @@ export default function getFormDataWithExtraFilters({
const labelColors = scale.getColorMap();

// if dashboard metadata + filters have not changed, use cache if possible
const cachedFormData = cachedFormdataByChart[sliceId];
if (
(cachedFiltersByChart[sliceId] || {}) === filters &&
(colorScheme == null ||
cachedFormdataByChart[sliceId].color_scheme === colorScheme) &&
cachedFormdataByChart[sliceId].color_namespace === colorNamespace &&
isEqual(cachedFormdataByChart[sliceId].label_colors, labelColors) &&
!!cachedFormdataByChart[sliceId] &&
dataMask === undefined
cachedFiltersByChart[sliceId] === filters &&
cachedFormData?.color_scheme === colorScheme &&
cachedFormData?.color_namespace === colorNamespace &&
isEqual(cachedFormData?.label_colors, labelColors) &&
!!cachedFormData &&
areObjectsEqual(cachedFormData?.dataMask, dataMask, {
ignoreUndefined: true,
})
) {
return cachedFormdataByChart[sliceId];
return cachedFormData;
}

let extraData: { extra_form_data?: JsonObject } = {};
Expand Down