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

Jump to bottom

perf: Improve dashboard performance by decreasing rerenders #30958

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

perf: Improve dashboard performance by decreasing rerenders #30958

wants to merge 17 commits into from
+1,781 −1,767

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Nov 18, 2024 •
edited
Loading

SUMMARY

Due to many inefficient redux selectors and other factors causing numerous rerenders, large dashboards were running very slow.

This PR refactors many components related to Dashboard and native filters - some components are rewritten to functional to utilize React's memoization, Redux selectors optimized to select smaller objects that change less frequently, optimized frequently called functions.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

The tests were conducted on a large dashboard - 300+ charts, 40+ filters. Test steps: reload the page, let the 1st tab load, switch to a different tab, let everything load.
Tested on my M2 Pro Macbook - since it's likely that an average Superset user works on a slower computer, I also run the tests with 4x and 20x CPU throttling.
Testing tool - chrome dev tools, performance tab

  1. No throttling
    Before - total scripting time 2.73s (screenshot 1), each chart holder blocks the main thread for ~70ms
    After - total scripting time 1.48s (screenshot 2), each chart holder blocks the main thread for ~5ms

  2. 4x throttling
    Before - total scripting time 11.93s (screenshot 3), each chart holder blocks the main thread for ~300ms
    After - total scripting time 6.23s (screenshot 4), each chart holder blocks the main thread for ~20ms

  3. 20x throttling
    Before - total scripting time 82s (screenshot 5), each chart holder blocks the main thread for ~2s
    After - total scripting time 32s (screenshot 6), each chart holder blocks the main thread for ~100ms

SCREENSHOTS BEFORE:

  1. No throttling

image

  1. 4x throttling

image

  1. 20x throttling

image

SCREENSHOTS AFTER:

  1. No throttling

image

  1. 4x throttling

image

  1. 20x throttling

image

TESTING INSTRUCTIONS

No functional changes - all dashboard functionalities should work like before.

Recommend thorough testing of:

  • Filter interactions
  • Chart loading/rendering
  • Dashboard navigation
  • Cross-filter functionality
  • Dashboard state persistence

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

@kgabryje kgabryje marked this pull request as draft November 18, 2024 12:50
@dosubot dosubot bot added the dashboard:performance Related to Dashboard performance label Nov 18, 2024
@kgabryje kgabryje marked this pull request as ready for review November 18, 2024 15:28
@dosubot dosubot bot added the change:frontend Requires changing the frontend label Nov 18, 2024
@villebro
Copy link
Member

These PRs from Kamil always make me soo happy! ❤️

@villebro
Copy link
Member

/testenv up

villebro
villebro reviewed Nov 18, 2024
View reviewed changes
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.

I did my best to find anything to comment or even nit about in this PR, but this looks really solid. I will have to resort to testing to see there's anything that looks out of the ordinary. Will approve once I'm done with the testing rounds..

@github-actions GitHub Actions
Copy link
Contributor

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

fisjac
fisjac reviewed Nov 18, 2024
View reviewed changes
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
</FiltersPanel>
);
}}
{renderChild}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

superset-frontend/src/dashboard/components/DashboardBuilder/state.ts

const toggleDashboardFiltersOpen = useCallback((visible?: boolean) => {
setDashboardFiltersOpen(prevState => visible ?? !prevState);
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

superset-frontend/src/dashboard/components/SyncDashboardState/index.tsx Outdated
],
(metadata, dashboardId, colorScheme, filters, dataMask) => {
// Building nativeFilters object without spreading in reduce
const optimizedNativeFilters = Object.keys(filters).reduce((acc, key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice improvement. Nit: should this be named nativeFilters outside of the context of this PR, I don't think "optimized" make sense.

@villebro
Copy link
Member

/testenv up

@github-actions GitHub Actions
Copy link
Contributor

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

@kgabryje
Copy link
Member Author

@villebro unfortunately the ephemeral environments have been broken for some time now 🙁

@kgabryje
Copy link
Member Author

Neat AI summary provided by @rusackas 🙂

Key changes include:

  1. Memoization optimizations:
    • Added useMemo/useCallback hooks throughout components
    • Memoized selectors with createSelector
    • Reduced unnecessary re-renders
  2. Component modernization:
    • Converted class components to functional components with hooks
    • Removed redundant Redux connections in favor of useSelector/useDispatch
    • Simplified prop passing and state management
  3. Architecture improvements:
    • Moved Chart container logic directly into component
    • Simplified data flow and prop drilling
    • Better separation of concerns

The main potential risk areas to watch:

  1. FilterBar and native filtering changes - ensure filter state management remains consistent with previous behavior
  2. Chart component refactoring - verify chart rendering and updates work correctly, especially around:
    • Data loading states
    • Filter interactions
    • Resize handling
  3. Tab/Dashboard navigation - confirm tab state management and routing remains intact

Recommend thorough testing of:
- Filter interactions
- Chart loading/rendering
- Dashboard navigation
- Cross-filter functionality
- Dashboard state persistence

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisjac fisjac fisjac left review comments

@villebro villebro villebro left review comments

@justinpark justinpark Awaiting requested review from justinpark

@geido geido Awaiting requested review from geido

@rusackas rusackas Awaiting requested review from rusackas

@eschutho eschutho Awaiting requested review from eschutho

@Vitor-Avila Vitor-Avila Awaiting requested review from Vitor-Avila

@sadpandajoe sadpandajoe Awaiting requested review from sadpandajoe

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
change:frontend Requires changing the frontend dashboard:performance Related to Dashboard performance size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kgabryje @villebro @fisjac