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/cmd/stringer: stringer should not use go/types #25650

Open
robpike opened this issue May 31, 2018 · 19 comments

Comments

Projects
None yet
10 participants
@robpike
Copy link
Contributor

commented May 31, 2018

It takes much longer to run stringer on a program than to compile it. Stringer should be nearly instantaneous, but I suspect the new importer technology is killing performance. Something certainly is.

At tip, wiith a freshly written file, so the build cache has the dependencies but not the output of building list.go itself:

% go version
go version devel +47be3d49c7 Thu May 10 23:19:40 2018 +0000 darwin/amd64
% cat list.go
package main

import (
	"gopkg.in/src-d/go-license-detector.v2/licensedb/filer"
)

func main() {
}

type Type int

const (
	Other Type = iota
	A
	B
	C
	D
)

func getFiler(file string) filer.Filer {
	return nil
}
% time go build

real	0m1.617s
user	0m1.290s
sys	0m0.408s
% time stringer -type Type

real	0m4.197s
user	0m2.409s
sys	0m0.782s
%

Stringer is taking almost 3 times as long to run as it takes to compile and link the binary. That's crazy. If we remove the dependency on filer, which has a pretty big set of dependencies itself, stringer runs quickly again.

@gopherbot gopherbot added this to the Unreleased milestone May 31, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 31, 2018

My understanding is that the plan to make things better again is #14120 (comment) .

@griesemer

This comment has been minimized.

Copy link
Contributor

commented May 31, 2018

I suspect the stringer is using the default setup of go/types which is using a source importer (rather than an importer based on gc-compiled export data). As a consequence, each time stringer is run, it type-checks not only the file containing the stringer directive, but also all imported packages, recursively, all the way down...

