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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: performance regression of gopls/internal/lsp tests with older Go versions #59184

Closed
findleyr opened this issue Mar 22, 2023 · 3 comments
Closed

x/tools/gopls: performance regression of gopls/internal/lsp tests with older Go versions #59184

findleyr opened this issue Mar 22, 2023 · 3 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.0

Comments

@findleyr
Copy link
Contributor

Just noticed while debugging an issue at older go versions:

The ./internal/lsp tests (which usually take around 10s on my laptop) take 20-30s on Go 1.17 and earlier. By comparison, these tests take ~10s at gopls/v0.11.0. This is a significant regression that must be understood. Either the importer/exporter are significantly slower at these Go versions, or there is some other problem. It may be (per https://go.dev/issue/59179) that there are significant errors in packages that fail to type-check pre generics, and the import/exprot performance is degraded on these packages.
@findleyr findleyr added this to the gopls/v0.12.0 milestone Mar 22, 2023
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Mar 22, 2023
@findleyr findleyr changed the title x/tools/gopls: performance regresion of gopls/internal/lsp tests with older Go versions x/tools/gopls: performance regression of gopls/internal/lsp tests with older Go versions Mar 22, 2023
@gopherbot
Copy link

Change https://go.dev/cl/480075 mentions this issue: gopls/internal/lsp/cache: parse the builtin file through the cache

@findleyr
Copy link
Contributor Author

findleyr commented Mar 29, 2023 •
edited

Surprisingly, bisected to https://go.dev/cl/456975. I suspect it is related to loads being in the critical path of certain operations. On older go versions, our tests have more orphaned files (those build tagged for 1.18+), that are probably being redundantly reloaded.

@findleyr findleyr mentioned this issue Mar 29, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/480197 mentions this issue: gopls/internal/lsp/cache: record unloadable files if loading completes

gopherbot pushed a commit to golang/tools that referenced this issue Mar 29, 2023
@findleyr @gopherbot
gopls/internal/lsp/cache: parse the builtin file through the cache
4b11203 
Likely because I assumed it was pinned to the snapshot, the builtin file
was not being parsed through the parse cache.

Completion builds a lot of builtin signatures, snapshot.BuiltinFile was
showing up as a hot spot in benchmarks and test profiles.

For golang/go#59184

Change-Id: Ic919562d2194f8f42736a4d6ea2d32e76f06e3ef
Reviewed-on: https://go-review.googlesource.com/c/tools/+/480075
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
@golang golang locked and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.12.0
Development

No branches or pull requests
2 participants
@gopherbot @findleyr