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

Avoid unnecessary dashboard parameters updates #4969

Closed
wants to merge 1 commit into from

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Jun 11, 2020

What type of PR is this? (check all applicable)

  • Bug Fix
  • Other

Description

This still needs some testing and analysis on the impact, but I'll do tomorrow (as well as detailing the solution).

I'll try to bring alternatives too (it's possible that #4724 could help here as an alternative).

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Not yet

@gabrieldutra
Copy link
Member Author

(there's a TL;DR in below)

Some explanation: Currently the Parameters component is responsible for updating the URL, which normally is fine. It updates the url whenever the parameters' "Apply All" is hit, but also does that when the parameters are updated externally (prop change). This leads us to this problematic part on Dashboard code:

const globalParameters = useMemo(() => dashboard.getParametersDefs(), [dashboard]);

The Dashboard-level parameters are calculated whenever Dashboard updates, since the array will hold a different address and React only does a shallow comparison, the url will be updated.

During the Performance tests, I mentioned in #4724 that the majority of dashboard updates comes from widgets being loaded. That PR includes two things:

  1. Decouple the Widgets loading state from the "dashboard" object;
  2. Debounce the updating widgets call, so widgets updates are done "together" when they happen in a short period of time.

Those respectively reduce: - the amount of "dashboard" updates; - the overall number of triggered updates.

This PR currently converts the "shallow" comparison to be a deep one, which is fully optimized in the sense of "parameter" updates, but adds a few complexity from the deep comparison. So, I want to bring #4724 up again as an alternative, fully, or at least with the state separation. My understanding is that we should benefit more from the overall reduction in page updates than with avoiding the extra complexity it adds.

TL;DR
This PR converts uses deep comparison for dashboard parameters instead of React shallow ones, re-review #4724 for a solution that reduces actual number of dashboard updates. (or consider only decoupling widgets load state).

Thanks! 🙂

@justinclift justinclift deleted the dashboard-parameters-updates branch August 12, 2023 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant