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/gofmt: format files in parallel, similar to 'go fmt' #43566

Open
mvdan opened this issue Jan 7, 2021 · 9 comments
Open

cmd/gofmt: format files in parallel, similar to 'go fmt' #43566

mvdan opened this issue Jan 7, 2021 · 9 comments

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented Jan 7, 2021

There are two distinct formatting tools in a Go release; go fmt, which works on packages, and gofmt, which works on files. When go fmt ./... gets called, it ends up spawning one gofmt process for each file to format in each package. The concurrency is limited to the number of available CPUs, so as to format multiple files at once and go faster.

This makes sense, because gofmt is generally bound by CPU usage, not by I/O or some other factor. In the future, gofmt's CPU usage could be reduced a little, but I think it will always be bound by CPU and not the disk's read/write speed.

Unfortunately, this setup leaves gofmt being a sequential tool, to the point that it's measurably slow on large codebases - even though my CPU sits mostly idle. Using gofmt directly is also useful for two reasons:

  • It has more options, like -s, -d, and -r
  • It's more versatile; it can work on files which don't form part of any package, or format many Go modules at once

I think we should teach gofmt to use all available CPUs, using a similar mechanism to what go fmt does today. On a regular modern machine, this should make common operations like gofmt -l -w . many times faster.

go fmt could also benefit from this change. For example, we could have it call gofmt on all of the files in a package at once, reducing the number of times it has to fork and exec the other tool. An alternative could be to call gofmt in batches of files, such as 100 at a time, to perform better on tiny packages or with huge files.

I'm happy to work on this for Go 1.17, producing benchmarks for formatting all of $GOROOT/src using each tool. I did some similar work for go mod verify last year, which you can see here: https://go-review.googlesource.com/c/go/+/229817

cc @griesemer for cmd/gofmt, @bcmills @jayconrod @matloob for cmd/go

@mvdan
Copy link
Member Author

@mvdan mvdan commented Jan 7, 2021

One potential downside of this is the loss of determinism. Right now, gofmt -l -w . will generally always produce the same input, as long as the input files are the same. When adding goroutines, the output of changed files or errors will become non-deterministic.

I don't imagine this will be a problem in general, though. Plus, go fmt ./... is already non-deterministic in the order that it formats files.

If someone truly wants determinism, they could easily sort the output by the filename prefix, or write a short script/program to walk the filesystem and run gofmt sequentially on each file.

Edit: Yet another option would be to regain determinism by sorting the results of formatting each file, as opposed to printing directly to stdout. That doesn't seem worth the effort to me at least, though.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 7, 2021

Why would the output order need to be nondeterministic? We can have the goroutines work in parallel even if they block on actually emitting the output sequentially. We would lose a little bit of parallelism that way, but if emitting the output ends up on the critical path then the extra parallelism probably isn't that important anyway.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Jan 7, 2021

That's the kind of thing I meant by that last edit to "regain determinism by sorting the results". We could do it, but I'm not convinced it's worth the effort given that go fmt does not have it.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 7, 2021

Arguably the nondeterminism of go fmt is a bug that should be fixed. 😉

@mvdan
Copy link
Member Author

@mvdan mvdan commented Jan 7, 2021

Hm, if we go down that rabbit hole, then I imagine a handful of other tools like go mod verify should be fixed to have deterministic output as well. Though in most of those cases, it should only matter if many errors are printed, or with debug flags like -x. Do we really care about that kind of output being deterministic? I imagine not.

gofmt -l is a bit of a special case, though, because a machine capturing that output is a pretty common occurrence.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 7, 2021

I really care about the output of things like go mod verify being deterministic. I want to be able to write tests for that output, and it's much easier to write tests for things with deterministic output.

(Also, I like being able to run a command that produces a bunch of errors and work through those errors sequentially, and that's much harder to do if the output is nondeterministic and the thing I'm trying to fix might be hopping all around in it.)

@mvdan
Copy link
Member Author

@mvdan mvdan commented Jan 7, 2021

Of course, tests and humans :) You're absolutely right. I'll do it this way for gofmt and go fmt, then. We can file issues for other tools as needed.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 7, 2021

I'm ok with this idea. A faster gofmt for the common use case is a good thing.

With respect to deterministic output: I believe the most common case is gofmt -w. I'm not sure we need to ensure that the order in which the files are written (if changed) remains deterministic; though I think that could be also be achieved by waiting with writing the files until the end if need be (at some memory cost).

Having deterministic output for operations that produce (terminal) output (-l, -d) is probably a good idea.

For other tools (e.g. the gotype command go/types/gotype.go) I have found it useful to have a mode that enforces non-concurrent execution as a means to explicitly exclude concurrency as the culprit in case of bugs. This doesn't have to be exposed, but for debugging it would be nice to have a (source-code) constant that just needs to be flipped to disable concurrency.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 16, 2021

Change https://golang.org/cl/284139 mentions this issue: cmd/gofmt: format files in parallel batches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants