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: avoid work to compute unused fields in 'go list' #29666

Closed
hyangah opened this issue Jan 10, 2019 · 15 comments
Closed

cmd/go: avoid work to compute unused fields in 'go list' #29666

hyangah opened this issue Jan 10, 2019 · 15 comments
Assignees
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. ToolSpeed
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Jan 10, 2019

We want to get the list of available versions of a module
and the best way is to use go list -m --versions <module>.

Under the hood, the command retrieves the tag list (which we expected)
and also fetches some versions (which is surprising).

Our guess is that the go list does so in order to find information
like the latest version's commit time, etc. For our use case, we don't need
that information and the extra fetch slows down the go list especially
when the repository is large.

Can we have a flag to skip the fetch?

An alternative mentioned by @bcmills is to make go list smarter so it
performs steps necessary to fill in the requested information (e.g. fields
mentioned in -f, or with an additional option to specify fields to prune if
-json is requested)

$ GO111MODULE=on tipgo list -x -m -f '{{.Versions}}' --versions rsc.io/quote

mkdir -p $GOPATH/pkg/mod/cache/vcs # git2 https://github.com/rsc/quote
# lock $GOPATH/pkg/mod/cache/vcs/$HASH.lockmkdir -p $GOPATH/pkg/mod/cache/vcs/$HASH # git2 https://github.com/rsc/quote
cd $GOPATH/pkg/mod/cache/vcs/$HASH; git init --bare
0.006s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git init --bare
cd $GOPATH/pkg/mod/cache/vcs/$HASH; git remote add origin https://github.com/rsc/quote 
0.004s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git remote add origin https://github.com/rsc/quote
cd $GOPATH/pkg/mod/cache/vcs/$HASH; git ls-remote -q origin
0.298s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git ls-remote -q origin
cd $GOPATH/pkg/mod/cache/vcs/$HASH; git cat-file --batch
0.004s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git cat-file --batch

cd $GOPATH/pkg/mod/cache/vcs/$HASH; git fetch -f origin refs/tags/v2.0.0:refs/tags/v2.0.0 refs/tags/v2.0.1:refs/tags/v2.0.1 refs/tags/v3.0.0:refs/tags/v3.0.0
0.365s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git fetch -f origin refs/tags/v2.0.0:refs/tags/v2.0.0 refs/tags/v2.0.1:refs/tags/v2.0.1 refs/tags/v3.0.0:refs/tags/v3.0.0

cd $GOPATH/pkg/mod/cache/vcs/$HASH; git cat-file --batch
0.004s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git cat-file --batch
go: finding rsc.io/quote v1.5.2
cd $GOPATH/pkg/mod/cache/vcs/$HASH; git tag -l
0.003s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git tag -l
cd $GOPATH/pkg/mod/cache/vcs/$HASH; git -c log.showsignature=false log -n1 '--format=format:%H %ct %D' refs/tags/v1.5.2
0.004s # cd $GOPATH/pkg/mod/cache/vcs/$HASH; git -c log.showsignature=false log -n1 '--format=format:%H %ct %D' refs/tags/v1.5.2

[v1.0.0 v1.1.0 v1.2.0 v1.2.1 v1.3.0 v1.4.0 v1.5.0 v1.5.1 v1.5.2 v1.5.3-pre1]

@bcmills @heschik @katiehockman @jayconrod

@heschi
Copy link
Contributor

heschi commented Jan 10, 2019

From what I can piece together, Hana's example above is particularly poor, but probably unfixable. It's looking for a v1 module, but it needs to download every non-v1 version too to make sure that they all have a go.mod. (If one didn't, then it'd be +incompatible and a valid answer for the query.)

Previously we had been looking at the way it behaves when fetching rsc.io/quote/v2, or a v1 module from a repo that doesn't have v2 yet. That can probably be improved.

@jayconrod jayconrod added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go modules labels Jan 10, 2019
@jayconrod jayconrod added this to the Go1.13 milestone Jan 10, 2019
@bcmills
Copy link
Contributor

bcmills commented Jan 11, 2019

I would rather do the field-based pruning than add a separate flag. Field-based pruning gives us the opportunity to eliminate a lot of different kinds of redundant work, whereas a “skip timestamp fetching” flag would have a very narrow benefit.

@bcmills
Copy link
Contributor

bcmills commented Jan 11, 2019

One nice option for the JSON field-pruning might be to allow the -json flag to accept a comma-separated list of fields with an explicit = connector:

go list -m -json=Path,Versions,Replace.Path --versions rsc.io/quote

@bcmills
Copy link
Contributor

bcmills commented Jan 11, 2019

It's looking for a v1 module, but it needs to download every non-v1 version too to make sure that they all have a go.mod.

