x/tools/gopls: missing fields / methods in completion results

[findleyr]

While working in x/tools recently, I observed that I was sometimes missing struct fields in completion results. This should never be the case.

It turns out these results are missing due to completion budget being exhausted. Setting "completionBudget" to "1s" worked. Bisecting, I discovered that prior to https://go.dev/cl/347563 I generally (though not always) got the results I expected.

Clearly our regression testing is not catching regressions in completion performance, and the default completion budget of 100ms can lead to significantly degraded results.

We need to:

1. Investigate why struct fields could ever be missed in completion results.
2. Evaluate our current completion benchmarks, improve them if necessary, and hook them into performance monitoring (see also x/tools,x/build: performance monitoring for gopls #53538).
3. Look for low-hanging improvements to completion performance (there must be some, for example the performance hit of https://go.dev/cl/347563 should be easy to mitigate).

[muirdm]

I think this return is likely the problem. Even "shallow" completion candidates still go through the deep search stuff, so that return could stop before even depth=0 candidates have been processed.

The easy fix would be to finish processing the current queue before returning. Or maybe only finish the current queue if it is depth=0.

[findleyr]

Thanks @muirdm! We should guarantee that depth=0 stuff completes, in addition to fixing the performance regression.

[gopherbot]

Change https://go.dev/cl/419988 mentions this issue: gopls/internal/regtest/bench: refactor and improve benchmarks

[gopherbot]

Change https://go.dev/cl/420220 mentions this issue: gopls/internal/regtest/bench: add a test for completion following edits

[findleyr]

Oh my goodness, a huge chunk of time is spent in go/types.missingMethodReason, via types.AssignableTo and types.ConvertibleTo, which is totally wasted work...

Will file an issue if one does not already exist... (later)

[gopherbot]

Change https://go.dev/cl/422906 mentions this issue: internal/lsp/completion: don't use Type.String for checking identity