overhaul how we load module info to format Go files #211
Conversation
|
@screwyprof care to confirm that this fix works for you? |
|
note to self: add benchcmd benchmark numbers to the commit message before merging. I imagine we're slightly faster than before on average, due to the deduplication of |
|
@mvdan I've tested it on my setup. Works just fine with |
|
Thank you both! Merging on green. @screwyprof I'll release v0.3.1 sometime next week; you can use master until then if you wish. |
We would call "go mod edit -json" for each Go file we formatted, as each file may be in a different directory, and thus inside a different module. However, the first mistake is that we always ran the command in the directory where gofumpt is run, not the directory containing each Go file. Our tests weren't strict enough to catch this; now octal-literal.txt is, via its run of gofmt before it calls cd. The second mistake, and a pretty embarrassing one, is that since v0.3.0 made gofumpt concurrent, it has been racy in how it writes to globals like langVersion from multiple goroutines. octal-literals.txt now tests for this by adding a nested Go module. Finally, we could also run into open file limits, because spawning a child process and grabbing its output opens files of its own such as named pipes. The added test shows this with a limit of 256 and 10k tiny Go files: --- FAIL: TestWithLowOpenFileLimit (0.30s) ulimit_unix_test.go:82: writing 10000 tiny Go files ulimit_unix_test.go:104: running with 1 paths ulimit_unix_test.go:104: running with 10000 paths ulimit_unix_test.go:112: error: got non-nil error comment: stderr: open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/014.go: too many open files open /tmp/TestWithLowOpenFileLimit2753748366/001/p003/017.go: too many open files open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/000.go: too many open files open /tmp/TestWithLowOpenFileLimit2753748366/001/p004/019.go: too many open files Instead, only call "go mod edit -json" once per directory, and do it in the main thread to reduce its parallelism. Also make it grab fdSem as well, for good measure. This may not be a complete fix, as we're not sure how many files are open by an exec.Command.Output call. However, we are no longer able to reproduce a failure, so leave that as a TODO. Fixes #208.
|
I had to redo part of the patch, because the old code was racy (multiple goroutines writing to globals) and didn't actually call I'm less inclined to include benchmark numbers now, because master is racy, so it currently cheats by "caching" global values with module info, but in a racy way that's completely broken, so its performance is kinda random. |
mvdan
changed the title
(see commit message)
Fixes #208.