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

Select: Performance improvements when opening menu and when hovering over options #69230

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

JoaoSilvaGrafana
Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana commented May 29, 2023

What is this feature?

While investigating performance issues with our current Select component, there were 4 different interactions that were slow to the user (#69132). This feature addresses 2 of those performance issues:

  • Hovering over an option

Before, when hovering over an option in a very large list (300000 in this case) there was a visible lag and in the profiler we can see a "long task" (in red) for opening the menu and then another "long task" for each of the hovers that it picks up:

Screen.Recording.2023-05-29.at.14.50.27.mov

After, there is no lag and only one "long task" left for the opening of the menu:

Screen.Recording.2023-05-29.at.14.49.20.mov
  • Opening the menu

This one does not solve it completely, as there are issues with react-select itself and how it renders and calculates which options to have on the list (there is an issue on the react-select repo JedWatson/react-select#3128).

What this does is that when the inputValue is falsy (for instance '' on first open), it passes the filterOptions function as null, which means it just shows all the options. Then when there is an inputValue it sets the filterOptions function back. This does improve performance as can be seen in the next video, when we try to open the current virtualized list with 300000 options it takes approximately 2.5s, but the one with filterOptions={null} "only" approximately 1.7s.

Screen.Recording.2023-05-29.at.15.56.00.mov

Why do we need this feature?

There have been several issues discussing the performance of the select component when there are thousands+ of options (see https://github.com/grafana/observability-metrics-squad/issues/47 or #57675). This improves the performance in the two cases mentioned above.

Who is this feature for?

Users using selects with very long lists

Which issue(s) does this PR fix?:

Fixes partly #69132

Special notes for your reviewer:

There is 1 known loss in functionality here. Previously, while a user is hovering over an option, it was possible to press "enter" or "tab" to select that option, while now if they do press "enter" or "tab" it will be the option that is focused with the keyboard that will be selected. We could make the "hovering fix" only happen via prop, or only when there are more than a certain number of options, but I don't think this is a severe lack of functionality to warrant another prop to the already 50+ that Select has. Worth hearing if other people have different opinions.
If there is any other regression in functionality please let me know!

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@JoaoSilvaGrafana JoaoSilvaGrafana added type/performance add to changelog area/grafana/ui Issues that belong to components in the @grafana/ui library no-backport Skip backport of PR labels May 29, 2023
@JoaoSilvaGrafana JoaoSilvaGrafana added this to the 10.1.x milestone May 29, 2023
@JoaoSilvaGrafana JoaoSilvaGrafana requested a review from a team as a code owner May 29, 2023 16:16
@JoaoSilvaGrafana JoaoSilvaGrafana requested review from joshhunt and ashharrison90 and removed request for a team May 29, 2023 16:16
@joshhunt
Copy link
Contributor

We could make the "hovering fix" only happen via prop, or only when there are more than a certain number of options, but I don't think this is a severe lack of functionality to warrant another prop to the already 50+ that Select has.

I agree with your decision here. It would be more bad to have this interaction differ across Grafana.

Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

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

Great improvement. Tested this in the Virtualised story (by bumping the generateThousandsOfOptions up to 100,000), and with the Prometheus query editor and while this doesn't solve all problems (initial open is still a bit slow, typing isn't super responsive), it ups the performance from unusable to slightly tolerable, which is a bit improvement!

Would be good to get review from others, such as @grafana/observability-metrics on this as well!

@joshhunt
Copy link
Contributor

I think the next step after this should be to virtualize-by-default. Might want to control this via a feature flag first (just to be safe).

Copy link
Contributor

@gtk-grafana gtk-grafana left a comment

Choose a reason for hiding this comment

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

Code changes look good, working for me locally. Thanks for taking the time to improve this component!

@JoaoSilvaGrafana JoaoSilvaGrafana merged commit 1aac15f into main Jun 21, 2023
15 checks passed
@JoaoSilvaGrafana JoaoSilvaGrafana deleted the joao_silva/select_perf branch June 21, 2023 09:28
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library no-backport Skip backport of PR type/performance
Projects
Archived in project
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants