cmd/go: avoid flag.FlagSet.VisitAll at init time

We want to error early if GOFLAGS contains any flag that isn't known to
any cmd/go command. Thus, at init time we would recursively use VisitAll
on each of the flagsets to populate a map of all registered flags.

This was unfortunate, as populating said map constituted a whole 5% of
the run-time of 'go env GOARCH'. This is because VisitAll is pretty
expensive; it copies all the maps from the flagset's map to a slice,
sorts the slice, then does one callback per flag.

First, this was a bit wasteful. We only ever needed to query the
knownFlag map if GOFLAGS wasn't empty. If it's empty, there's no work to
do, thus we can skip the map populating work.

Second and most important, we don't actually need the map at all. A
flag.FlagSet already has a Lookup method, so we can simply recursively
call those methods for each flag in GOFLAGS. Add a hasFlag func to make
that evident.

This mechanism is different; its upfront cost is none, but it will
likely mean a handful of map lookups for each flag in GOFLAGS. However,
that tradeoff is worth it; we don't expect GOFLAGS to contain thousands
of flags. The most likely scenario is less than a dozen flags, in which
case constructing a "unified" map is not at all a net win.

One possible reason the previous mechanism was that way could be
AddKnownFlag. Thankfully, the one and only use of that API was removed
last year when Bryan cleaned up flag parsing in cmd/go.

The wins for the existing benchmark with an empty GOFLAGS are
significant:

	name         old time/op       new time/op       delta
	ExecGoEnv-8        575µs ± 1%        549µs ± 2%  -4.44%  (p=0.000 n=7+8)

	name         old sys-time/op   new sys-time/op   delta
	ExecGoEnv-8       1.69ms ± 1%       1.68ms ± 2%    ~     (p=0.281 n=7+8)

	name         old user-time/op  new user-time/op  delta
	ExecGoEnv-8       1.80ms ± 1%       1.66ms ± 2%  -8.09%  (p=0.000 n=7+8)

To prove that a relatively large number of GOFLAGS isn't getting
noticeably slower, we measured that as well, via benchcmd and GOFLAGS
containing 50 valid flags:

	GOFLAGS=$(yes -- -race | sed 50q) benchcmd -n 500 GoEnvGOFLAGS go env GOARCH

And the result, while noisy, shows no noticeable difference (note that
it measures 3ms instead of 0.6ms since it's sequential):

	name          old time/op         new time/op         delta
	GoEnvGOFLAGS         3.04ms ±32%         3.03ms ±35%    ~     (p=0.156 n=487+481)

Finally, we've improved the existing Go benchmark. Now it's parallel,
and it also reports sys-time and user-time, which are useful metrics.

Change-Id: I9b4551415cedf2f819eb184a02324b8bd919e2bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/248757
Reviewed-by: Bryan C. Mills <[email protected]>
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: mvdan

src/cmd/go/init_test.go

@@ -7,6 +7,7 @@ package main_test
 import (
 	"internal/testenv"
 	"os/exec"
+	"sync/atomic"
 	"testing"
 )
 
@@ -15,20 +16,27 @@ import (
 // the benchmark if any changes were done.
 func BenchmarkExecGoEnv(b *testing.B) {
 	testenv.MustHaveExec(b)
-	b.StopTimer()
 	gotool, err := testenv.GoTool()
 	if err != nil {
 		b.Fatal(err)
 	}
-	for i := 0; i < b.N; i++ {
-		cmd := exec.Command(gotool, "env", "GOARCH")
 
-		b.StartTimer()
-		err := cmd.Run()
-		b.StopTimer()
+	// We collect extra metrics.
+	var n, userTime, systemTime int64
 
-		if err != nil {
-			b.Fatal(err)
+	b.ResetTimer()
+	b.RunParallel(func(pb *testing.PB) {
+		for pb.Next() {
+			cmd := exec.Command(gotool, "env", "GOARCH")
+
+			if err := cmd.Run(); err != nil {
+				b.Fatal(err)
+			}
+			atomic.AddInt64(&n, 1)
+			atomic.AddInt64(&userTime, int64(cmd.ProcessState.UserTime()))
+			atomic.AddInt64(&systemTime, int64(cmd.ProcessState.SystemTime()))
 		}
-	}
+	})
+	b.ReportMetric(float64(userTime)/float64(n), "user-ns/op")
+	b.ReportMetric(float64(systemTime)/float64(n), "sys-ns/op")
 }

src/cmd/go/internal/base/base.go

@@ -56,6 +56,20 @@ var Go = &Command{
 	// Commands initialized in package main
 }
 
+// hasFlag reports whether a command or any of its subcommands contain the given
+// flag.
+func hasFlag(c *Command, name string) bool {
+	if f := c.Flag.Lookup(name); f != nil {
+		return true
+	}
+	for _, sub := range c.Commands {
+		if hasFlag(sub, name) {
+			return true
+		}
+	}
+	return false
+}
+
 // LongName returns the command's long name: all the words in the usage line between "go" and a flag or argument,
 func (c *Command) LongName() string {
 	name := c.UsageLine

src/cmd/go/internal/base/goflags.go

@@ -13,15 +13,7 @@ import (
 	"cmd/go/internal/cfg"
 )
 
-var (
-	goflags   []string                // cached $GOFLAGS list; can be -x or --x form
-	knownFlag = make(map[string]bool) // flags allowed to appear in $GOFLAGS; no leading dashes
-)
-
-// AddKnownFlag adds name to the list of known flags for use in $GOFLAGS.
-func AddKnownFlag(name string) {
-	knownFlag[name] = true
-}
+var goflags []string // cached $GOFLAGS list; can be -x or --x form
 
 // GOFLAGS returns the flags from $GOFLAGS.
 // The list can be assumed to contain one string per flag,
@@ -38,34 +30,19 @@ func InitGOFLAGS() {
 		return
 	}
 
-	// Build list of all flags for all commands.
-	// If no command has that flag, then we report the problem.
-	// This catches typos while still letting users record flags in GOFLAGS
-	// that only apply to a subset of go commands.
-	// Commands using CustomFlags can report their flag names
-	// by calling AddKnownFlag instead.
-	var walkFlags func(*Command)
-	walkFlags = func(cmd *Command) {
-		for _, sub := range cmd.Commands {
-			walkFlags(sub)
-		}
-		cmd.Flag.VisitAll(func(f *flag.Flag) {
-			knownFlag[f.Name] = true
-		})
+	goflags = strings.Fields(cfg.Getenv("GOFLAGS"))
+	if len(goflags) == 0 {
+		// nothing to do; avoid work on later InitGOFLAGS call
+		goflags = []string{}
+		return
 	}
-	walkFlags(Go)
 
 	// Ignore bad flag in go env and go bug, because
 	// they are what people reach for when debugging
 	// a problem, and maybe they're debugging GOFLAGS.
 	// (Both will show the GOFLAGS setting if let succeed.)
 	hideErrors := cfg.CmdName == "env" || cfg.CmdName == "bug"
 
-	goflags = strings.Fields(cfg.Getenv("GOFLAGS"))
-	if goflags == nil {
-		goflags = []string{} // avoid work on later InitGOFLAGS call
-	}
-
 	// Each of the words returned by strings.Fields must be its own flag.
 	// To set flag arguments use -x=value instead of -x value.
 	// For boolean flags, -x is fine instead of -x=true.
@@ -85,7 +62,7 @@ func InitGOFLAGS() {
 		if i := strings.Index(name, "="); i >= 0 {
 			name = name[:i]
 		}
-		if !knownFlag[name] {
+		if !hasFlag(Go, name) {
 			if hideErrors {
 				continue
 			}