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

Cache command picks for the duration of the quick pick #200549

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Dec 11, 2023

Fixes #200548

The PR seems to work, the main things that need clarity are:

  • Whether this is the right place to do the caching, I'm sure there's a more elegant way to do this.
  • Does it invalidate the cache correctly, are there edge cases I'm missing?
  • Should we do this more generally? I'm not sure how dynamic providers can be.

It essentially removes this getCommandPicks entirely when filtering, until the provider changes:

image

Below are some numbers in milliseconds of the duration of _getPicks before and after when filtering 1+ letters.

Before

  • 12.72
  • 10.05
  • 17.49
  • 10.90
  • 11.11
  • avg 12.45

After:

  • 3.39
  • 2.11
  • 3.88
  • 4.46
  • 4.21
  • avg 3.61 (-8.84, -72%)

Since the change reduces the work by about 9ms, this is expected to more often than not display the results in 1 less frame on a 60Hz display and almost always in >= 1 less frame on a 144Hz display.

@Tyriar Tyriar added this to the December / January 2024 milestone Dec 11, 2023
@Tyriar Tyriar self-assigned this Dec 11, 2023
@TylerLeonhardt
Copy link
Member

One of the things I'm currently worried about with this change is that I wanna know what happens when you start the Command Palette right away at start up... I worry that that initial list doesn't contain all commands because some components are delayed in their initialization. We do wait a bit but it might not be enough for low end machines.

I'd be fine accepting this and see what insiders has to say though. cc @bpasero for any historical insight.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I think this is a slippery slope: commands are not that static:

  • they can change based on the active editor which you can change while picker is opened
  • pickers can be cancelled when you jump to another picker which then could mean we accidentally cache an empty array because of that cancellation

I am also not a big fan of the explicit new method to clear cache, ideally the picker implementation could do this internally, for example by wiring this into the disposable or when a command is accepted.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 13, 2023

@TylerLeonhardt could we do a quick command count check and reload to avoid problems there? We could move the caching into getCommandPicks itself with that? We could also cache getCodeEditorCommandPicks and getGlobalCommandPicks independently (post mapping into picks to save more time).

@bpasero sure it would be better to not need to do this, but 9ms is a significant price to pay on every keystroke. I think the above suggestion mostly addresses your concern.

@Tyriar Tyriar modified the milestones: December / January 2024, February 2024 Dec 19, 2023
@Tyriar Tyriar modified the milestones: February 2024, March 2024 Feb 20, 2024
@Tyriar Tyriar modified the milestones: March 2024, Backlog Mar 22, 2024
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.

The command palette should cache commands while filtering
3 participants