perf(dashboard): Improve perf of highlighting charts in scope of active filter #15424

kgabryje · 2021-06-28T12:17:24Z

SUMMARY

Due to passing as a prop nativeFilters object (from redux), which changes on each focus/unfocus of native filters, from DashboardComponent, all dashboard components (tabs, rows, chart holders, dividers...) were being rerendered on each native filter focus. This PR optimizes that behaviour by passing nativeFilters only to Tabs component.
Also, I optimized rendering FiltersBadge component - due to passing complex objects as props, expensive calculations were being re-run multiple times.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Enable DASHBOARD_NATIVE_FILTERS feature flag
Open a dashboard and add some native filters
Focus on a native filter, verify that charts and tabs highlighting works as before (but it should be a bit faster on big dashboards, as only FilterFocusHighlight component gets rerendered).

ADDITIONAL INFORMATION

Has associated issue:
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: @graceguo-supercat @villebro @junlincc

…ve filter

codecov · 2021-06-28T19:22:22Z

Codecov Report

Merging #15424 (4eae3ca) into master (8205ea5) will increase coverage by 0.01%.
The diff coverage is 63.05%.

❗ Current head 4eae3ca differs from pull request most recent head 930108e. Consider uploading reports for the commit 930108e to get more accurate results

@@            Coverage Diff             @@
##           master   #15424      +/-   ##
==========================================
+ Coverage   76.99%   77.01%   +0.01%     
==========================================
  Files         975      974       -1     
  Lines       50634    50705      +71     
  Branches     6222     6238      +16     
==========================================
+ Hits        38988    39052      +64     
- Misses      11435    11440       +5     
- Partials      211      213       +2

Flag	Coverage Δ
javascript	`71.78% <65.03%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
...rset-frontend/src/components/ErrorMessage/types.ts	`100.00% <ø> (ø)`
...src/components/FilterableTable/FilterableTable.tsx	`82.26% <ø> (ø)`
superset-frontend/src/components/Select/Select.tsx	`0.00% <0.00%> (ø)`
...frontend/src/dashboard/components/Header/index.jsx	`67.15% <ø> (ø)`
...c/dashboard/components/gridComponents/Markdown.jsx	`82.82% <ø> (ø)`
...d/src/dashboard/components/gridComponents/index.js	`100.00% <ø> (ø)`
...nd/src/dashboard/containers/DashboardComponent.jsx	`100.00% <ø> (+7.69%)`	⬆️
...uperset-frontend/src/explore/exploreUtils/index.js	`66.45% <0.00%> (ø)`
...ers/components/TimeGrain/TimeGrainFilterPlugin.tsx	`0.00% <0.00%> (ø)`
superset-frontend/src/setup/setupColors.ts	`86.66% <0.00%> (ø)`
... and 21 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 8205ea5...930108e. Read the comment docs.

suddjian · 2021-06-28T19:44:43Z

superset-frontend/src/dashboard/components/FiltersBadge/index.tsx

+      selectIndicatorsForChart(chartId, dashboardFilters, datasources, chart),
+    [
+      chartId,
+      JSON.stringify(chart),


Interesting dependency trick! Do we need to suppress linter warnings here?

It turned out that it wasn't necessary here, but we do use that trick throughout our codebase. ESLint complains about that, but I think that we can live with that if we must use complex objects in the dependency array

suddjian · 2021-06-28T19:50:04Z

superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts

  dashboardLayout: Layout,
  chartConfiguration: ChartConfiguration = {},
 ): Indicator[] => {
-  const chart = charts[chartId];
+  // no indicators before chart is rendered
+  if (chart.chartStatus !== 'rendered') return [];


This logic should probably be in a render function rather than in a selector

amitmiran137 · 2021-06-28T21:06:56Z

#15420 could you also resolve that issue ?

suddjian

Awesome! The original work on this feature was kind of rushed so I'm really glad you were able to get these optimizations done!

villebro

Great work! One minor problem noticed, other than that LGTM!

superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts

kgabryje · 2021-06-29T10:33:58Z

#15420 could you also resolve that issue ?

Thanks for reporting that! I'll tackle it in a separate PR as it seems to be out of scope of this one

villebro

LGTM 👍

…ve filter (#15424) * perf(dashboard): Improve perf of highlighting charts in scope of active filter * More optimizations * fix tests * Fix lint * bug fix (cherry picked from commit f109da4)

…ve filter (apache#15424) * perf(dashboard): Improve perf of highlighting charts in scope of active filter * More optimizations * fix tests * Fix lint * bug fix

perf(dashboard): Improve perf of highlighting charts in scope of acti…

5b47bc9

…ve filter

kgabryje requested a review from villebro June 28, 2021 12:17

pull-request-size bot added the size/M label Jun 28, 2021

amitmiran137 added the v1.3 label Jun 28, 2021

pull-request-size bot added size/L and removed size/M labels Jun 28, 2021

More optimizations

e98d859

kgabryje force-pushed the perf/focus-active branch from a6f9129 to e98d859 Compare June 28, 2021 16:09

kgabryje requested a review from suddjian June 28, 2021 16:12

fix tests

c906c8c

pull-request-size bot added size/XL and removed size/L labels Jun 28, 2021

Fix lint

64812f9

suddjian reviewed Jun 28, 2021

View reviewed changes

suddjian approved these changes Jun 28, 2021

View reviewed changes

villebro requested changes Jun 29, 2021

View reviewed changes

superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts Outdated Show resolved Hide resolved

bug fix

930108e

villebro approved these changes Jun 29, 2021

View reviewed changes

kgabryje merged commit f109da4 into apache:master Jun 29, 2021

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

mistercrunch added the 🚢 1.3.0 label Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(dashboard): Improve perf of highlighting charts in scope of active filter #15424

perf(dashboard): Improve perf of highlighting charts in scope of active filter #15424

kgabryje commented Jun 28, 2021 •

edited

Loading

codecov bot commented Jun 28, 2021 •

edited

Loading

suddjian Jun 28, 2021

kgabryje Jun 29, 2021

suddjian Jun 28, 2021

kgabryje Jun 29, 2021

amitmiran137 commented Jun 28, 2021

suddjian left a comment

villebro left a comment

kgabryje commented Jun 29, 2021

villebro left a comment

perf(dashboard): Improve perf of highlighting charts in scope of active filter #15424

perf(dashboard): Improve perf of highlighting charts in scope of active filter #15424

Conversation

kgabryje commented Jun 28, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 28, 2021 • edited Loading

Codecov Report

suddjian Jun 28, 2021

Choose a reason for hiding this comment

kgabryje Jun 29, 2021

Choose a reason for hiding this comment

suddjian Jun 28, 2021

Choose a reason for hiding this comment

kgabryje Jun 29, 2021

Choose a reason for hiding this comment

amitmiran137 commented Jun 28, 2021

suddjian left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

kgabryje commented Jun 29, 2021

villebro left a comment

Choose a reason for hiding this comment

kgabryje commented Jun 28, 2021 •

edited

Loading

codecov bot commented Jun 28, 2021 •

edited

Loading