It occurs to me that this could interact with caching in an interesting way: if you're only looking for new versions and you know when you last checked the repo, then it's probably safe to assume that any tag that was +incompatible still is, and any tag that was a module still is. So perhaps some sort of flag to restrict the result to “tags added since timestamp T” would avoid the need for that fetch too.

@jayconrod
Copy link
Contributor

I really like the idea of -json accepting a list of fields that are actually needed. I think we should do that on the regular go list (not -m) side as well.

@bcmills bcmills changed the title cmd/go: allow 'go list -m --versions' to skip vcs-fetching cmd/go: avoid work to compute unused fields in 'go list' Jan 17, 2019
@bcmills bcmills added ToolSpeed NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 17, 2019
@bcmills
Copy link
Contributor

bcmills commented Jan 17, 2019

Retitled to address the general problem: we won't be adding a specific flag to avoid VCS fetches, but we certainly can optimize the work that we do based on the requested output.

@jayconrod
Copy link
Contributor

In CL 270858, we noticed that go list -m all loads .info files for each module in the build list in order to find the version's timestamp. That's printed with -json and maybe -f. If neither of those flags are specified, then .info files aren't used: go build and go get don't need them. We should skip loading those files if we don't need their content.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/309190 mentions this issue: cmd/go/internal/modload: avoid loading the module graph to list only the name of the main module

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/309191 mentions this issue: cmd/go/internal/modload: avoid loading the full module graph when listing specific modules

gopherbot pushed a commit that referenced this issue Apr 30, 2021
…the name of the main module

For #36460
For #29666

Change-Id: I9e46f7054d52c053be80c483757cdd34b22822d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/309190
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit that referenced this issue Apr 30, 2021
…ting specific modules

For #36460
For #41297
Updates #29666

Change-Id: I5f324c0ef9a164f8043d2188101d141bb5fa7454
Reviewed-on: https://go-review.googlesource.com/c/go/+/309191
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit that referenced this issue Apr 20, 2022
If the user provides the -json flag to explicitly specify fields, but
doesn't specify the Deps or DepsErrors fields, skip computing the deps
fields.

For #29666

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

Change https://go.dev/cl/402736 mentions this issue: cmd/go: skip computing BuildInfo in go list unless it's needed

gopherbot pushed a commit that referenced this issue May 4, 2022
The only fields of the go list output that require BuildInfo to be
computed are the Stale and StaleReason fields. If a user explicitly
requests JSON fields and does not ask for Stale or StaleReason, skip
the computation of BuildInfo.

For #29666

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

Change https://go.dev/cl/417595 mentions this issue: doc/go1.19: add a release note for 'go list -json=SomeField'

gopherbot pushed a commit that referenced this issue Jul 14, 2022
For #29666.

Change-Id: I575375fb039e5809b0ed2ce985f6352a61142d63
Reviewed-on: https://go-review.googlesource.com/c/go/+/417595
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
For golang#29666.

Change-Id: I575375fb039e5809b0ed2ce985f6352a61142d63
Reviewed-on: https://go-review.googlesource.com/c/go/+/417595
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
@matloob
Copy link
Contributor

matloob commented Oct 13, 2022

Closing this issue as the -json=SomeField feature has been released in Go 1.19

@matloob matloob closed this as completed Oct 13, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Oct 13, 2022
fviernau added a commit to fviernau/go that referenced this issue Feb 14, 2023
If the user provides the -json flag to explicitly specify fields, but
doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of golang#29666.
fviernau added a commit to fviernau/go that referenced this issue Feb 14, 2023
If the user provides the -json flag to explicitly specify fields, but
doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of golang#29666.
fviernau added a commit to oss-review-toolkit/ort that referenced this issue Feb 14, 2023
The `-json` option allows specifying a comma separated list of
property names to include in the output. The `go list` command has a
mechanism for skipping certain processing steps which are not needed for
computing the values corresponding to those specified property names,
see [^1],[^2] and [^3]. Such a mechanism is not in place for the `-f` option.

Migrate from `-f` to `-json` to benefit from above mechanism, removing
potential sources of issues. It could be that this allows to drop the
parameter `-buildvcs=false`, which is left for future investigation.

The primary reason for switching to `-json` is to extend the mentioned
mechanism [^4]] in `go list` to conditionally skip computing embed
files in an analog way which would fix [^5].

[^1]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/load/pkg.go#L2768-L2776
[^2]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/list/list.go#L605-L606
[^3]: golang/go#29666
[^4]: golang/go#49534 (comment)
[^5]: #5560

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/468075 mentions this issue: cmd/go: don't compute Embed fields if they're not needed

