commit 92b2b8860dcc28461198c6125fbae2383161d2e5
author Daniel Martí <mvdan@mvdan.cc> Sat Aug 15 16:20:50 2020 +0200
committer Daniel Martí <mvdan@mvdan.cc> Sat Sep 12 12:39:50 2020 +0000
tree bc21522df9cb0a7c491bc8ef355657431ab107f4
parent 806f478499b57c5167fb5301101961b7563903d2

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 <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>