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/imports: use named imports for all import path/package name mismatches #28428

Closed
heschi opened this issue Oct 26, 2018 · 17 comments
Closed

Comments

@heschi
Copy link
Contributor

heschi commented Oct 26, 2018

Some packages' import paths don't match their package name. The only way to know for sure what package an import path contains is to parse it a little. That means that you can't tell just by looking at a Go file whether all its imports are satisfied, because you don't know for sure which import statements provide which package names.

goimports does its parsing in the importPathToNameGoPathParse function. As the name suggests, it's GOPATH-specific. In module mode, mapping an import path to a package is much more costly -- you have to run go list, perhaps via go/packages. Having that call in the middle of the fast path of goimports is not ideal, especially today where it can take a good chunk of a second to run.

After talking about this with @ianthehat, our proposal is that goimports add an import alias for all mismatched packages. (Version suffixes would be okay.) That would make the fast path of goimports purely syntactic, so it can be even faster than today. We think it's also clearer for readers.

@bradfitz, does this sound good to you? It may be pretty noticeable to some users but hopefully they'll like it.

@gopherbot gopherbot added this to the Unreleased milestone Oct 26, 2018
@bradfitz
Copy link
Contributor

I feel like this was discussed elsewhere. I'll try to find it later.

I'm not entirely opposed to it. One additional benefit is that it kinda nudges people towards making them match, since their callers' imports will be uglier otherwise.

But I'm a little concerned that you might be proposing that the move to go/packages will make this path slower, at least on the first run (before it adds the proposed alias hint).

I've spent a fair bit of time making sure goimports is fast (the directory fastwalk stuff, the heuristics on which order we parse packages in to get a match sooner than later to minimize I/O and parsing, etc). I don't want to throw those speed benefits out with some move to an abstraction layer.

If anything, can't go/packages cache aggressively somewhere? disk/memory?

@heschi
Copy link
Contributor Author

heschi commented Oct 26, 2018

Sorry, hopefully not too redundant.

Yeah, moving it to go/packages is a big slowdown today, something like a factor of 20. It's not good. Russ has mentioned some thoughts about adding an on-disk cache to go list but nobody has immediate plans to do anything AFAIK.

We wouldn't merge go/packages into goimports until the performance hit is lower, but shelling out to go list will always have some cost. Also, we want to give early adopters a version of goimports that works with modules. If we don't make a change like this, it'll be annoyingly slow, but if we make the change only to the modules version then the two versions will conflict.

Basically, while this isn't a magic bullet, it simplifies the goimports code, makes the module version a lot faster in the relevant cases, the non-module version a tiny bit faster in all cases, and IMO makes the code more readable. So I think it's worth doing.

the heuristics on which order we parse packages in

For the record, we do lose this. I don't think there's any way to avoid it without calling go list multiple times, which is almost certainly worse than just biting the bullet.

@bradfitz
Copy link
Contributor

This worries me.

Before goimports moves to a new in-development abstraction layer, I think the new abstraction layer needs to consider the needs of one of its early users and be designed (& implemented) accordingly.

20x or even 2x performance loss is pretty sad. 2x would be more palatable, though.

@heschi
Copy link
Contributor Author

heschi commented Oct 29, 2018

I haven't been in the strategic discussions, but my understanding is that Russ wants go list to be the only API for accessing package information. goimports by definition needs to load first the package being analyzed, and then (if necessary) the candidate imports. Those calls are almost the entire performance difference, accounting for between 70-95% of the total runtime of the test cases I looked at. There is very little room for optimization of the way go list is called. There might be some slack in the loading of type information, but it's not the main problem right now.

Given that, I don't think it's helpful to blame the slowdown on go/packages. If go list is the only way to access package information in module information, and go list is slow, then everything that supports modules is going to be slow, regardless of any abstraction layers.

Again, nobody is saying that this is going to be merged until the performance is better. We are taking the same experimental approach with many other tools, though admittedly most of them are less performance sensitive or run as a server with some form of in-memory caching. See #24661.

@bradfitz
Copy link
Contributor

Given that, I don't think it's helpful to blame the slowdown on go/packages

I'm not blaming any specific piece. I haven't been following closely enough to even be able to do that. When I say "go/packages" I just mean "all the new stuff", whatever that may be. I just want the new stuff to coordinate with its own pieces to be able to do what goimports needs, quickly.

If we have to go through go list, then maybe go list needs some sort of hint/sort/streaming flags. I don't know because I don't know any of the designs.

@heschi
Copy link
Contributor Author

heschi commented Oct 29, 2018

Okay, but that's not what this bug was asking about. go list is not going to get faster before 1.13 at the earliest, and we still need to give early adopters something before then. We can make this change in all versions of goimports, just the modules version, or tell people to live with it/fix it manually. Do you have any opinion on that question?

@bradfitz
Copy link
Contributor

I'm fine with always adding the aliases in any/all versions of goimports, and doing so at any time.

