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

x/tools/gopls: use BFS instead of DFS for deep completions #39113

Open
stamblerre opened this issue May 16, 2020 · 1 comment
Open

x/tools/gopls: use BFS instead of DFS for deep completions #39113

stamblerre opened this issue May 16, 2020 · 1 comment
Labels
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 16, 2020

I suspect that using a breadth-first traversal for deep completions will lead to improvements in the speed with which deep completions finds matching candidates.

I also think this change may allow us to restructure the completions code in way that is a bit clearer by giving completions more defined "phases":

  1. collecting candidates
  2. checking if the candidates are matches and scoring
  3. formatting the matches into completion items

These phases are already exist in the code, but the fact that the deep completions happen in (*completer).found muddies the waters a bit.

Somewhat related: We could have the 100ms deep completion timeout apply to phases 1 and 2, always (even without deep completions), with no time limit on 3 to avoid dropping matches that we ran out of time to format.

@muirdm: What do you think? I haven't actually tried implementing any of this, so I'm sure I'm missing a lot, but does this sound reasonable / doable?

@muirdm
Copy link

@muirdm muirdm commented May 17, 2020

I agree breadth first search will typically find the good candidates sooner, so it will give better results when don't have enough time to search all the candidates.

  1. collecting candidates
  2. checking if the candidates are matches and scoring

There are cases with a lot of deep candidates (e.g. 100k), so it is important to be able to filter weak candidates as you go. You don't want to end up collecting 100k candidates, sorting and taking the top N since that will be slow. Currently we track the top N=3 deep candidate scores we have seen, and if we find a new candidate not in the top N then we don't collect it.

We could have the 100ms deep completion timeout apply to phases 1 and 2, always (even without deep completions), with no time limit on 3 to avoid dropping matches that we ran out of time to format.

My original intention was to have the 100ms budget apply to everything as you suggest, but OTOH it would be weird to miss an "obvious" lexical candidate because gopls ran out of budget for whatever reason.

Currently the item formatting can be quite slow due to documentation fetching and type/alias formatting. I don't think we should exclude the time spent formatting until we speed those up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.