The existing translation assumes an in-memory return values, thus it returns
the address of the result(s). Most consumers immediately load from the
address to get the value, and in late call expansion that is the favored idiom,
and it is also the favored idiom when arguments and results use registers
instead of memory.
Change-Id: Ie0ccc70f399682a42509d847b330ef3956462d56
Reviewed-on: https://go-review.googlesource.com/c/go/+/240186
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Extend use of AuxCall
Change-Id: I68b6d9bad09506532e1415fd70d44cf6c15b4b93
Reviewed-on: https://go-review.googlesource.com/c/go/+/239081
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This is prerequisite to moving call expansion later into SSA,
and probably a good idea anyway. Passes tests.
This is the first minimal CL that does a 1-for-1 substitution
of *ssa.AuxCall for *obj.LSym. Next step (next CL) is to make
this change for all calls so that additional information can
be stored in AuxCall.
Change-Id: Ia3a7715648fd9fb1a176850767a726e6f5b959eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/237680
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
"cannot assign to" compiler errors are very laconic: they never
explain why the lhs cannot be assigned to (with one exception, when
assigning to a struct field in a map).
This change makes them a little more specific, in two more cases: when
assigning to a string, or to a const; by giving a very brief reason
why the lhs cannot be assigned to.
Change-Id: I244cca7fc3c3814e00e0ccadeec62f747c293979
Reviewed-on: https://go-review.googlesource.com/c/go/+/255199
Trust: Alberto Donizetti <alb.donizetti@gmail.com>
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
I just submitted CL 255297 which mostly fixed this problem, but totally
forgot to actually acquire/release the heap lock. Oops.
Updates #41391.
Change-Id: I45b42f20a9fc765c4de52476db3654d4bfe9feb3
Reviewed-on: https://go-review.googlesource.com/c/go/+/255298
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Update #40954
Change-Id: Ifaab7349631ccb12fc892882bbdf7f0ebf3d845f
Reviewed-on: https://go-review.googlesource.com/c/go/+/251158
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
They can't reasonably be allocated on the heap. Not a huge deal, but
it has an interesting and useful side effect.
After CL 249917, the compiler and runtime treat pointers to
go:notinheap types as uintptrs instead of real pointers (no write
barrier, not processed during stack scanning, ...). That feature is
exactly what we want for cgo to fix#40954. All the cases we have of
pointers declared in C, but which might actually be filled with
non-pointer data, are of this form (JNI's jobject heirarch, Darwin's
CFType heirarchy, ...).
Fixes#40954
Change-Id: I44a3b9bc2513d4287107e39d0cbbd0efd46a3aae
Reviewed-on: https://go-review.googlesource.com/c/go/+/250940
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The alias doesn't need to be marked go:notinheap. It gets its
notinheap-ness from the target type.
Without this change, the type alias test in the notinheap.go file
generates these two errors:
notinheap.go:62: misplaced compiler directive
notinheap.go:63: type nih must be go:notinheap
The first is a result of go:notinheap pragmas not applying
to type alias declarations.
The second is the result of then trying to match the notinheap-ness
of the alias and the target type.
Add a few more go:notinheap tests while we are here.
Update #40954
Change-Id: I067ec47698df6e9e593e080d67796fd05a1d480f
Reviewed-on: https://go-review.googlesource.com/c/go/+/250939
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Trust: Keith Randall <khr@golang.org>
CL 249917 made the mspan in MSpanCountAlloc no longer stack-allocated
(for good reason), but then allocated an mspan on each call and did not
free it, resulting in a leak. That allocation was also not protected by
the heap lock, which could lead to data corruption of mheap fields and
the spanalloc.
To fix this, export some functions to allocate/free dummy mspans from
spanalloc (with proper locking) and allocate just one up-front for the
benchmark, freeing it at the end. Then, update MSpanCountAlloc to accept
a dummy mspan.
Note that we need to allocate the dummy mspan up-front otherwise we
measure things like heap locking and fixalloc performance instead of
what we actually want to measure: how fast we can do a popcount on the
mark bits.
Fixes#41391.
Change-Id: If6629a6ec1ece639c7fb78532045837a8c872c04
Reviewed-on: https://go-review.googlesource.com/c/go/+/255297
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Michael Knyszek <mknyszek@google.com>
Change-Id: I54976b004b4db006509f5e0781b1c2e46cfa09ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/244577
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Joel Sing <joel@sing.id.au>
Illumos provides the pipe2 syscall. Add a wrapper to
internal/syscall/unix and use it to implement os.Pipe.
Change-Id: I26ecdbcae1e8d51f80e2bc8a86fb129826387b1f
Reviewed-on: https://go-review.googlesource.com/c/go/+/254981
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Use a common runtime slicecopy function to copy strings or slices
into slices. This deduplicates similar code previously used in
reflect.slicecopy and runtime.stringslicecopy.
Change-Id: I09572ff0647a9e12bb5c6989689ce1c43f16b7f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/254658
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Martin Möhrmann <moehrmann@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
The cgo tool would sometimes emit a bitfield at an offset that did not
correspond to the C offset, such as for the example in the new test.
Change-Id: I61b2ca10ee44a42f81c13ed12865f2060168fed5
Reviewed-on: https://go-review.googlesource.com/c/go/+/252378
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Fixes#37255
Change-Id: Ic0fde3498afefed6e4447f8476e4da7c1faa7145
Reviewed-on: https://go-review.googlesource.com/c/go/+/219640
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Giovanni Bajo <rasky@develer.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
For #37438
For #41315
For #36460
Change-Id: I17041c35ec91ff6ffb547e0f32572673d191b1ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/254820
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
For #40728
Change-Id: I6618f1b5a632e8b353a483a83bb0cdf4ef6df72c
Reviewed-on: https://go-review.googlesource.com/c/go/+/251881
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Due to an inverted condition, we were emitting a "matched no packages"
warning twice in some cases and not at all in others.
For #41315
Change-Id: I472cd2d4f75811c8734852f2bdd7346f4c612816
Reviewed-on: https://go-review.googlesource.com/c/go/+/254819
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
In https://golang.org/cl/221397 we made commands like "go version -v"
error, since both of the command's flags only make sense when arguments
follow them. Without arguments, the command only reports Go's own
version, and the flags are most likely a mistake.
However, the script below is entirely reasonable:
export GOFLAGS=-v # make all Go commands verbose
go version
go build
After the previous CL, "go version" would error. Instead, only error if
the flag was passed explicitly, and not via GOFLAGS.
The patch does mean that we won't error on "GOFLAGS=-v go version -v",
but that very unlikely false negative is okay. The error is only meant
to help the user not misuse the flags, anyway - it's not a critical
error of any sort.
To reuse inGOFLAGS, we move it to the base package and export it there,
since it's where the rest of the GOFLAGS funcs are.
Fixes#41264.
Change-Id: I74003dd25d94bacf9ac507b5cad778fd65233321
Reviewed-on: https://go-review.googlesource.com/c/go/+/254157
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>
Since CL 254369, 'go mod graph' now reports an error when invoked
outside a module. This broke the mod_outside test, which expected
'go mod graph' to succeed with no output.
Change-Id: Ic30ee68f1f4c4d33795bdf7df70a7631fb9395e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/255017
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
This makes error reporting a bit more consistent for 'go mod'
subcommands. Most of these commands only work in module mode when a
go.mod file is present.
Setting modload.ForceUseModules reports an error when GO111MODULE=off.
Setting modload.RootMode to modload.NeedRoot reports an error when no
go.mod file is present.
Change-Id: I1daa8d2971cb8658e0c804765839d903734a412e
Reviewed-on: https://go-review.googlesource.com/c/go/+/254369
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
With this change, 'go install' will install executables in module mode
without using or modifying the module in the current directory, if
there is one.
For #40276
Change-Id: I922e71719b3a4e0c779ce7a30429355fc29930bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/254365
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Illumos supports iovec read/write. Add the writev wrapper to
internal/syscall/unix and use it to implement internal/poll.writev for
net.(*netFD).writeBuffers.
Change-Id: Ie256c2f96aba8e61fb21991788789a049425f792
Reviewed-on: https://go-review.googlesource.com/c/go/+/254638
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Currently, there's awkward reentrancy issue with funccompile:
funccompile -> compile -> dtypesym -> geneq/genhash/genwrapper -> funccompile
Though it's not a problem at this moment, some attempts by @mdempsky to
move order/walk/instrument into buildssa was failed, due to SSA cache
corruption.
This commit fixes that reentrancy issue, by making generated functions
to be pumped through the same compile workqueue that normal functions
are compiled. We do this by adding them to xtop, instead of calling
funccompile directly in geneq/genhash/genwrapper. In dumpdata, we look
for uncompiled functions in xtop instead of compilequeue, then finish
compiling them.
Updates #38463Fixes#33485
Change-Id: Ic9f0ce45b56ae2ff3862f17fd979253ddc144bb5
Reviewed-on: https://go-review.googlesource.com/c/go/+/254617
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
This allows the global initializers function to go through normal
mid-end optimizations (e.g., inlining, escape analysis) like any other
function.
Updates #33485.
Change-Id: I9bcfe98b8628d1aca09b4c238d8d3b74c69010a5
Reviewed-on: https://go-review.googlesource.com/c/go/+/254839
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
funchdr and funcbody currently assume that either (1) Curfn == nil &&
dclcontext == PEXTERN, or (2) Curfn != nil && dclcontext == PAUTO.
This is a reasonable assumption during parsing. However, these
functions end up getting used in other contexts, and not all callers
are so disciplined about Curfn/dclcontext handling.
This CL changes them to save/restore arbitrary Curfn/dclcontext pairs
instead. This is necessary for the followup CL, which pushes fninit
earlier. Otherwise, Curfn/dclcontext fall out of sync, and funchdr
panics.
Passes toolstash-check.
Updates #33485.
Change-Id: I19b1be23db1bad6475345ae5c81bbdc66291a3a7
Reviewed-on: https://go-review.googlesource.com/c/go/+/254838
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
This reverts CL 253037.
Reason for revert: The recommended way to check for a type of error is errors.As. API changes should also start with a proposal.
Change-Id: I62896717aa47ed491c2c4775d2b05d80e5e9cde3
Reviewed-on: https://go-review.googlesource.com/c/go/+/254837
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This reverts CL 254537.
Reason for revert: Reason for revert: The recommended way to check for a type of error is errors.As. API changes should also start with a proposal.
Change-Id: I07c37428575e99c80b17525833a61831d10963bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/254857
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Previously, it was impossible to call FailNow in a Cleanup.
Because it can terminate a panicking goroutine and cause its
parent hangs on t.signal channel. This CL sends the signal
in a deferred call to prevent the hang.
Fixes#41355
Change-Id: I4552d3a7ea763ef86817bf9b50c0e37fb34bf20f
Reviewed-on: https://go-review.googlesource.com/c/go/+/254637
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Emmanuel Odeke <emm.odeke@gmail.com>
As of CL 251159, when 'go list -deps std' is run within GOROOT/src, it
treats the vendored external dependencies as real module dependencies,
not standard-library "vendor/" packages (which still exist in that
case, but are treated as distinct packages outside the "std" module).
Fixes#41358
Updates #30241
Change-Id: Ic23eae9829d90e74a340d49ca9052e9191597410
Reviewed-on: https://go-review.googlesource.com/c/go/+/254738
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
The paths may contain spaces. Quote them.
Change-Id: I1f67085a1e7c40f60282c2fea7104fb44a01e310
Reviewed-on: https://go-review.googlesource.com/c/go/+/254739
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
AIX fsync syscall doesn't work on read-only files. Using fsync_range
instead allows syscall.Fsync to work on any files.
Fixes#41372
Change-Id: I66d33e847875496af53da60828c1bddf6c2b76b7
Reviewed-on: https://go-review.googlesource.com/c/go/+/254657
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This CL ensures that responses served via CGI and FastCGI
have a Content-Type header based on the content of the
response if not explicitly set by handlers.
If the implementers of the handler did not explicitly
specify a Content-Type both CGI implementations would default
to "text/html", potentially causing cross-site scripting.
Thanks to RedTeam Pentesting GmbH for reporting this.
Fixes#40928
Fixes CVE-2020-24553
Change-Id: I82cfc396309b5ab2e8d6e9a87eda8ea7e3799473
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/823217
Reviewed-by: Russ Cox <rsc@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/252179
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
CL 96455 brings CL 57753 to Windows
However, a path comparison within it was left unquoted.
If the Go source directory resides in a path containing whitespace,
the interpreter will compare against the first portion of the path string,
and treat the remainder as an expression.
This patch amends that.
For example, consider the path
`C:\Users\Dominic Della Valle\Projects\Go\goroot\src`
Issuing `make.bat` will print out `'Della' is not recognized as an internal or external command, operable program or batch file.` before proceeding.
Change-Id: Ifcec159baeec940c29c61aa721c64c13c6fd8c14
GitHub-Last-Rev: 809ddbb4db
GitHub-Pull-Request: golang/go#41319
Reviewed-on: https://go-review.googlesource.com/c/go/+/253898
Run-TryBot: Giovanni Bajo <rasky@develer.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Trust: Giovanni Bajo <rasky@develer.com>
runtime.GC() doesn't guarantee the finalizer has run, so use a channel
instead to make sure finalizer was run in call to "after()".
Fixes#41361
Change-Id: I69c801e29aea49757ea72c52e8db13239de19ddc
Reviewed-on: https://go-review.googlesource.com/c/go/+/254401
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
CL 254397 attached OVARLIVE nodes to OCALLxxx nodes Nbody.
The NeedsWrapper flag is now redundant with n.Nbody.Len() > 0
condition, so use that condition instead and remove the flag.
Passes toolstash-check.
Change-Id: Iebc3e674d3c0040a876ca4be05025943d2b4fb31
Reviewed-on: https://go-review.googlesource.com/c/go/+/254398
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
So we can insert theses OVARLIVE nodes right after OpStaticCall in SSA.
This helps fixing issue that unsafe-uintptr arguments are not kept alive
during return statement, or can be kept alive longer than expected.
Fixes#24491
Change-Id: Ic04a5d1bbb5c90dcfae65bd95cdd1da393a66800
Reviewed-on: https://go-review.googlesource.com/c/go/+/254397
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Allows users to check:
errors.Is(err, &UnmarshalTypeError{})
errors.Is(err, &UnmarshalFieldError{})
errors.Is(err, &InvalidUnmarshalError{})
errors.Is(err, &UnsupportedValueError{})
errors.Is(err, &MarshalerError{})
which is the recommended way of checking for kinds of errors.
SyntaxError.Is was implemented in CL 253037.
As and Unwrap relevant methods will be added in future CLs.
Change-Id: I1f8a503b8fdc0f3afdfe9669a91f3af8d960e028
GitHub-Last-Rev: 930cda5384
GitHub-Pull-Request: golang/go#41360
Reviewed-on: https://go-review.googlesource.com/c/go/+/254537
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Trust: Emmanuel Odeke <emm.odeke@gmail.com>
Allows users to check:
errors.Is(err, &json.SyntaxError{})
which is the recommended way of checking for kinds of errors.
Change-Id: I20dc805f20212765e9936a82d9cb7822e73ec4ef
GitHub-Last-Rev: e2627ccf8e
GitHub-Pull-Request: golang/go#41210
Reviewed-on: https://go-review.googlesource.com/c/go/+/253037
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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>
This CL changes "T literal.M" error message to "T{...}.M". It's clearer
expression and focusing user on actual issue.
Updates #38745
Change-Id: I84b455a86742f37e0bde5bf390aa02984eecc3c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/253677
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This is a refactoring intended to break the dependency from
internal/modfetch to internal/get. No change in functionality is intended.
Change-Id: If51aba7139cc0b62ecc9ba454c055c99e8f36f0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/254364
Run-TryBot: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
