-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: high memory consumption in certain workspaces when staticcheck is enabled #61178
Comments
Per ongoing discussion in #45457, we have narrowed this down to SSA/SSI analyzers that are enabled (nilness and staticcheck). Looking at the profile posted there, it appears that encoding/decoding staticcheck's IR is causing quite a bit of allocation:
As reported by @folago, with staticcheck/nilness enabled they are observing up to 12GB of disk reads, vs ~300MB of reads without. This indicates that SSA structures may be consuming more than the fixed 1GB of cache, which could result in a lot of churn. Next step: try to reproduce this in an open source repository that we know to have expensive analysis, such as hashicorp/terraform-provider-aws. (I'll note that I am intentionally not CCing Dominik yet, as I suspect our driver lacks some sophistication, and don't want to bother him :) ) |
This repro'ed on hashicorp/terraform-provider-aws: just open the @adonovan and I investigated, and thus far, we don't see anything "wrong" (apart from the process consuming all available memory and crashing, of course). Building the staticcheck IR is expensive, and now that gopls has proper fact support, the staticcheck I'll note that running staticcheck on Therefore, I think there are three action items:
|
|
Thanks @dominikh. I'll just note that this repository was chosen because it is known to be very expensive to analyze. It has some large low-level packages. 10GB does not seem entirely unreasonable. But it may be worthwhile to understand the cost a bit better: e.g. is there some common edge through a method that causes buildir to be effectively quadratic? |
Change https://go.dev/cl/511215 mentions this issue: |
Change https://go.dev/cl/511216 mentions this issue: |
The problem is very easy to reproduce without staticcheck or buildir, and without gopls. Simply add a dummy fact to nilness.Analyzer.FactTypes and run the nilness command:
See #61506. |
When analysis takes a long time, it can be a poor experience for our users. Address this in a number of ways: - Limit parallelism of analysis, so that it doesn't consume all available memory. - Add progress reporting if analysis takes longer than 5s, for better visibility and to allow cancellation. In case this is annoying to users, add a setting to disable this feature. For golang/go#61178 Change-Id: I15e05f39c606ff7a3fecee672918ee3c4a640fbc Reviewed-on: https://go-review.googlesource.com/c/tools/+/511215 gopls-CI: kokoro <[email protected]> Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
Gopls can rely on a deterministic ordering of methods, and therefore does not need to sort in the objectpath algorithm, which turns out to be enormously expensive on certain repositories (see golang/go#61178). Add hooks to skip method sorting during encode/decode, and use this for analysis facts. This CL is intentionally surgical, to avoid unintended side-effects for this change in behavior. Notably, other uses of objectpath are unaffected. For golang/go#61178 Change-Id: If915dab45b837e23ae5b841e3a9367aa0b53df89 Reviewed-on: https://go-review.googlesource.com/c/tools/+/511216 Run-TryBot: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
The memory problem has been mitigated by reduced parallelism, so I'll close this. But there is more work to do on analysis, tracked by #61506. |
Forked from #45457 (comment), where @folago is reporting that they still encounter high memory consumption in their workspace. In fact, this memory consumption is even higher than [email protected].
I suspect analysis facts, the import graph for open packages, or perhaps a memory leak. Unfortunately this reproducer is not open source, but with some help we should be able to narrow down the cause.
CC @adonovan
The text was updated successfully, but these errors were encountered: