Avoid using packages.Load #420

linzhp · 2020-03-26T18:06:58Z

Description
Partially fixing #419

It turn out there are two places calling go list:

createPackageMap
parsePackageImport, which calls packages.Load, which calls go list by default.

This PR avoids calling packages.Load and saves one of the go list calls. Some performance data:

With 0b87a54:

$ go clean --cache --modcache && time go run ./mockgen -source /Users/zplin/gocode/src/github.com/bazelbuild/bazel-gazelle/language/lang.go > /dev/null
go: downloading golang.org/x/tools v0.0.0-20190425150028-36563e24a262
go: extracting golang.org/x/tools v0.0.0-20190425150028-36563e24a262
go: finding golang.org/x/tools v0.0.0-20190425150028-36563e24a262
go run ./mockgen -source  > /dev/null  6.98s user 6.10s system 121% cpu 10.767 total

After this PR:

$ go clean --cache --modcache && time go run ./mockgen -source /Users/zplin/gocode/src/github.com/bazelbuild/bazel-gazelle/language/lang.go > /dev/null
go: downloading golang.org/x/mod v0.2.0
go: extracting golang.org/x/mod v0.2.0
go: downloading golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898
go: extracting golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898
go: finding golang.org/x/mod v0.2.0
go: finding golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898
go run ./mockgen -source  > /dev/null  3.77s user 3.81s system 118% cpu 6.383 total

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests

Reviewer Notes

The code flow looks good.
Tests added.

linzhp · 2020-03-27T05:16:13Z

@codyoss I will add tests if the overall approach looks good to you

codyoss · 2020-03-27T14:30:52Z

Thanks for the PR, I probably won't have time to look at this today, but will try to get to it tomorrow. Thanks for being an active member in the community 😄

linzhp · 2020-03-27T18:36:36Z

The travis-ci is failing even all tests are passing.

codyoss · 2020-03-27T18:57:39Z

6.19s$ ./ci/check_go_generate.sh
prog.go:14:2: use of internal package github.com/golang/mock/mockgen/internal/tests/internal_pkg/subdir/internal/pkg not allowed

mockgen/parse.go

codyoss · 2020-03-28T18:29:35Z

mockgen/parse.go

-		return "", err
+	moduleMode := os.Getenv("GO111MODULE")
+	// trying to find the module
+	if moduleMode != "off" {


Is auto a special case?

Might be worth being explicit /w a switch

the meaning of auto changes from Go release to release. To be safe, I only skip checking go.mod when the mode is "off". This may result in different behavior than "go list" in older Go releases.

For example, when "GO111MODULE" is "auto", if a directory is under GOPATH, but also has a go.mod file, "go list" will use GOPATH in 1.12, and use Go module in 1.13. This PR is similar to the behavior of 1.13.

codyoss · 2020-03-28T18:31:54Z

Overall, I think this seems good as long as we can test this throughly. Maybe we can/should also set different CI versions /w different GO111MODULE modes to help facilitate testing...

googlebot · 2020-03-30T17:42:35Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-30T17:42:36Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-30T17:47:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-30T17:47:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

codyoss · 2020-04-08T22:18:34Z

Could you please re-pull master. I will get back to this review this week, sorry for the delay.

linzhp · 2020-04-20T20:27:56Z

@codyoss any progress?

codyoss · 2020-04-20T21:11:49Z

Sorry, was waiting for the CI to go green, but it appears to have gotten stuck. I will get to this today or tomorrow.

codyoss · 2020-04-24T19:43:25Z

It looks like there are failures with go generate: https://travis-ci.org/github/golang/mock/jobs/672731180

fixed linting issues Add tests unexport error making it Windows compatible merge from upstream

linzhp · 2020-04-24T21:13:17Z

@codyoss fixed CI errors

xtonyjiang · 2020-04-27T03:43:27Z

It's too bad that in order to get this performance boost we have to not use go/packages, since I assume go/packages is supposed to allow us to not have to do all this branching logic based on whether we're using modules or not or what Go version we're using.

In the ideal world we would be able to take advantage of some go/packages-like thing that abstracts away the underlying import resolution system, without having the overhead of go list.

So I suppose we've accepted that we'll sacrifice possibly having to maintain this code path with future Go versions in order to get this speed boost? (Although maybe there won't be much more future maintenance because everything is stable now? Not sure what the Go roadmap looks like here.)

linzhp · 2020-04-27T05:34:27Z

Yeah, Go's dependency manager is so tightly coupled with go build, making it very hard to reuse the dependency resolution logic if we use a different build system like Bazel.

Under Bazel's action environment, we don't have access to either GOCACHE or mod cache in the host machine. Every mockgen action runs from cold cache, similar to right after go clean --cache --modcache. Unfortunately, go list downloads and partially compiles the packages and dependencies. With cold cache, this is slow. The downloads and compilation are not necessary to find out the package path from a source directory. A lot of computation and I/O is wasted.

Yes, there is a trade-off between performance and future maintenance. Since Bazel suffers from this issue the more than go build, I can potentially implement the logic in a package driver and set GOPACKAGESDRIVER in bazel's gomock rule so packages.Load will not call goListDriver.

I thought implementing it in golang/mock would benefit more people. That's why I sent PR here first.

linzhp · 2020-05-04T05:12:06Z

@codyoss weekly ping...

linzhp · 2020-05-11T20:49:51Z

@codyoss weekly ping...

codyoss

Sorry this took so long to get back to.

mockgen/testdata/gomod/go.mod

codyoss · 2020-05-19T14:56:59Z

@linzhp Thank you for your contribution! 🎆

linzhp mentioned this pull request Mar 27, 2020

Document why a user should choose the source or reflect mode #406

Open

codyoss reviewed Mar 28, 2020

View reviewed changes

mockgen/parse.go Outdated Show resolved Hide resolved

codyoss reviewed Mar 28, 2020

View reviewed changes

linzhp force-pushed the parse branch from 3c91ab5 to e46c6d8 Compare March 30, 2020 16:41

linzhp force-pushed the parse branch from ce5ef7c to 2dcfc60 Compare March 30, 2020 17:47

linzhp force-pushed the parse branch from 2dcfc60 to 3c8b1b5 Compare April 8, 2020 22:27

linzhp added 2 commits April 24, 2020 13:57

avoid using packages.Load

d8225d0

fixed linting issues Add tests unexport error making it Windows compatible merge from upstream

fixed ci errors

a68df0a

linzhp force-pushed the parse branch from 3c8b1b5 to a68df0a Compare April 24, 2020 21:09

xtonyjiang mentioned this pull request May 8, 2020

Avoid calling packages.Load vektra/mockery#258

Closed

codyoss suggested changes May 18, 2020

View reviewed changes

mockgen/testdata/gomod/go.mod Outdated Show resolved Hide resolved

Dynamically creating testdata

47dadef

codyoss approved these changes May 19, 2020

View reviewed changes

codyoss merged commit 92f53b0 into golang:master May 19, 2020

linzhp mentioned this pull request May 19, 2020

Removing GOPATH in source mode jmhodges/bazel_gomock#44

Closed

linzhp deleted the parse branch March 26, 2021 08:26

linzhp mentioned this pull request Mar 26, 2021

Add ability to override the source-package in mockgen #531

Closed

FastNav mentioned this pull request Jun 16, 2021

Avoid using packages.Load if possible maxbrunsfeld/counterfeiter#181

Open

fe3dback mentioned this pull request Jun 23, 2022

Avoid/optimize packages.Load fe3dback/go-arch-lint#36

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid using packages.Load #420

Avoid using packages.Load #420

linzhp commented Mar 26, 2020 •

edited

Loading

linzhp commented Mar 27, 2020

codyoss commented Mar 27, 2020

linzhp commented Mar 27, 2020

codyoss commented Mar 27, 2020

codyoss Mar 28, 2020

codyoss Mar 28, 2020

linzhp Mar 30, 2020

codyoss commented Mar 28, 2020

googlebot commented Mar 30, 2020

googlebot commented Mar 30, 2020

googlebot commented Mar 30, 2020

googlebot commented Mar 30, 2020

codyoss commented Apr 8, 2020

linzhp commented Apr 20, 2020

codyoss commented Apr 20, 2020

codyoss commented Apr 24, 2020

linzhp commented Apr 24, 2020

xtonyjiang commented Apr 27, 2020

linzhp commented Apr 27, 2020

linzhp commented May 4, 2020

linzhp commented May 11, 2020

codyoss left a comment

codyoss commented May 19, 2020

Avoid using packages.Load #420

Avoid using packages.Load #420

Conversation

linzhp commented Mar 26, 2020 • edited Loading

linzhp commented Mar 27, 2020

codyoss commented Mar 27, 2020

linzhp commented Mar 27, 2020

codyoss commented Mar 27, 2020

codyoss Mar 28, 2020

Choose a reason for hiding this comment

codyoss Mar 28, 2020

Choose a reason for hiding this comment

linzhp Mar 30, 2020

Choose a reason for hiding this comment

codyoss commented Mar 28, 2020

googlebot commented Mar 30, 2020

googlebot commented Mar 30, 2020

googlebot commented Mar 30, 2020

googlebot commented Mar 30, 2020

codyoss commented Apr 8, 2020

linzhp commented Apr 20, 2020

codyoss commented Apr 20, 2020

codyoss commented Apr 24, 2020

linzhp commented Apr 24, 2020

xtonyjiang commented Apr 27, 2020

linzhp commented Apr 27, 2020

linzhp commented May 4, 2020

linzhp commented May 11, 2020

codyoss left a comment

Choose a reason for hiding this comment

codyoss commented May 19, 2020

linzhp commented Mar 26, 2020 •

edited

Loading