x/tools/gopls: textDocument/definition is slow in large codebase #55293
Comments
gopherbot
added
Tools
|
Thanks for the investigation! The full trace file would be helpful, as would RPC logs from your session: I don't think you should ever see duplicate results in the span of a single |
|
After looking at this a bit more, it is possible that due to some of the memory optimizations gopls has in place we may encounter that cache.typeCheck entry up to three times a single package up to three times (though the majority of packages should be type checked only once). However, more concerning is the duplicate "cache.parseGo" spans for a single within the same definition trace. If I'm understanding correctly, this is what you are reporting as happening up to 20 times. |
|
So, the only way that Are you able to reproduce this reliably? If so, does it reproduce on every definition request, or just the first? |
|
(files removed, happy to share if needed) Here's the full trace information - as an example, the The request originates from
I am able to repo this reliably, and this occurs on every request from this package. Thanks for taking a look! |
|
Could you also clarify the intended behavior here? After the initial rpc is made ( |
I do see a bunch of these logs in the gopls output - I wonder if some packages aren't getting type checked correctly, causing gopls to reindex packages every time this occurs. I'm not sure exactly what this means though - if you think this is relevant, could you help decode this log? I do notice that the second argument is always a |
|
@findleyr - mind taking a look at https://go-review.googlesource.com/c/tools/+/433335 ? Turns out the logs are relevant - they were indicating that gopls was re-parsing certain packages while responding to these RPCs. I did some more digging and found that the private types mentioned in the logs were exposed publicly through type casting. was forcing recomputation of a package, as the After commenting out the relevant types, I found that gopls resolved RPCs in a timely manner. I believe that this caused a huge impact for me because of how messed up the import graph is for these packages - we have two packages with code like the above, and the combination looks like it could cause up to tens of package recomputations! Here are some public packages which were also mentioned in logs: I dug through most of these and believe that my change will fix these errors! |
|
@aashishkapur nice investigation! I will look at your CL (sorry for the delay: yesterday was awash with meetings). Before I dig in, this is still surprising to me. A bit of background: gopls has a concept of a "workspace" package (a package that you are working on) vs a "non-workspace" package (a package that you import). Unfortunately, right now they go through most of the same code-paths, and we save a lot of information for non-workspace packages that is not necessary in most cases. In order to reduce this memory, we trim the AST of non-workspace packages before type-checking, so that we have the same package APIs, just for a smaller amount of code. Resolving a definition request should be simple: make sure the current package is type-checked, and then figure out which Subsequently, the editor may open a non-workspace package, turning it into a workspace package, but that should happen after the So while your CL may fix the problem, the fact that it fixes the problem is surprising to me, and may point to a different bug. CC @adonovan |
hyangah
added
the
NeedsInvestigation
|
oh, interesting. To clarify, does gopls trim a workspace package at all? How does gopls think about imports or dependencies of a package currently open in vscode? I ask because I am navigating within the same package and see this re-parsing behavior. The trace info linked above is a request going from one file to another in the same package. I believe this is also present when navigating from one package to another, but I'll check. |
No, gopls does not trim a workspace package. It defines the workspace based on the go.mod or go.work boundaries of the open folder (i.e. expands to the nearest surrounding go.work or (failing that) go.mod). So if an import of an open package is within the same module (or a module referenced by go.work), then it will not be trimmed. If an import is outside that boundary, it will be trimmed. There is definitely something broken about the behavior you are observing. |
|
I see - all packages with in a workspace should already be parsed. All of these files / packages in this codebase are in a single workpace, with Would we expect the |
|
Another thought - i wonder if test packages are the cause of this re-parsing. The logs indicate that all re-parsing contains a test package Are |
|
Ok, I think I understand what is happening. @aashishkapur you are using a Are you able to try upgrading to Go 1.18 or later? |
|
Unfortunately, I'm still seeing the same issue I see the same re-parsing in the trace logs as well as the |
|
Here's some more trace info from running on This package contains code very similar to which was a test case in my CL! |
|
Thanks again for the details! I notice that all of the packages in your example are test variants, which seems highly likely to be related. |
|
Looking into this further, I believe this is a consequence of (or at least related to) https://go-review.git.corp.google.com/c/tools/+/347563, which intended to fix #47825. In that CL we updated gopls to search test variants for the package containing the target position, to fix a problem that the target declaration was not found. This could potentially cause a lot of unnecessary synchronous type-checking if the target declaration is outside of the workspace. Even before that CL we were doing unnecessary type-checking, as there's no need for us to fully type-check the target package: we know which position we're jumping to. In general, our textDocument/definition is doing way more work that it needs to, and I'm going to rewrite it to be much simpler, which on first principles should solve the problem you observe: we should only need the current package, which will already be type-checked. However, I haven't yet reproduced behavior as poor as you observe, and I'm concerned that other logic (such as hover) that goes through the leaky |
|
Change https://go.dev/cl/438495 mentions this issue: |
|
Change https://go.dev/cl/438496 mentions this issue: |
|
Change https://go.dev/cl/438497 mentions this issue: |
In all cases where we call FindPackageFromPos, we know that the given position must be in the forward transitive closure of an originating package. Refactor to use this information, potentially saving significant type-checking when searching for a package. As a result of this change, we never need to search intermediate test variants when querying PackagesForFile. Also replace snapshot arguments with token.FileSet arguments, when the snapshot is only needed for its FileSet. For golang/go#55293 Change-Id: Icf6236bea76ab5105a6bab24ce3afc574147882b Reviewed-on: https://go-review.googlesource.com/c/tools/+/438495 gopls-CI: kokoro <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
When searching for declaration information about a position, it should suffice to search the narrowest fully type-checked package containing the file. This should significantly improve performance when there are many test variants of the current package, that have not yet been type-checked in the ParseFull mode (as reported in golang/go#55293). For golang/go#55293 Change-Id: I89a1999f9fe82dea51dd47db769c90b69be5e454 Reviewed-on: https://go-review.googlesource.com/c/tools/+/438496 Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Robert Findley <[email protected]> gopls-CI: kokoro <[email protected]>
Metadata.IsIntermediateTestVariant can be derived from PkgPath and ForTest, so make it a method. Along the way, document the easily missed fact that intermediate test variants are not workspace packages. For golang/go#55293 Change-Id: Ie03011aef9c91ebce89e8aad01ef39b65bdde09a Reviewed-on: https://go-review.googlesource.com/c/tools/+/438497 Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Run-TryBot: Robert Findley <[email protected]>
|
@aashishkapur I think the changes above should fix the problem, but could you test? To install gopls at tip, you can do the following: |
|
Gentle ping: anyone who experienced this issue, could you please confirm that it is now resolved? Closing for now as I believe the root cause is understood and mitigated. |
|
@findleyr Thanks for your help here - performance is looking much much better! |
findleyr
added
gopls/performance
gopls version
go env
What Happened
I noticed that
go-to-definitionin vscode on a go file was taking a long time, and consistently saw the following log when navigating within a package (specifically, I was using go to definition to navigate within a package. I'm operating within a large monorepo, with some heavily interlinked packages)I pulled up the "Trace Information" tab in the debug server, and found a log for
textDocument/DefinitionRPC. This log looks to consist ofcache.parseGoandcache.TypeCheckevents, but many of these events are replicated, seemly indicating that duplicate work is build done. I see multiple packages being parsed many times (up to ~20, looks to be dependent on imports), with all of its files listed underneathAppears that there's duplicate work being done - I looked through the codebase for these events and found many todos added in golang/tools@f79f3aa. (eg a b). Is there any planned work to speed up the
cache.typeCheckandcache.parseGooperations?Happy to DM the full trace file to someone if that would help!
The text was updated successfully, but these errors were encountered: