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: high memory consumption in certain workspaces when staticcheck is enabled #61178

Closed
findleyr opened this issue Jul 5, 2023 · 8 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Jul 5, 2023

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 gopls@v0.11.0.

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

@findleyr findleyr added this to the gopls/v0.12.5 milestone Jul 5, 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 Jul 5, 2023
@findleyr
Copy link
Contributor Author

findleyr commented Jul 6, 2023

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:

(pprof) top
Showing nodes accounting for 13800.10MB, 88.23% of 15640.41MB total
Dropped 449 nodes (cum <= 78.20MB)
Showing top 10 nodes out of 105
      flat  flat%   sum%        cum   cum%
 9905.08MB 63.33% 63.33% 10184.11MB 65.11%  honnef.co/go/tools/go/ir.memberFromObject
 1119.06MB  7.15% 70.48%  1392.08MB  8.90%  golang.org/x/tools/go/types/objectpath.namedMethods
  541.59MB  3.46% 73.95%  2420.59MB 15.48%  golang.org/x/tools/internal/facts.(*Decoder).Decode
  522.31MB  3.34% 77.29%   522.31MB  3.34%  os.ReadFile
  351.41MB  2.25% 79.53%   351.41MB  2.25%  reflect.unsafe_NewArray
  325.03MB  2.08% 81.61%   325.03MB  2.08%  honnef.co/go/tools/go/ir.NewConst (inline)
  310.03MB  1.98% 83.59%   310.03MB  1.98%  go/types.NewParam (inline)
  268.52MB  1.72% 85.31%   268.52MB  1.72%  go/types.Id
  241.51MB  1.54% 86.86%   241.51MB  1.54%  encoding/gob.decString
  215.56MB  1.38% 88.23%   719.62MB  4.60%  honnef.co/go/tools/go/ir.makeWrapper

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 :) )

@findleyr findleyr changed the title x/tools/gopls: still encountering high memory consumption in certain workspaces x/tools/gopls: encountering high memory consumption in certain workspaces when staticcheck is enabled Jul 6, 2023
@findleyr findleyr changed the title x/tools/gopls: encountering high memory consumption in certain workspaces when staticcheck is enabled x/tools/gopls: high memory consumption in certain workspaces when staticcheck is enabled Jul 6, 2023
@findleyr
Copy link
Contributor Author

This repro'ed on hashicorp/terraform-provider-aws: just open the internal/provider package (or anything that imports it).

@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 buildir analyzer must be run on everything in the forward transitive closure of open packages. When that closure is large, it take a long time. I'll note that the nilness analyzer and buildssa analyzers don't use facts, so unlike buildir it is NOT the case that buildssa must be run on all dependencies. That's why enabling nilness was not nearly as expensive as staticcheck.

I'll note that running staticcheck on internal/provider also takes a while. However, staticcheck does a better job of limiting the high water mark for memory usage: on my computer it tops out at 10GB and doesn't go much higher.

Therefore, I think there are three action items:

  • Limit analysis concurrency to reduce the high water mark of memory consumed by analysis in gopls.
  • Check with @dominikh whether this performance is consistent with his expectations.
  • (possibly) separate "fast" and "slow" analyzers, so that we may prioritize running fast analyzers with low latency.

@dominikh
Copy link
Member

staticcheck using 10 GB seems quite high to me. I'll try and do some analysis tomorrow.

@findleyr
Copy link
Contributor Author

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?

@gopherbot
Copy link

Change https://go.dev/cl/511215 mentions this issue: gopls/internal/lsp/cache: UX improvements for analysis

@gopherbot
Copy link

Change https://go.dev/cl/511216 mentions this issue: go/types/objectpath: avoid sorting methods for gopls

@adonovan
Copy link
Member

adonovan commented Jul 21, 2023

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:

terraform-provider-aws$ (cd ~/w/xtools && go build -o nilness ) && time ~/w/xtools/nilness -cpuprofile=prof ./internal/provider/
... time passes, memory fades ...

See #61506.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 24, 2023
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 <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jul 24, 2023
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 <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
@findleyr
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

No branches or pull requests

4 participants