(That is, in fact it's surprisingly fast doing all this work... :-)

It may be sufficient to simply configure types.Config with the gc importer for now (if my hunch above is correct).

@griesemer

This comment has been minimized.

Copy link
Contributor

commented May 31, 2018

@robpike Since you have a good test case, would you mind doing the following change and see what the difference is:

diff --git a/cmd/stringer/importer19.go b/cmd/stringer/importer19.go
index deddadb1..ed08eab4 100644
--- a/cmd/stringer/importer19.go
+++ b/cmd/stringer/importer19.go
@@ -12,5 +12,5 @@ import (
 )
 
 func defaultImporter() types.Importer {
-       return importer.For("source", nil)
+       return importer.For("gc", nil)
 }

I suspect this should fix it.

Maybe this should be settable with a flag?

(As an aside, we can probably get rid of the support for Go 1.8 now (importer18.go)).

@robpike

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

That change gives me an error that it can't find the filer import.

I gave you a full reproducible example. It requires importing a big thing but it's not hard to reproduce.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

(@robpike Just changing the source string from "source" to "gc" (nothing else) should work w/o problem - except for a constant change there's not source change. )

Anyway, I've verified that switching from "source" to "gc" addresses the issue. On my machine, with the current version:

$ time stringer -type Type

real	0m5.002s
user	0m2.892s
sys	0m0.901s

After applying the above change suggested above and re-installing the stringer:

$ time stringer -type Type

real	0m0.037s
user	0m0.013s
sys	0m0.017s

which looks much more reasonable again.

This just needs a decision: Should stringer require that the package it operates on compiles (and all its imports are installed)? If so, we can just make the switch.

Alternatively, we could provide an additional stringer flag (say -sourceImports, or something like that) which would switch to the current behavior.

We should fix this for 1.11. It's embarasingly slow as is.

@robpike Comments?

@robpike

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

I still get this bug when I use "gc":

stringer: checking package: x.go:4:2: could not import gopkg.in/src-d/go-license-detector.v2/licensedb/filer (can't find import: "gopkg.in/src-d/go-license-detector.v2/licensedb/filer")

but not when I use "source". What is my mistake? It's confusing.

Stringer should not care that the package compiles, only that the file parses, but changes people keep making to how things work are breaking all such assumptions.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

@robpike I think you're effectively experiencing #10249. If you go install your packages first, then switching to the 'gc' importer should work. That problem--that go install is needed for the 'gc' importer to work--is a large part of why we switched to the 'src' importer in the first place.

Some other tools (e.g. vet) can handle this semi-magically, since they have direct support from cmd/go, and can do the install as needed, with appropriate caching. But stringer cannot, as it stands.

Presumably we don't want to maintain a 'src' importer cache, since we already have a cmd/go cache. Maybe handling all of this smoothly is something that the new (planned) 'go/packages' package could do? cc @alandonovan

Stringer should not care that the package compiles, only that the file parses

If that is the case, then stringer should not use go/types at all. But it has, at least as far back as #10249 (2015), so this is at least not new. It might be worth investigating to what extent we can use only go/ast.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

Also CC @myitcv, who has been thinking about stringer's use of go/types for a bit.

@myitcv

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

Stringer should not care that the package compiles

I'd tend to agree with this. That's not to say therefore that stringer can't depend on go/types - I'll defer to others on whether stringer should depend on go/types and assume the status quo for now - rather that it should not fail if it doesn't type check.

Instead stringer could do as much as possible given the severity of error:

  • package doesn't parse - error
  • package parses, but type declaration not found - error
  • type declaration found, but no stringer-able const values found because of type-checking errors - no error, just output empty String() method
  • ...
  • package fully type checks, stringer behaves as it currently does

If we are comfortable that stringer should depend on go/types (to make its output more efficient, whatever the motivation was here...) then there is still the "speed" issue. Per @josharian, my understanding is that the speed issue will be solved by @alandonovan's go/packages work and reliance on the build cache (obviating the go install step).

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

Stringer is not in the standard repo. It could be changed for now to use go/importer.Default. The long-term (hopefully quite short-term) plan is to use golang.org/x/tools/go/packages. In fact maybe this should be one of the first programs converted.

/cc @alandonovan

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

Decision: @alandonovan's new go/packages will be used to fix this.

@rsc rsc added the NeedsFix label Jun 25, 2018

@rsc rsc added NeedsDecision and removed release-blocker labels Jun 25, 2018

@gopherbot gopherbot removed the NeedsFix label Jun 25, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

@robpike clarifies that the bigger problem is not how long this takes but that it assumes it can type-check the source code at all. His suggestion is to go back to stringer NOT type-checking the source code. (This happened in https://go-review.googlesource.com/c/tools/+/40403 for #10249.)

New decision: @robpike will update stringer NOT to use go/types.

@rsc rsc changed the title x/tools/cmd/stringer: stringer has become outrageously slow x/tools/cmd/stringer: stringer should not use go/types Jun 25, 2018

@rsc rsc assigned robpike and unassigned griesemer Jun 25, 2018

@rsc rsc modified the milestones: Go1.11, Unreleased Jun 25, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jul 3, 2018

Change https://golang.org/cl/121884 mentions this issue: cmd/stringer: revert CL 40403

gopherbot pushed a commit to golang/tools that referenced this issue Jul 3, 2018

cmd/stringer: revert CL 40403
Revert "cmd/stringer: use source importer when available"

This reverts CL 40403.

The idea is to avoid type-checking and use just parsing, which should be
enough for stringer.

Separately reopening golang/go#10249 because the original change closed that issue,
but the change is itself causing other problems as described in the discussion
at golang/go#25650.

This reversion restores the old behavior of stringer and will be followed
with other fixes if they can be worked out.

Change-Id: I8404d78da08043ede1a36b0e135a3fc7fdf6728d
Reviewed-on: https://go-review.googlesource.com/121884
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@robpike

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

Note that it must use go/types (I've paged the stack back into my small brain) so that it can evaluate constants, which requires type checking, but perhaps it can be done efficiently.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

@robpike what about using Alan's go/packages as Russ suggested above? The caching should help make it fast enough, I imagine.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

Is the go/packages thing published somewhere to look at?

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

@cznic https://go-review.googlesource.com/c/tools/+/116359, though there isn't any published code yet.

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

Hoping to send out working code today. It's been a fractal of details.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 6, 2018

Change https://golang.org/cl/122535 mentions this issue: cmd/stringer: revert back to source importer

gopherbot pushed a commit to golang/tools that referenced this issue Jul 7, 2018

cmd/stringer: revert back to source importer
Roll back my two recent changes. Stringer is now very slow again,
but works in most use cases.

My git foo is insufficient to do this as a revert, but it is a by-hand
reversion of CLs

	https://go-review.googlesource.com/121884
	https://go-review.googlesource.com/121995

See the issue for a long conversation about the general problem.

Update golang/go#10249
Update golang/go#25650

Change-Id: I7b6ce352a4c7ebf0977883509e9d7189aaac1251
Reviewed-on: https://go-review.googlesource.com/122535
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

SOF3 pushed a commit to SOF3/go-stringer-inverse that referenced this issue Aug 23, 2018

cmd/stringer: revert CL 40403
Revert "cmd/stringer: use source importer when available"

This reverts CL 40403.

The idea is to avoid type-checking and use just parsing, which should be
enough for stringer.

Separately reopening golang/go#10249 because the original change closed that issue,
but the change is itself causing other problems as described in the discussion
at golang/go#25650.

This reversion restores the old behavior of stringer and will be followed
with other fixes if they can be worked out.

Change-Id: I8404d78da08043ede1a36b0e135a3fc7fdf6728d
Reviewed-on: https://go-review.googlesource.com/121884
Reviewed-by: Ian Lance Taylor <iant@golang.org>

SOF3 pushed a commit to SOF3/go-stringer-inverse that referenced this issue Aug 23, 2018

cmd/stringer: revert back to source importer
Roll back my two recent changes. Stringer is now very slow again,
but works in most use cases.

My git foo is insufficient to do this as a revert, but it is a by-hand
reversion of CLs

	https://go-review.googlesource.com/121884
	https://go-review.googlesource.com/121995

See the issue for a long conversation about the general problem.

Update golang/go#10249
Update golang/go#25650

Change-Id: I7b6ce352a4c7ebf0977883509e9d7189aaac1251
Reviewed-on: https://go-review.googlesource.com/122535
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

SOF3 pushed a commit to SOF3/go-stringer-inverse that referenced this issue Aug 23, 2018

cmd/stringer: revert CL 40403
Revert "cmd/stringer: use source importer when available"

This reverts CL 40403.

The idea is to avoid type-checking and use just parsing, which should be
enough for stringer.

Separately reopening golang/go#10249 because the original change closed that issue,
but the change is itself causing other problems as described in the discussion
at golang/go#25650.

This reversion restores the old behavior of stringer and will be followed
with other fixes if they can be worked out.

Change-Id: I8404d78da08043ede1a36b0e135a3fc7fdf6728d
Reviewed-on: https://go-review.googlesource.com/121884
Reviewed-by: Ian Lance Taylor <iant@golang.org>

SOF3 pushed a commit to SOF3/go-stringer-inverse that referenced this issue Aug 23, 2018

cmd/stringer: revert back to source importer
Roll back my two recent changes. Stringer is now very slow again,
but works in most use cases.

My git foo is insufficient to do this as a revert, but it is a by-hand
reversion of CLs

	https://go-review.googlesource.com/121884
	https://go-review.googlesource.com/121995

See the issue for a long conversation about the general problem.

Update golang/go#10249
Update golang/go#25650

Change-Id: I7b6ce352a4c7ebf0977883509e9d7189aaac1251
Reviewed-on: https://go-review.googlesource.com/122535
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@golang golang deleted a comment from Wolfkid200444 Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.