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

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

Merged
merged 3 commits into from
Aug 26, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b45c15d
perf(dashboard): reduce number of rerenders of Charts
kgabryje Aug 25, 2021
c982379
Remove static property
kgabryje Aug 26, 2021
ccd5980
Cleanup
kgabryje Aug 26, 2021
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
View file
Open in desktop
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 ?? undefined,
colorScheme: metadata?.color_scheme ?? undefined,
editMode: canEdit && editMode,
isPublished: dashboardData.published,
hasUnsavedChanges: false,
Expand Down
5 changes: 4 additions & 1 deletion superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
View file
Open in desktop
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',
Copy link

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 All @@ -100,6 +101,8 @@ const ChartOverlay = styled.div`
`;

export default class Chart extends React.Component {
static whyDidYouRender = true;

Copy link
Member

Choose a reason for hiding this comment

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

do we want to start introducing these in the codebase now? I'm fine either way

Copy link

Choose a reason for hiding this comment

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

i have no idea what is this. Since this is open source code base, everything should either self-explained or well documented for the whole community.

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 left that in by accident, thanks for pointing out!
@graceguo-supercat In order to detect unnecessary rerenders, I use a library whyDidYouRender (https://github.com/welldone-software/why-did-you-render). Adding a static property whyDidYouRender = true to a component tells the library to watch that component and report rerenders in the form of console logs. Of course, I meant to cleanup those props, but clearly I missed that one 🙂

constructor(props) {
super(props);
this.state = {
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/dashboard/containers/Chart.jsx
View file
Open in desktop
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
15 changes: 9 additions & 6 deletions superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts
View file
Open in desktop
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 @@ -68,13 +69,15 @@ export default function getFormDataWithExtraFilters({

// if dashboard metadata + filters have not changed, use cache if possible
if (
(cachedFiltersByChart[sliceId] || {}) === filters &&
(colorScheme == null ||
cachedFormdataByChart[sliceId].color_scheme === colorScheme) &&
cachedFormdataByChart[sliceId].color_namespace === colorNamespace &&
isEqual(cachedFormdataByChart[sliceId].label_colors, labelColors) &&
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
areObjectsEqual(cachedFormdataByChart[sliceId]?.dataMask, dataMask, {
ignoreUndefined: true,
})
Copy link
Member

Choose a reason for hiding this comment

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

While we're modifying code here and for the sake of DRY, would it make sense to break out

const cachedFormdata = cachedFormdataByChart[sliceId];

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Btw, Is it designed to not use the cache when the colorScheme is undefined? Because previous logic that colorScheme == null will return true if it is undefined and go to next condition. In addition, the above line of code seems that the colorScheme will not be null value whatever.

colorScheme: metadata?.color_scheme ?? undefined,

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point Stephen! I think we can safely remove colorScheme === null and ?? undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Issues from both comments fixed

) {
return cachedFormdataByChart[sliceId];
}
Expand Down