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

kgabryje · 2021-08-25T10:40:03Z

SUMMARY

We generate formData, which is passed through props to Chart, with a function getFormDataWithExtraFilters. The idea is to generate formData, save it in a cache object, and on each subsequent render use the cached version. However, due to incorrect equality checks, we never used the cached version and created a new formData object each time. That resulted not only in recalculating the same logic many times, but also triggered Chart's rerender, as formData object's reference was changing.
Moreover, in Chart component's shouldComponentUpdate function we triggered rerender each time chart's visibility changed - in other words, we rerendered all charts every time we changed a tab.
This PR fixes both of those behaviours.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No changes

TESTING INSTRUCTIONS

Everything should work as it did before.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

CC @junlincc @jinghua-qa

villebro

LGTM and appears to work well. Two minor non-blocking comments

villebro · 2021-08-25T11:41:37Z

superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts

-    (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,
+    })


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

const cachedFormdata = cachedFormdataByChart[sliceId];

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,

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

Issues from both comments fixed

villebro · 2021-08-25T11:42:25Z

superset-frontend/src/dashboard/components/gridComponents/Chart.jsx

+  static whyDidYouRender = true;
+


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

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.

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 🙂

graceguo-supercat · 2021-08-26T07:11:44Z

superset-frontend/src/dashboard/components/gridComponents/Chart.jsx

@@ -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',


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?

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 😆

codecov · 2021-08-26T13:16:19Z

Codecov Report

Merging #16444 (47b7db4) into master (970d762) will decrease coverage by 0.24%.
The diff coverage is 71.40%.

❗ Current head 47b7db4 differs from pull request most recent head ccd5980. Consider uploading reports for the commit ccd5980 to get more accurate results

@@            Coverage Diff             @@
##           master   #16444      +/-   ##
==========================================
- Coverage   76.64%   76.40%   -0.25%     
==========================================
  Files        1000     1002       +2     
  Lines       53489    53670     +181     
  Branches     6816     6855      +39     
==========================================
+ Hits        40996    41005       +9     
- Misses      12257    12426     +169     
- Partials      236      239       +3

Flag	Coverage Δ
hive	`?`
javascript	`70.83% <64.47%> (-0.06%)`	⬇️
presto	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...set-frontend/src/components/ModalTrigger/index.jsx	`95.65% <ø> (ø)`
...-frontend/src/components/OmniContainer/Omnibar.tsx	`100.00% <ø> (ø)`
...et-frontend/src/components/TableView/TableView.tsx	`94.52% <ø> (ø)`
superset-frontend/src/dashboard/actions/hydrate.js	`1.72% <ø> (+0.02%)`	⬆️
...et-frontend/src/dashboard/components/Dashboard.jsx	`78.84% <ø> (ø)`
...frontend/src/dashboard/components/Header/index.jsx	`69.49% <ø> (ø)`
...rontend/src/dashboard/containers/DashboardPage.tsx	`0.00% <0.00%> (ø)`
...perset-frontend/src/explore/components/Control.tsx	`76.47% <ø> (ø)`
...-frontend/src/explore/components/ControlHeader.jsx	`86.20% <ø> (ø)`
.../components/ExploreAdditionalActionsMenu/index.jsx	`100.00% <ø> (ø)`
... and 143 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 970d762...ccd5980. Read the comment docs.

graceguo-supercat

LGTM

* upstream/master: fix: create example DB if needed (apache#16451) fix(native-filters): add handle undefined control value gracefully (apache#16468) Revert "chore: Changes the DatabaseSelector to use the new Select component (apache#16334)" (apache#16478) fix(explore): JS error for creating new metrics from columns (apache#16477) fix: queryEditor bug (apache#16452) docs: make code snippet usable with required imports (apache#16473) perf(dashboard): decouple redux props from dashboard components (apache#16421) perf(dashboard): reduce number of rerenders of Charts (apache#16444)

* perf(dashboard): reduce number of rerenders of Charts * Remove static property * Cleanup

perf(dashboard): reduce number of rerenders of Charts

b45c15d

kgabryje requested review from ktmud, zhaoyongjie and villebro August 25, 2021 10:40

pull-request-size bot added the size/M label Aug 25, 2021

kgabryje requested a review from rusackas August 25, 2021 10:40

villebro approved these changes Aug 25, 2021

View reviewed changes

junlincc added hold:testing! On hold for testing test_priority:NA and removed hold:testing! On hold for testing labels Aug 25, 2021

graceguo-supercat reviewed Aug 26, 2021

View reviewed changes

kgabryje added 2 commits August 26, 2021 11:20

Remove static property

c982379

Cleanup

ccd5980

graceguo-supercat approved these changes Aug 26, 2021

View reviewed changes

kgabryje merged commit 8ad495a into apache:master Aug 26, 2021

kgabryje mentioned this pull request Sep 7, 2021

fix(dashboard): label colors included in explore url #16621

Merged

9 tasks

This was referenced Oct 14, 2021

DASHBOARD_CROSS_FILTER causes some bugs and performance issues #16866

Closed

perf(native-filters): Decrease number of unnecessary rerenders in native filters #17115

Merged

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

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

c5924c7

* perf(dashboard): reduce number of rerenders of Charts * Remove static property * Cleanup

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

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

2c09e62

* perf(dashboard): reduce number of rerenders of Charts * Remove static property * Cleanup

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kgabryje commented Aug 25, 2021

villebro left a comment

villebro Aug 25, 2021

stephenLYZ Aug 25, 2021

kgabryje Aug 26, 2021

kgabryje Aug 26, 2021

villebro Aug 25, 2021

graceguo-supercat Aug 26, 2021

kgabryje Aug 26, 2021

graceguo-supercat Aug 26, 2021

kgabryje Aug 26, 2021

codecov bot commented Aug 26, 2021

graceguo-supercat left a comment

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

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

Conversation

kgabryje commented Aug 25, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 26, 2021

Codecov Report

graceguo-supercat left a comment

Choose a reason for hiding this comment