cmd/go: remove double parallelism from "go fmt"

Now that gofmt knows how to format many files in parallel,
there's no need for "go fmt" to have its own parallelism.
Instead of running "gofmt -l -w $file" in parallel with GOMAXPROCS,
simply collect a large list of files and hand it to "gofmt -l -w $files".

The benchmark below was obtained via:

	benchcmd -n 10 FmtGorootCmd go fmt cmd

We can see a drastic improvement in system time per call.
This makes sense, as we used to fork+exec one gofmt program per file,
and now we only do that for every thousand or so files.

We also see an increase in peak memory usage and user CPU time.
This seems to be because each gofmt process was very short lived before.
This meant that there was a limit to the total amount of allocations
produced by go/parser and go/printer before the process stopped,
and thus the GC probably didn't kick in most of the time.

Now that each gofmt process formats hundreds or thousands of files,
a lot of those allocations pile up in the same process,
making peak-RSS go higher and piling on garbage for the GC to clean up.

Finally, note that time/op seems largely unchanged.
I did many benchmark runs; some ended up in noise like the one below,
and others gave small wall time speed-ups of 3-4%.
It seems like we get very little wall time benefit,
possibly due to the factors mentioned earlier cancelling each other out.

Overall, it seems worthwhile to not let "go fmt" do its own parallelism,
to keep the tool simpler to understand and maintain going forward.
Plus, the sys-time savings do seem to be the biggest change here.

	name          old time/op         new time/op         delta
	FmtGorootCmd          850ms ± 4%          842ms ± 6%      ~     (p=0.529 n=10+10)

	name          old user-time/op    new user-time/op    delta
	FmtGorootCmd          7.30s ± 4%          7.67s ± 3%    +5.07%  (p=0.000 n=10+10)

	name          old sys-time/op     new sys-time/op     delta
	FmtGorootCmd          1.66s ± 7%          0.43s ±24%   -74.08%  (p=0.000 n=10+10)

	name          old peak-RSS-bytes  new peak-RSS-bytes  delta
	FmtGorootCmd         30.1MB ± 4%        199.4MB ±21%  +563.03%  (p=0.000 n=10+10)

To make use of the already-present "maximum exec arg length limit"
constant in cmd/go/internal, move it to cmd/internal.

Fixes #43566.

Change-Id: If864151d0c851a40bf7138f9864640f15a066d48
Reviewed-on: https://go-review.googlesource.com/c/go/+/353309
Trust: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index b13191f678ddb9f4ea8671768a800067185882e0..07e99628965bde494161d3a3a46d296ee95298e5 100644 (file)
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -31,7 +31,6 @@ import (
        "cmd/go/internal/cache"
        "cmd/go/internal/cfg"
        "cmd/go/internal/robustio"
-       "cmd/go/internal/work"
        "cmd/internal/sys"
 )
 
