This is part of a general effort to shrink walk.
In an ideal world, we'd have an SSA op for allocation,
but we don't yet have a good mechanism for introducing
function calling during SSA compilation.
In the meantime, SSA conversion is a better place for it.
This also makes it easier to introduce new optimizations;
instead of doing the typecheck walk dance,
we can simply write what we want the backend to do.
I introduced a new opcode in this change because:
(a) It avoids a class of bugs involving correctly detecting
whether this ONEW is a "before walk" ONEW or an "after walk" ONEW.
It also means that using ONEW or ONEWOBJ in the wrong context
will generally result in a faster failure.
(b) Opcodes are cheap.
(c) It provides a better place to put documentation.
This change also is also marginally more performant:
name old alloc/op new alloc/op delta
Template 39.1MB ± 0% 39.0MB ± 0% -0.14% (p=0.008 n=5+5)
Unicode 28.4MB ± 0% 28.4MB ± 0% ~ (p=0.421 n=5+5)
GoTypes 132MB ± 0% 132MB ± 0% -0.23% (p=0.008 n=5+5)
Compiler 608MB ± 0% 607MB ± 0% -0.25% (p=0.008 n=5+5)
SSA 2.04GB ± 0% 2.04GB ± 0% -0.01% (p=0.008 n=5+5)
Flate 24.4MB ± 0% 24.3MB ± 0% -0.13% (p=0.008 n=5+5)
GoParser 29.3MB ± 0% 29.1MB ± 0% -0.54% (p=0.008 n=5+5)
Reflect 84.8MB ± 0% 84.7MB ± 0% -0.21% (p=0.008 n=5+5)
Tar 36.7MB ± 0% 36.6MB ± 0% -0.10% (p=0.008 n=5+5)
XML 48.7MB ± 0% 48.6MB ± 0% -0.24% (p=0.008 n=5+5)
[Geo mean] 85.0MB 84.8MB -0.19%
name old allocs/op new allocs/op delta
Template 383k ± 0% 382k ± 0% -0.26% (p=0.008 n=5+5)
Unicode 341k ± 0% 341k ± 0% ~ (p=0.579 n=5+5)
GoTypes 1.37M ± 0% 1.36M ± 0% -0.39% (p=0.008 n=5+5)
Compiler 5.59M ± 0% 5.56M ± 0% -0.49% (p=0.008 n=5+5)
SSA 16.9M ± 0% 16.9M ± 0% -0.03% (p=0.008 n=5+5)
Flate 238k ± 0% 238k ± 0% -0.23% (p=0.008 n=5+5)
GoParser 306k ± 0% 303k ± 0% -0.93% (p=0.008 n=5+5)
Reflect 990k ± 0% 987k ± 0% -0.33% (p=0.008 n=5+5)
Tar 356k ± 0% 355k ± 0% -0.20% (p=0.008 n=5+5)
XML 444k ± 0% 442k ± 0% -0.45% (p=0.008 n=5+5)
[Geo mean] 848k 845k -0.33%
Change-Id: I2c36003a7cbf71b53857b7de734852b698f49310
Reviewed-on: https://go-review.googlesource.com/c/go/+/167957
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
This lets us get rid of a handful of cmd/vet/whitelist/all.txt entries,
since the stdmethods pass is now happy with the encoding/xml package.
Change-Id: I9de2190984dd00342903967262790c7f6b1f0a75
Reviewed-on: https://go-review.googlesource.com/c/go/+/168458
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
As proposed in Issue #29934, update errors produced by the os package to
work with errors.Is sentinel tests. For example,
errors.Is(err, os.ErrPermission) is equivalent to os.IsPermission(err)
with added unwrapping support.
Move the definition for os.ErrPermission and others into the syscall
package. Add an Is method to syscall.Errno and others. Add an Unwrap
method to os.PathError and others.
Updates #30322
Updates #29934
Change-Id: I95727d26c18a5354c720de316dff0bffc04dd926
Reviewed-on: https://go-review.googlesource.com/c/go/+/163058
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
Some cgo tests aren't yet available on aix/ppc64.
-shared and -static don't work as expected and will be fixed latter.
Updates #30565
Change-Id: Ic59cabe685cb1cbdf89a8d1d1a1d2c4b0e8ef442
Reviewed-on: https://go-review.googlesource.com/c/go/+/164018
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit handles AIX cgo in cmd/nm tests.
Change-Id: I6753a0102e4f2c4c7bd4d7c999f62a0cb3d2183c
Reviewed-on: https://go-review.googlesource.com/c/go/+/164017
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Add an Unwrap method to PathError so it works with the errors.Is/As
functions.
Change-Id: Ia6171c0418584f3cd53ee99d97c687941a9e3109
Reviewed-on: https://go-review.googlesource.com/c/go/+/168097
Reviewed-by: Damien Neil <dneil@google.com>
On Windows, GetFileAttributesEx fails with ERROR_SHARING_VIOLATION for
some files, like c:\pagefile.sys. In this case,
newFileStatFromWin32finddata is used to fill file info, but it does not fill
name and path.
After getting file stat from newFileStatFromWin32finddata, just set file info
name and path before return fixes the issue.
Fixes#30883
Change-Id: I654e96c634e8a9bf5ce7e1aaa93968e88953620d
Reviewed-on: https://go-review.googlesource.com/c/go/+/167779
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
It seems like we need to pay special attention to capturing error
condition on the event port of SmartOS. The previous attempt CL 167777
works on Oracle Solaris but doesn't work on SmartOS for the uncertain
reason. It's better to disable the reporting for now.
Updates #30624.
Fixes#30840.
Change-Id: Ieca5dac4fceb7e8c9cb4db149bb4c2e79691588c
Reviewed-on: https://go-review.googlesource.com/c/go/+/167782
Run-TryBot: Mikio Hara <mikioh.public.networking@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This CL instrinsifies Add64 with arm64 instruction sequence ADDS, ADCS
and ADC, and optimzes the case of carry chains.The CL also changes the
test code so that the intrinsic implementation can be tested.
Benchmarks:
name old time/op new time/op delta
Add-224 2.500000ns +- 0% 2.090000ns +- 4% -16.40% (p=0.000 n=9+10)
Add32-224 2.500000ns +- 0% 2.500000ns +- 0% ~ (all equal)
Add64-224 2.500000ns +- 0% 1.577778ns +- 2% -36.89% (p=0.000 n=10+9)
Add64multiple-224 6.000000ns +- 0% 2.000000ns +- 0% -66.67% (p=0.000 n=10+10)
Change-Id: I6ee91c9a85c16cc72ade5fd94868c579f16c7615
Reviewed-on: https://go-review.googlesource.com/c/go/+/159017
Run-TryBot: Ben Shi <powerman1st@163.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
while command line is a noun.
Change-Id: I9dfc2f6841d1171854857a8daa785d55afe2c5f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/167783
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
'SUBQ $-0x80, r' is shorter to encode than 'ADDQ $0x80, r',
and functionally equivalent. Use it instead.
Shaves off a few bytes here and there:
file before after Δ %
compile 25935856 25927664 -8192 -0.032%
nm 4251840 4247744 -4096 -0.096%
Change-Id: Ia9e02ea38cbded6a52a613b92e3a914f878d931e
Reviewed-on: https://go-review.googlesource.com/c/go/+/168344
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Flagalloc has the unenviable task of splitting
flag-generating ops that have been merged with loads
when the flags need to "spilled" (i.e. regenerated).
Since there weren't very many of them, there was a hard-coded list
of ops and bespoke code written to split them.
This change migrates load splitting into rewrite rules,
to make them easier to maintain.
Change-Id: I7750eafb888a802206c410f9c341b3133e7748b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/166978
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
This adds a ReportMetric method to testing.B that lets the user report
custom benchmark metrics and override built-in metrics.
Fixes#26037.
Change-Id: I8236fbde3683fc27bbe45cbbedfd377b435edf64
Reviewed-on: https://go-review.googlesource.com/c/go/+/166717
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
MIME sniffing standard defines the RAR signature as 52 61 72 20 1A 07 00.
But this signature is wrong, the RARlab spec defines the 4th byte must
be 0x21 or "!", not 0x20 or " ". Checking a rar file also indicates that:
$ file abc.rar
abc.rar: RAR archive data, v1d, os: Win32
$ head -c 7 abc.rar | od -v -t x1
0000000 52 61 72 21 1a 07 00
0000007
There is also an issue to fix this problem in MIME standard.
See:
- https://www.rarlab.com/technote.htm#rarsign
- https://github.com/whatwg/mimesniff/issues/63Fixes#30926
Change-Id: Id2e2de7ecbf7f44d37ebaf280efd05e4972c5078
Reviewed-on: https://go-review.googlesource.com/c/go/+/167781
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This works around the NetBSD <stdint.h> which defines the type using
"#define" rather than typedef.
Fixes#30918
Updates #29878
Change-Id: I8998eba52139366ae46762bdad5fcae85f9b4027
Reviewed-on: https://go-review.googlesource.com/c/go/+/168337
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This fixes two problems missed in CL 164877.
First, p.Internal.BuildInfo is now part of the cache key. This is
important since p.Internal.BuildInfo causes the build action to
synthesize a new source file, which affects the output.
Second, recompileForTest is always called for test
packages. Previously, it was only called when there were internal test
sources, so the fix in CL 164877 did not apply to packages that only
had external tests.
Fixes#30374
Change-Id: Iac2d7e8914f0313f9ab4222299a866f67889eb2e
Reviewed-on: https://go-review.googlesource.com/c/go/+/168200
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit removes spaces which were wrongly added in
//go:cgo_export_static during CL 164010.
Change-Id: Iadd18efdde9ff32e907d793a72ef0f9efda35fe6
Reviewed-on: https://go-review.googlesource.com/c/go/+/168317
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit fixes runtime.sigfwd for aix/ppc64. fn is a function
descriptor and not a function. R2 must be saved and restored.
Change-Id: Ie506b0bdde562ca37202d19973ba1d537c3d64e2
Reviewed-on: https://go-review.googlesource.com/c/go/+/164015
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This change makes the runtime-integrated network poller report only
critical event scanning errors.
In the previous attempt, CL 166497, we treated any combination of error
events as event scanning errors and it caused false positives in event
waiters because platform-dependent event notification mechanisms allow
event originators to use various combination of events.
To avoid false positives, this change makes the poller treat an
individual error event as a critical event scanning error by the
convention of event notification mechanism implementations.
Updates #30624.
Fixes#30817.
Fixes#30840.
Change-Id: I906c9e83864527ff73f636fd02bab854d54684ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/167777
Run-TryBot: Mikio Hara <mikioh.public.networking@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Change-Id: Icc42843adb15c2aba1cfea854fad049c6704344b
Reviewed-on: https://go-review.googlesource.com/c/go/+/164014
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
When closing a pipe, use CancelIoEx to cancel pending I/O.
This makes concurrent Read and Write calls return os.ErrClosed.
This change also enables some pipe tests on Windows.
Fixes#28477Fixes#25835
Change-Id: If52bb7d80895763488a61632e4682a78336e8420
Reviewed-on: https://go-review.googlesource.com/c/go/+/164721
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit disables new cgo pprof tests and adds an handler in sigtramp
to refuse SIGPROF signal.
Updates #28555
Change-Id: I152a871f8636e93328d411329104c6f023bd1691
Reviewed-on: https://go-review.googlesource.com/c/go/+/164013
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This is primarily achieved by checking for arithmetic overflow
instead of using Add and Equal.
It's a decent performance improvement even though
the function still isn't inlined.
name old time/op new time/op delta
Sub-6 242ns ± 0% 122ns ± 0% -49.59% (p=0.002 n=8+10)
Updates #17858.
Change-Id: I1469b618183c83ea8ea54d5ce277eb15f2ec0f11
Reviewed-on: https://go-review.googlesource.com/c/go/+/131196
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit fixes TestSigStackSwapping by increasing the signal stack
size. This is needed because SIGSTKSZ is too small when VMX is used on
AIX.
Change-Id: Ic2b5faa65745228d0768383b3d6ebd4b6f9f532c
Reviewed-on: https://go-review.googlesource.com/c/go/+/164012
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This commit adds C support for net package for aix/ppc64.
Change-Id: I704710991fc013c6ec3511671def6791df4854f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/164038
Run-TryBot: Mikio Hara <mikioh.public.networking@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Mikio Hara <mikioh.public.networking@gmail.com>
C trampolines are made by fixup CSECTS which are added between two
symbols. If such CSECTS is added inside Go functions, all method
offsets stored in moduledatas will be wrong.
In order to prevent this, every C code is moved at the end of the
executable and long calls are created for GO functions called by C
code.
The main function can't longer be made in Go as AIX __start isn't using
a long call to branch on it. Therefore, a main is defined on
runtime/cgo.
Change-Id: I214b18decdb83107cf7325b298609eef9f9d1330
Reviewed-on: https://go-review.googlesource.com/c/go/+/164010
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
When initializing a new object, we're often writing
1) to a location that doesn't have a pointer to a heap object
2) a pointer that doesn't point to a heap object
When both those conditions are true, we can avoid the write barrier.
This CL detects case 1 by looking for writes to known-zeroed
locations. The results of runtime.newobject are zeroed, and we
perform a simple tracking of which parts of that object are written so
we can determine what part remains zero at each write.
This CL detects case 2 by looking for addresses of globals (including
the types and itabs which are used in interfaces) and for nil pointers.
Makes cmd/go 0.3% smaller. Some particular cases, like the slice
literal in #29573, can get much smaller.
TODO: we can remove actual zero writes also with this mechanism.
Update #29573
Change-Id: Ie74a3533775ea88da0495ba02458391e5db26cb9
Reviewed-on: https://go-review.googlesource.com/c/go/+/156363
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
The exact-width integer types are required to use two’s complement
representation and may not have padding bits, cf. §7.20.1.1/1 in the C11
standard or https://en.cppreference.com/w/c/types/integer. This ensures that
they have the same domain and representation as the corresponding Go types.
Fixes#29878
Change-Id: Ie8a51e91666dfd89731c7859abe47356c94ca1be
GitHub-Last-Rev: 546a2cc3f1
GitHub-Pull-Request: golang/go#29907
Reviewed-on: https://go-review.googlesource.com/c/go/+/159258
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
The name change init -> init.ializers was initially required for
initialization code.
With CL 161337 there's no wrapper code any more, there's a data
structure instead (named .inittask). So we can go back to just
plain init appearing in tracebacks.
RELNOTE=yes
Update #29919. Followon to CL 161337.
Change-Id: I5a4a49d286df24b53b2baa193dfda482f3ea82a5
Reviewed-on: https://go-review.googlesource.com/c/go/+/167780
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Some of the registers in which indexes + length were supposed to
be passed were wrong.
Update #30116
Change-Id: I1089366b7429c1e0ecad9219b847db069ce6b5d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/168041
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Instead of writing an init function per package that does the same
thing for every package, just write that implementation once in the
runtime. Change the compiler to generate a data structure that encodes
the required initialization operations.
Reduces cmd/go binary size by 0.3%+. Most of the init code is gone,
including all the corresponding stack map info. The .inittask
structures that replace them are quite a bit smaller.
Most usefully to me, there is no longer an init function in every -S output.
(There is an .inittask global there, but it's much less distracting.)
After this CL we could change the name of the "init.ializers" function
back to just "init".
Update #6853
R=go1.13
Change-Id: Iec82b205cc52fe3ade4d36406933c97dbc9c01b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/161337
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
golang.org/cl/166983 started serializing the Ninit field of OCALL
nodes within function inline bodies (necessary to fix a regression in
building crypto/ecdsa with -gcflags=-l=4), but this means the Ninit
field needs to be typechecked when the imported function body is used.
It's unclear why this wasn't necessary for the crypto/ecdsa
regression.
Fixes#30907.
Change-Id: Id5f0bf3c4d17bbd6d5318913b859093c93a0a20c
Reviewed-on: https://go-review.googlesource.com/c/go/+/168199
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A few examples (for accessing a slice of length 3):
s[-1] runtime error: index out of range [-1]
s[3] runtime error: index out of range [3] with length 3
s[-1:0] runtime error: slice bounds out of range [-1:]
s[3:0] runtime error: slice bounds out of range [3:0]
s[3:-1] runtime error: slice bounds out of range [:-1]
s[3:4] runtime error: slice bounds out of range [:4] with capacity 3
s[0:3:4] runtime error: slice bounds out of range [::4] with capacity 3
Note that in cases where there are multiple things wrong with the
indexes (e.g. s[3:-1]), we report one of those errors kind of
arbitrarily, currently the rightmost one.
An exhaustive set of examples is in issue30116[u].out in the CL.
The message text has the same prefix as the old message text. That
leads to slightly awkward phrasing but hopefully minimizes the chance
that code depending on the error text will break.
Increases the size of the go binary by 0.5% (amd64). The panic functions
take arguments in registers in order to keep the size of the compiled code
as small as possible.
Fixes#30116
Change-Id: Idb99a827b7888822ca34c240eca87b7e44a04fdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/161477
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
If -o points to a directory that exists then allow multiple
executables to be written to that directory.
Fixes#14295
Change-Id: Ic951e637c70a2ada5e7534bae9a43901a39fe2c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/167679
Run-TryBot: Daniel Theophanes <kardianos@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Divisions are generally slow. The compiler can optimize a division
to use a sequence of faster multiplies and shifts (magic constants)
if the divisor is not know at compile time.
The value of the span element size in mcentral.grow is not known at
compile time but magic constants to compute n / span.elementsize
are already stored in class_to_divmagic and mspan.
They however need to be adjusted to work for
(0 <= n <= span.npages * pagesize) instead of
(0 <= n < span.npages * pagesize).
Change-Id: Ieea59f1c94525a88d012f2557d43691967900deb
Reviewed-on: https://go-review.googlesource.com/c/go/+/148057
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
In golang.org/cl/145218, a feature was added where the JSON decoder
would keep track of the entire path to a field when reporting an
UnmarshalTypeError.
However, we all failed to check if this affected the benchmarks - myself
included, as a reviewer. Below are the numbers comparing the CL's parent
with itself, once it was merged:
name old time/op new time/op delta
CodeDecoder-8 12.9ms ± 1% 28.2ms ± 2% +119.33% (p=0.002 n=6+6)
name old speed new speed delta
CodeDecoder-8 151MB/s ± 1% 69MB/s ± 3% -54.40% (p=0.002 n=6+6)
name old alloc/op new alloc/op delta
CodeDecoder-8 2.74MB ± 0% 109.39MB ± 0% +3891.83% (p=0.002 n=6+6)
name old allocs/op new allocs/op delta
CodeDecoder-8 77.5k ± 0% 168.5k ± 0% +117.30% (p=0.004 n=6+5)
The reason why the decoder got twice as slow is because it now allocated
~40x as many objects, which puts a lot of pressure on the garbage
collector.
The reason is that the CL concatenated strings every time a nested field
was decoded. In other words, practically every field generated garbage
when decoded. This is hugely wasteful, especially considering that the
vast majority of JSON decoding inputs won't return UnmarshalTypeError.
Instead, use a stack of fields, and make sure to always use the same
backing array, to ensure we only need to grow the slice to the maximum
depth once.
The original CL also introduced a bug. The field stack string wasn't
reset to its original state when reaching "d.opcode == scanEndObject",
so the last field in a decoded struct could leak. For example, an added
test decodes a list of structs, and encoding/json before this CL would
fail:
got: cannot unmarshal string into Go struct field T.Ts.Y.Y.Y of type int
want: cannot unmarshal string into Go struct field T.Ts.Y of type int
To fix that, simply reset the stack after decoding every field, even if
it's the last.
Below is the original performance versus this CL. There's a tiny
performance hit, probably due to the append for every decoded field, but
at least we're back to the usual ~150MB/s.
name old time/op new time/op delta
CodeDecoder-8 12.9ms ± 1% 13.0ms ± 1% +1.25% (p=0.009 n=6+6)
name old speed new speed delta
CodeDecoder-8 151MB/s ± 1% 149MB/s ± 1% -1.24% (p=0.009 n=6+6)
name old alloc/op new alloc/op delta
CodeDecoder-8 2.74MB ± 0% 2.74MB ± 0% +0.00% (p=0.002 n=6+6)
name old allocs/op new allocs/op delta
CodeDecoder-8 77.5k ± 0% 77.5k ± 0% +0.00% (p=0.002 n=6+6)
Finally, make all of these benchmarks report allocs by default. The
decoder ones are pretty sensitive to generated garbage, so ReportAllocs
would have made the performance regression more obvious.
Change-Id: I67b50f86b2e72f55539429450c67bfb1a9464b67
Reviewed-on: https://go-review.googlesource.com/c/go/+/167978
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Move the shared code of IndexByte and IndexByteString into
indexbytebody. This will allow to implement optimizations (e.g.
for #29001) in a single function.
Change-Id: I1d550da8eb65f95e492a460a12058cc35b1162b6
Reviewed-on: https://go-review.googlesource.com/c/go/+/167939
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
UserHomeDir always returns "/" for platforms where the home directory
is not always well defined. However, the user might set HOME before
running a Go program on those platforms and on at least iOS, HOME
is actually set to something useful (the root of the app specific
writable directory).
This CL changes UserHomeDir to use the root directory "/" only if
$HOME is empty.
Change-Id: Icaa01de53cd585d527d9a23b1629375d6b7f67e9
Reviewed-on: https://go-review.googlesource.com/c/go/+/167802
Run-TryBot: Elias Naur <mail@eliasnaur.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
