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>

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"))

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 {

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
 	}
 

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))

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)