@@ -1378,7 +1377,7 @@ func TestLdFlagsLongArgumentsIssue42295(t *testing.T) {
                }`)
        testStr := "test test test test test \n\\ "
        var buf bytes.Buffer
-       for buf.Len() < work.ArgLengthForResponseFile+1 {
+       for buf.Len() < sys.ExecArgLengthLimit+1 {
                buf.WriteString(testStr)
        }
        tg.run("run", "-ldflags", fmt.Sprintf(`-X "main.extern=%s"`, buf.String()), tg.path("main.go"))

diff --git a/src/cmd/go/internal/fmtcmd/fmt.go b/src/cmd/go/internal/fmtcmd/fmt.go
index 2b89a078acce4a3d7e7c94db8b99f614faadaece..19656eab7fc661c4cea1fabf55630f38ab2b3c98 100644 (file)
--- a/src/cmd/go/internal/fmtcmd/fmt.go
+++ b/src/cmd/go/internal/fmtcmd/fmt.go
@@ -11,14 +11,12 @@ import (
        "fmt"
        "os"
        "path/filepath"
-       "runtime"
-       "sync"
 
        "cmd/go/internal/base"
        "cmd/go/internal/cfg"
        "cmd/go/internal/load"
        "cmd/go/internal/modload"
-       "cmd/internal/str"
+       "cmd/internal/sys"
 )
 
 func init() {
@@ -53,18 +51,13 @@ See also: go fix, go vet.
 func runFmt(ctx context.Context, cmd *base.Command, args []string) {
        printed := false
        gofmt := gofmtPath()
-       procs := runtime.GOMAXPROCS(0)
-       var wg sync.WaitGroup
-       wg.Add(procs)
-       fileC := make(chan string, 2*procs)
-       for i := 0; i < procs; i++ {
-               go func() {
-                       defer wg.Done()
-                       for file := range fileC {
-                               base.Run(str.StringList(gofmt, "-l", "-w", file))
-                       }
-               }()
-       }
+
+       gofmtArgs := []string{gofmt, "-l", "-w"}
+       gofmtArgLen := len(gofmt) + len(" -l -w")
+
+       baseGofmtArgs := len(gofmtArgs)
+       baseGofmtArgLen := gofmtArgLen
+
        for _, pkg := range load.PackagesAndErrors(ctx, load.PackageOpts{}, args) {
                if modload.Enabled() && pkg.Module != nil && !pkg.Module.Main {
                        if !printed {
@@ -89,11 +82,18 @@ func runFmt(ctx context.Context, cmd *base.Command, args []string) {
                // not to packages in subdirectories.
                files := base.RelPaths(pkg.InternalAllGoFiles())
                for _, file := range files {
-                       fileC <- file
+                       gofmtArgs = append(gofmtArgs, file)
+                       gofmtArgLen += 1 + len(file) // plus separator
+                       if gofmtArgLen >= sys.ExecArgLengthLimit {
+                               base.Run(gofmtArgs)
+                               gofmtArgs = gofmtArgs[:baseGofmtArgs]
+                               gofmtArgLen = baseGofmtArgLen
+                       }
                }
        }
-       close(fileC)
-       wg.Wait()
+       if len(gofmtArgs) > baseGofmtArgs {
+               base.Run(gofmtArgs)
+       }
 }
 
 func gofmtPath() string {

diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index 692d39452051674bb635a41543620627ed5daeec..99e92947ee5dff7669439c6a051cacce0ba0a2f9 100644 (file)
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -36,6 +36,7 @@ import (
        "cmd/go/internal/modload"
        "cmd/go/internal/trace"
        "cmd/internal/str"
+       "cmd/internal/sys"
 )
 
 // actionList returns the list of actions in the dag rooted at root
@@ -3307,12 +3308,6 @@ func passLongArgsInResponseFiles(cmd *exec.Cmd) (cleanup func()) {
        return cleanup
 }
 
-// Windows has a limit of 32 KB arguments. To be conservative and not worry
-// about whether that includes spaces or not, just use 30 KB. Darwin's limit is
-// less clear. The OS claims 256KB, but we've seen failures with arglen as
-// small as 50KB.
-const ArgLengthForResponseFile = (30 << 10)
-
 func useResponseFile(path string, argLen int) bool {
        // Unless the program uses objabi.Flagparse, which understands
        // response files, don't use response files.
@@ -3324,7 +3319,7 @@ func useResponseFile(path string, argLen int) bool {
                return false
        }
 
-       if argLen > ArgLengthForResponseFile {
+       if argLen > sys.ExecArgLengthLimit {
                return true
        }
 

diff --git a/src/cmd/go/internal/work/exec_test.go b/src/cmd/go/internal/work/exec_test.go
index 4eb762cb289d28394668f501d1f54b2902da651e..8bbf25bb337e8b44e49cf56ae3dd8dfba4aafcf7 100644 (file)
--- a/src/cmd/go/internal/work/exec_test.go
+++ b/src/cmd/go/internal/work/exec_test.go
@@ -7,6 +7,7 @@ package work
 import (
        "bytes"
        "cmd/internal/objabi"
+       "cmd/internal/sys"
        "fmt"
        "math/rand"
        "testing"
@@ -56,7 +57,7 @@ func TestEncodeDecodeFuzz(t *testing.T) {
        }
        t.Parallel()
 
-       nRunes := ArgLengthForResponseFile + 100
+       nRunes := sys.ExecArgLengthLimit + 100
        rBuffer := make([]rune, nRunes)
        buf := bytes.NewBuffer([]byte(string(rBuffer)))
 
@@ -67,7 +68,7 @@ func TestEncodeDecodeFuzz(t *testing.T) {
        for i := 0; i < 50; i++ {
                // Generate a random string of runes.
                buf.Reset()
-               for buf.Len() < ArgLengthForResponseFile+1 {
+               for buf.Len() < sys.ExecArgLengthLimit+1 {
                        var r rune
                        for {
                                r = rune(rng.Intn(utf8.MaxRune + 1))

diff --git a/src/cmd/internal/sys/args.go b/src/cmd/internal/sys/args.go
new file mode 100644 (file)
index 0000000..cc9fb64
--- /dev/null
+++ b/src/cmd/internal/sys/args.go
@@ -0,0 +1,13 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package sys
+
+// ExecArgLengthLimit is the number of bytes we can safely
+// pass as arguments to an exec.Command.
+//
+// Windows has a limit of 32 KB. To be conservative and not worry about whether
+// that includes spaces or not, just use 30 KB. Darwin's limit is less clear.
+// The OS claims 256KB, but we've seen failures with arglen as small as 50KB.
+const ExecArgLengthLimit = (30 << 10)