Sorry for the tangent. This bug just made me (perhaps rightfully) scared of upcoming performance problems.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/145699 mentions this issue: imports: create named imports for name/path mismatches

@dgryski
Copy link
Contributor

dgryski commented Oct 31, 2018

I wonder if common mismatches should be ignored? There are a lot of repos out there with a prefix of go- or a suffix of -go.

@heschi
Copy link
Contributor Author

heschi commented Oct 31, 2018

I'm open to the idea, but IMO we need some principle behind what we do and don't make exceptions for. Without that, this feels like a slippery slope to me. I would rather add a few spurious aliases than confuse people.

That said, dashes are a relatively easy case, since they're not valid in package names. There's currently some logic that handles them by removing the dashes altogether and doing a substring match; I think I'd rather do something like check every combination of fragments. It'll complicate the code a bit, but it wouldn't be the end of the world.

@heschi
Copy link
Contributor Author

heschi commented Nov 2, 2018

I'm looking to submit CL 145699 soon. I'm guessing from the lack of comments that there aren't strong feelings here, so my inclination is to try the simplest thing and see how it goes. Please speak now if you disagree.

@heschi heschi closed this as completed Nov 2, 2018
@magical
Copy link
Contributor

magical commented Nov 3, 2018

@heschik Did you mean to close this issue?

@bradfitz
Copy link
Contributor

bradfitz commented Nov 5, 2018

Reopening.

@heschi
Copy link
Contributor Author

heschi commented Nov 7, 2018

It's been a week with no comment so I'm going to go ahead and submit.

(@magical, thanks for pointing out that I'd closed the issue, I had no idea that would happen and didn't get email about it)

@heschi
Copy link
Contributor Author

heschi commented Nov 7, 2018

Rolled back for #28645.

@heschi heschi reopened this Nov 7, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/148878 mentions this issue: imports: create named imports for name/path mismatches (again)

@dmitshur dmitshur changed the title x/tools/imports: create aliases for all import path/package name mismatches x/tools/imports: create named imports for all import path/package name mismatches Nov 9, 2018
@dmitshur dmitshur changed the title x/tools/imports: create named imports for all import path/package name mismatches x/tools/imports: use named imports for all import path/package name mismatches Nov 9, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/152000 mentions this issue: imports: create named imports for name/path mismatches (again)

opencontrail-ci-admin pushed a commit to Juniper/contrail that referenced this issue Dec 4, 2018
goimports has been changed to add named imports
when the imported package path does not match the package name:
golang/go#28428.

We did not use named imports in such cases,
so this broke the lint check in CI.

Apply "goimports -w" to make it happy.

Change-Id: I7f55aa34db9742cb778f0d91ea6a43fc5d4799c6
Partial-Bug: #1795683
harmony-ek pushed a commit to harmony-ek/harmony that referenced this issue Jan 25, 2019
Recent version of goimports committed a few days ago now adds an
explicit package name tag for imports whose last path component differ
from the package name (see golang/go#28428 and
https://go-review.googlesource.com/c/tools/+/152000).
siggy added a commit to linkerd/linkerd2 that referenced this issue Feb 24, 2019
goimports checks import lines, adding missing ones and removing
unreferenced ones:
https://godoc.org/golang.org/x/tools/cmd/goimports

It also requires named imports for packages whose
import paths don't match their package names:
- golang/go#28428
- https://go-review.googlesource.com/c/tools/+/145699/

Also standardized named imports of common Kubernetes packaages.

Part of #217

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit to linkerd/linkerd2 that referenced this issue Feb 25, 2019
goimports checks import lines, adding missing ones and removing
unreferenced ones:
https://godoc.org/golang.org/x/tools/cmd/goimports

It also requires named imports for packages whose
import paths don't match their package names:
- golang/go#28428
- https://go-review.googlesource.com/c/tools/+/145699/

Also standardized named imports of common Kubernetes packaages.

Part of #217

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit to linkerd/linkerd2 that referenced this issue Feb 25, 2019
goimports checks import lines, adding missing ones and removing
unreferenced ones:
https://godoc.org/golang.org/x/tools/cmd/goimports

It also requires named imports for packages whose
import paths don't match their package names:
- golang/go#28428
- https://go-review.googlesource.com/c/tools/+/145699/

Also standardized named imports of common Kubernetes packaages.

Part of #217

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
bwplotka added a commit to thanos-io/thanos that referenced this issue May 14, 2019
Main change is the import alias: golang/go#28428
Quite annoying but nothing much we can do.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
bwplotka added a commit to thanos-io/thanos that referenced this issue May 15, 2019
Main change is the import alias: golang/go#28428
Quite annoying but nothing much we can do.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
bwplotka added a commit to thanos-io/thanos that referenced this issue May 15, 2019
Main change is the import alias: golang/go#28428
Quite annoying but nothing much we can do.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
@golang golang locked and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants