-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
569e349
to
9ede2d3
Compare
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.209.37.190:8080. Credentials are |
@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 ;) |
There was a problem hiding this 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.
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
Outdated
Show resolved
Hide resolved
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 |
@kgabryje Thank you for the perf improvement! just 1 question here: I saw both |
@graceguo-supercat I kept |
@rusackas @jinghua-qa I added exported dashboard to the PR description 🙂 I'll spin up a new test env with |
/testenv up FEATURE_VERSIONED_EXPORT=true FEATURE_DASHBOARD_CROSS_FILTERS=true |
@kgabryje Ephemeral environment spinning up at http://54.191.63.77:8080. Credentials are |
There was a problem hiding this 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.
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
Outdated
Show resolved
Hide resolved
7a74e16
to
f9ea592
Compare
@jinghua-qa This env is still up to date, the last changes were non-functional |
There was a problem hiding this 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!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ephemeral environment shutdown and build artifacts deleted. |
🏷️ preset:2022.9 |
* 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)
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
ADDITIONAL INFORMATION
https://app.shortcut.com/preset/story/38683/performance-issues-to-navigate-between-tabs-depending-on-the-amount-of-charts-nested-tabs-on-a-dashboard