fviernau added a commit to fviernau/go that referenced this issue Feb 14, 2023
If the user provides the -json flag to explicitly specify fields, but
doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of golang#29666.
fviernau added a commit to fviernau/go that referenced this issue Feb 14, 2023
If the user provides the -json flag to explicitly specify fields, but
doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of golang#29666.
fviernau added a commit to fviernau/go that referenced this issue Feb 14, 2023
If the user provides the -json flag to explicitly specify fields, but
doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of golang#29666.
fviernau added a commit to oss-review-toolkit/ort that referenced this issue Feb 14, 2023
The `-json` option allows specifying a comma separated list of
property names to include in the output. The `go list` command has a
mechanism for skipping certain processing steps which are not needed for
computing the values corresponding to those specified property names,
see [^1],[^2] and [^3]. Such a mechanism is not in place for the `-f` option.

Migrate from `-f` to `-json` to benefit from above mechanism, removing
potential sources of issues. It could be that this allows to drop the
parameter `-buildvcs=false`, which is left for future investigation.

The primary reason for switching to `-json` is to extend the mentioned
mechanism [^4] in `go list` to conditionally skip computing embed
files in an analog way which would fix [^5].

[^1]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/load/pkg.go#L2768-L2776
[^2]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/list/list.go#L605-L606
[^3]: golang/go#29666
[^4]: golang/go#49534 (comment)
[^5]: #5560

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
fviernau added a commit to oss-review-toolkit/ort that referenced this issue Feb 15, 2023
The `-json` option allows specifying a comma separated list of
property names to include in the output. The `go list` command has a
mechanism for skipping certain processing steps which are not needed for
computing the values corresponding to those specified property names,
see [^1],[^2] and [^3]. Such a mechanism is not in place for the `-f` option.

Migrate from `-f` to `-json` to benefit from above mechanism, removing
potential sources of issues. It could be that this allows to drop the
parameter `-buildvcs=false`, which is left for future investigation.

The primary reason for switching to `-json` is to extend the mentioned
mechanism [^4] in `go list` to conditionally skip computing embed
files in an analog way which would fix [^5].

[^1]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/load/pkg.go#L2768-L2776
[^2]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/list/list.go#L605-L606
[^3]: golang/go#29666
[^4]: golang/go#49534 (comment)
[^5]: #5560

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
fviernau added a commit to oss-review-toolkit/ort that referenced this issue Feb 15, 2023
The `-json` option allows specifying a comma separated list of
property names to include in the output. The `go list` command has a
mechanism for skipping certain processing steps which are not needed
for computing the values corresponding to those specified property
names, see [^1],[^2] and [^3]. Such a mechanism is not in place for the
`-f` option.

Migrate from `-f` to `-json` to benefit from above mechanism, removing
potential sources of issues. It could be that this allows to drop the
parameter `-buildvcs=false`, which is left for future investigation.

The primary reason for switching to `-json` is to extend the mentioned
mechanism [^4] in `go list` to conditionally skip computing embed
files in an analog way which would fix [^5].

[^1]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/load/pkg.go#L2768-L2776
[^2]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/list/list.go#L605-L606
[^3]: golang/go#29666
[^4]: golang/go#49534 (comment)
[^5]: #5560

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
fviernau added a commit to oss-review-toolkit/ort that referenced this issue Feb 15, 2023
The `-json` option allows specifying a comma separated list of
property names to include in the output. The `go list` command has a
mechanism for skipping certain processing steps which are not needed
for computing the values corresponding to those specified property
names, see [^1],[^2] and [^3]. Such a mechanism is not in place for the
`-f` option.

Migrate from `-f` to `-json` to benefit from above mechanism, removing
potential sources of issues. It could be that this allows to drop the
parameter `-buildvcs=false`, which is left for future investigation.

The primary reason for switching to `-json` is to extend the mentioned
mechanism [^4] in `go list` to conditionally skip computing embed
files in an analog way which would fix [^5].

[^1]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/load/pkg.go#L2768-L2776
[^2]: https://github.com/golang/go/blob/0cd309e12818f988693bf8e4d9f1453331dcf9f2/src/cmd/go/internal/list/list.go#L605-L606
[^3]: golang/go#29666
[^4]: golang/go#49534 (comment)
[^5]: #5560

Signed-off-by: Frank Viernau <frank_viernau@epam.com>
fviernau added a commit to fviernau/go that referenced this issue Feb 20, 2023
If the user provides the -json flag to explicitly specify fields, but
doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of golang#29666.
gopherbot pushed a commit that referenced this issue Feb 23, 2023
If the user provides the -json flag to explicitly specify fields, but doesn't specify any *Embed* field, skip computing the embed fields.

This enhances the initial implementation of #29666.

Change-Id: I60e86fb25a445689aecbcc7f3f3f88e0f37a0fc5
GitHub-Last-Rev: 2795c19
GitHub-Pull-Request: #58522
Reviewed-on: https://go-review.googlesource.com/c/go/+/468075
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. ToolSpeed
Projects
None yet
Development

No branches or pull requests

9 participants