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): Improve performance of complex dashboards #19064

Merged
merged 9 commits into from
Mar 9, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Mar 8, 2022

SUMMARY

This PR improves the performance of dashboards with complex structure (multiple tabs, many levels of nesting, many charts, native filters).
The main culprit of poor performance was a function used for finding charts in filter's scope. getChartIdsInFilterScope was written in a very suboptimal way, which drastically slowed down loading charts - which was especially noticeable when opening a dashboard or switching tabs.
I've rewritten that function and made a few other improvements, which resulted in a much smoother experience.
The scope of the changes in this PR is related only to native filters and cross filters. Performance issues related to filter box were not addressed, as the filter box will soon be deprecated in favor of native filters.

The tests were performed on a dashboard with 100+ charts, 70+ tabs, up to 4 levels of nested tabs, up to 10 charts in a single tab, 9 native filters.

Dashboard used for testing: dashboard_export_20220309T112451.zip

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-03-08.at.14.07.50.mov

After:

Screen.Recording.2022-03-08.at.14.06.22.mov

TESTING INSTRUCTIONS

  1. Verify that building dashboards works like before
  2. Verify that applying native filters works like before
  3. Verify that applying cross filters works like before
  4. Verify that applying filters from filter box works like 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

https://app.shortcut.com/preset/story/38683/performance-issues-to-navigate-between-tabs-depending-on-the-amount-of-charts-nested-tabs-on-a-dashboard

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #19064 (37008db) into master (f9c7405) will increase coverage by 0.00%.
The diff coverage is 45.00%.

❗ Current head 37008db differs from pull request most recent head f9ea592. Consider uploading reports for the commit f9ea592 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #19064   +/-   ##
=======================================
  Coverage   66.50%   66.50%           
=======================================
  Files        1642     1644    +2     
  Lines       63442    63472   +30     
  Branches     6439     6452   +13     
=======================================
+ Hits        42191    42213   +22     
- Misses      19582    19589    +7     
- Partials     1669     1670    +1     
Flag Coverage Δ
javascript 51.23% <40.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...perset-frontend/src/addSlice/AddSliceContainer.tsx 61.76% <ø> (ø)
...ard/components/filterscope/FilterScopeSelector.jsx 79.20% <0.00%> (ø)
...ashboard/components/gridComponents/ChartHolder.jsx 58.82% <ø> (ø)
...end/src/dashboard/util/filterboxMigrationHelper.ts 39.20% <0.00%> (ø)
...end/src/dashboard/util/getChartIdsInFilterScope.ts 0.00% <0.00%> (ø)
...rontend/src/visualizations/TimeTable/TimeTable.jsx 0.00% <0.00%> (ø)
superset/databases/commands/create.py 92.15% <ø> (+3.92%) ⬆️
...src/dashboard/components/FiltersBadge/selectors.ts 68.46% <14.28%> (-2.18%) ⬇️
superset-frontend/src/views/CRUD/hooks.ts 47.15% <33.33%> (ø)
...frontend/src/SqlLab/components/SqlEditor/index.jsx 50.84% <40.00%> (-0.60%) ⬇️
... and 8 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 f9c7405...f9ea592. Read the comment docs.

@rusackas
Copy link
Member

rusackas commented Mar 8, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

@rusackas Ephemeral environment spinning up at http://34.209.37.190:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@rusackas
Copy link
Member

rusackas commented Mar 8, 2022

@graceguo-supercat is going to LOVE this PR 🚀

@kgabryje could you please export your test dashboard (if it uses sample data) and attach it here* and/or import it into the above ephemeral environment when it spins up?

*Fragility caveat that the import might get nuked with the ephemeral env, thus the request to upload/link here ;)

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

This is incredible work, a major achievement in improving dashboard performance! 👏 Changes look very straight forward and would actually serve as a great example for frontend development guidelines (use custom hooks to encapsulate complex state logic, memoize heavy computation and in the process avoid the need for JSON.stringify when referencing the results in dependency arrays etc). Also, not a blocker, but it would be great if getChartIdsInFilterScope could be converted to TS.

@jinghua-qa
Copy link
Member

@graceguo-supercat is going to LOVE this PR 🚀

@kgabryje could you please export your test dashboard (if it uses sample data) and attach it here* and/or import it into the above ephemeral environment when it spins up?

*Fragility caveat that the import might get nuked with the ephemeral env, thus the request to upload/link here ;)

Would be helpful if the perf test dashboard can be upload, currently the import dashboard feature seems not enable in the ephemeral env and i can not import the test dashboard

@graceguo-supercat
Copy link

graceguo-supercat commented Mar 9, 2022

@kgabryje Thank you for the perf improvement! just 1 question here: I saw both getChartIdsInFilterBoxScope and getChartIdsInFilterScope functions. So when to use each of them?

@kgabryje
Copy link
Member Author

kgabryje commented Mar 9, 2022

@kgabryje Thank you for the perf improvement! just 1 question here: I saw both getChartIdsInFilterBoxScope and getChartIdsInFilterScope functions. So when to use each of them?

@graceguo-supercat I kept getChartIdsInFilterBoxScope for legacy reasons. Refactoring the logic related to filter box would be rather risky - I decided not to do it since we're on the road of deprecating filter box in favour in native filters. I added a comment to getChartIdsInFilterBoxScope that redirects to the new getChartIdsInFilterScope.
So going forward, we should use only getChartIdsInFilterScope

@kgabryje
Copy link
Member Author

kgabryje commented Mar 9, 2022

@rusackas @jinghua-qa I added exported dashboard to the PR description 🙂 I'll spin up a new test env with VERSIONED_EXPORT enabled once docker builds

@kgabryje
Copy link
Member Author

kgabryje commented Mar 9, 2022

/testenv up FEATURE_VERSIONED_EXPORT=true FEATURE_DASHBOARD_CROSS_FILTERS=true

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

@kgabryje Ephemeral environment spinning up at http://54.191.63.77:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the awesome work @kgabryje. I left some non-blocking comments.

Given the critical nature of the components, I kindly ask that you wait for @jinghua-qa's approval before merging.

@kgabryje
Copy link
Member Author

kgabryje commented Mar 9, 2022

@kgabryje Ephemeral environment spinning up at http://54.191.63.77:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa This env is still up to date, the last changes were non-functional

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM, and feels super snappy in testing. This PR is such a huge improvement!!

Copy link
Member

@jinghua-qa jinghua-qa left a comment

Choose a reason for hiding this comment

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

LGTM

@kgabryje kgabryje merged commit 3c1fb94 into apache:master Mar 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

Ephemeral environment shutdown and build artifacts deleted.

kgabryje added a commit to preset-io/superset that referenced this pull request Mar 14, 2022
)

* perf(dashboard): Improve performance of filter indicators

* Improve perf of cross filters

* Rename old function

* Fix undefined

* fix type

* fix tests

* fix undefined

* Address code review comments

* Address code review comments
@sadpandajoe
Copy link
Member

🏷️ preset:2022.9

villebro pushed a commit that referenced this pull request Apr 3, 2022
* perf(dashboard): Improve performance of filter indicators

* Improve perf of cross filters

* Rename old function

* Fix undefined

* fix type

* fix tests

* fix undefined

* Address code review comments

* Address code review comments

(cherry picked from commit 3c1fb94)
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 need:qa-review Requires QA review preset:2022.9 size/L 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants