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

cmd/go: improve go list parallelism #59157

Closed
findleyr opened this issue Mar 20, 2023 · 9 comments
Closed

cmd/go: improve go list parallelism #59157

findleyr opened this issue Mar 20, 2023 · 9 comments
Assignees
Labels
GoCommand cmd/go gopls/performance Issues related to gopls performance (CPU, memory, etc). NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. ToolSpeed
Milestone

Comments

@findleyr
Copy link
Contributor

While staring at gopls execution patterns, I noticed surprisingly low CPU utilization while awaiting go list results. This got me excited that perhaps there was still some room for optimization.

I hooked up runtime/trace to cmd/go, and observed the following result while loading kubernetes using go/packages:
image

The circled regions appear to be pre- and post- processing packages in a single thread, the bulk of which amounts to sorting of dependencies. This should be possible to parallelize.

The middle region looks like recursive loading of imports, involving a mixture of I/O, and therefore does not achieve high CPU utilization. It's possible that this could also be optimized.

Overall, if we could reduce this 4.6s load time, it could have significant impact on gopls startup. (background: we're working to heavily optimize gopls via on-disk caching, and while historically go list has been a small fraction of load time, it has become the majority of load time). Eyeballing the trace, I would not be surprised if it would be easy to shave off 1s, and possible (but hard) to shave off 2s. :)

CC @matloob

@gopherbot
Copy link

Change https://go.dev/cl/477896 mentions this issue: cmd/go: add a -debug-runtime-trace flag

@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 21, 2023
@heschi
Copy link
Contributor

heschi commented Mar 21, 2023

cc @bcmills

@heschi heschi added this to the Go1.21 milestone Mar 21, 2023
@bcmills bcmills added ToolSpeed gopls/performance Issues related to gopls performance (CPU, memory, etc). and removed Performance labels Mar 21, 2023
gopherbot pushed a commit that referenced this issue Mar 21, 2023
The runtime/trace package proved useful for investigating go command
performance, and it makes sense (to me) to make this available for
development behind an undocumented flag, at the cost of ~25KB of binary
size. We could of course futher hide this functionality behind an
experiment or build tag, if necessary.

Updates #59157

Change-Id: I612320920ca935f1ee10bb6a803b7952f36c939b
Reviewed-on: https://go-review.googlesource.com/c/go/+/477896
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/481855 mentions this issue: cmd/go: suppress calls to collectDeps for test packages

@gopherbot
Copy link

Change https://go.dev/cl/482237 mentions this issue: cmd/go: suppress calls to collectDeps for test packages

gopherbot pushed a commit that referenced this issue Apr 4, 2023
Instead, do the cycle checking in recompileForTest once the test
variant packages have been poked in the right places in the dependency
tree(graph?).

(Pair programming with bcmills@.)

For #59157.

Change-Id: I0c644cb9f2c0dac3a5b0189e2aa0eef083c669f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/482237
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/482877 mentions this issue: cmd/go: return and handle errors from loadImport for bad imports

@gopherbot
Copy link

Change https://go.dev/cl/483016 mentions this issue: internal/lsp/testdata: update 'bad0' diagnostics test for 1.21

gopherbot pushed a commit to golang/tools that referenced this issue Apr 10, 2023
Go list is being updated to produce an error on the importing package,
rather than the imported package when there is an invalid import (such
as of a internal package, or of a vendored package but with the wrong
import path). This new error will produce a new diagnostic which is
causing the builders to fail on that change. This change updates the
tests to expect the new go list error on 1.21.

I'm not sure how to update the tests to work both before and after
that change. We can maybe change the golden diagnostic count summary
comparison to be disabled from this cl until the new cl is submitted,
but I'd like to see if I'm missing a better solution.

For golang/go#59157

Change-Id: I340dcddfd37edc4fc53aa9008eabd22366c34f2a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/483016
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/483495 mentions this issue: cmd/go: localize computation of deps/depserrors in list

gopherbot pushed a commit that referenced this issue Apr 10, 2023
And assign the error to the importing package. Before this change, for
some errors for bad imports, such as importing a vendored package with
the wrong path, we would make a dummy package for the imported package
with the error on it. Instead report the error on the importing
package where it belongs. Do so by returning an error from loadImport
and handling it on the importing package.

For #59157

Change-Id: I952e1a82af3857efc5da4fd3f8bc6be473a60cf5
Reviewed-on: https://go-review.googlesource.com/c/go/+/482877
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/482999 mentions this issue: internal/lsp/testdata: update for new diagnostic from go list

gopherbot pushed a commit that referenced this issue Apr 10, 2023
Stop depending on DepsErrors to report errors to the user and instead
only use it and compute it in list. Instead, use Incomplete to figure
out when a package or its depencies have an error, and only if they
do, do the work of finding all those errors.

For #59157

Change-Id: Ied927f53e7b1f66fad9248b40dd11ed960b3ef91
Reviewed-on: https://go-review.googlesource.com/c/go/+/483495
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@matloob matloob self-assigned this Apr 13, 2023
@gopherbot
Copy link

Change https://go.dev/cl/484496 mentions this issue: cmd/go: parallelize part of loading test packages in list

gopherbot pushed a commit to golang/tools that referenced this issue Apr 26, 2023
Now that go list reports import errors on the importing package,
update the disabled part of the test to expect that error. (We
commented out the line generating the diagnostic in the test before
making the change so that the change itself wouldn't generate a
diagnostic and break the test).

For golang/go#59157

Change-Id: I207f507c8e07dd55d3609ca2bb9bdf25528ec898
Reviewed-on: https://go-review.googlesource.com/c/tools/+/482999
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go gopls/performance Issues related to gopls performance (CPU, memory, etc). NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. ToolSpeed
Projects
None yet
Development

No branches or pull requests

5 participants