Found by running
$ go run golang.org/x/tools/go/analysis/passes/nilness/cmd/nilness@latest std
No actual bugs--other than one panic(nil)--but a
few places where error nilness was unclear.
Change-Id: Ia916ba30f46f29c1bcf928cc62280169b922463a
Reviewed-on: https://go-review.googlesource.com/c/go/+/486675
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Like https://go.dev/cl/481376 did for encoding/gob,
but now for encoding/xml, which had very similar code.
One minor difference is that encoding/xml now needs a SetLen before the
call to Index, as otherwise we index just past the length.
Still, calling Grow and SetLen is easier to understand,
and avoids needing to make a new zero value.
goos: linux
goarch: amd64
pkg: encoding/xml
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics
│ old │ new │
│ sec/op │ sec/op vs base │
Unmarshal-8 6.904µ ± 1% 6.980µ ± 1% +1.10% (p=0.009 n=6)
│ old │ new │
│ B/op │ B/op vs base │
Unmarshal-8 7.656Ki ± 0% 7.586Ki ± 0% -0.92% (p=0.002 n=6)
│ old │ new │
│ allocs/op │ allocs/op vs base │
Unmarshal-8 188.0 ± 0% 185.0 ± 0% -1.60% (p=0.002 n=6)
Change-Id: Id83feb467a9c59c80c7936aa892780aae7e8b809
Reviewed-on: https://go-review.googlesource.com/c/go/+/483135
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Test added.
Fixes#22860
Change-Id: I08304834a2b7b10b4ac729bf36761692eb4731da
Reviewed-on: https://go-review.googlesource.com/c/go/+/113075
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Fixes the misuse of "a" vs "an", according to English grammatical
expectations and using https://www.a-or-an.com/
Change-Id: I53ac724070e3ff3d33c304483fe72c023c7cda47
Reviewed-on: https://go-review.googlesource.com/c/go/+/480536
Run-TryBot: shuang cui <imcusg@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Growing by one is a simpler, and often cheaper,
operation compared to appending one (newly created) zero value.
The method was introduced in Go 1.20.
growSlice in dec_helpers.go is left alone,
as it grows using the builtin append instead of reflect.Append.
No noticeable performance difference on any of our benchmarks,
as this code only runs for slices large enough to not fit in
saferio.SliceCap, and none of our benchmarks use data that large.
goos: linux
goarch: amd64
pkg: encoding/gob
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics
│ old │ new │
│ sec/op │ sec/op vs base │
DecodeBytesSlice-8 11.37µ ± 1% 11.46µ ± 4% ~ (p=0.315 n=10)
DecodeInterfaceSlice-8 96.49µ ± 1% 95.75µ ± 1% ~ (p=0.436 n=10)
geomean 33.12µ 33.12µ +0.01%
│ old │ new │
│ B/op │ B/op vs base │
DecodeBytesSlice-8 22.39Ki ± 0% 22.39Ki ± 0% ~ (p=1.000 n=10)
DecodeInterfaceSlice-8 80.25Ki ± 0% 80.25Ki ± 0% ~ (p=0.650 n=10)
geomean 42.39Ki 42.39Ki +0.00%
│ old │ new │
│ allocs/op │ allocs/op vs base │
DecodeBytesSlice-8 169.0 ± 0% 169.0 ± 0% ~ (p=1.000 n=10) ¹
DecodeInterfaceSlice-8 3.178k ± 0% 3.178k ± 0% ~ (p=1.000 n=10) ¹
geomean 732.9 732.9 +0.00%
Change-Id: I468aebf4ae6f197a1fd35f6fee809ca591c1788f
Reviewed-on: https://go-review.googlesource.com/c/go/+/481376
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
I almost exclusively use these benchmarks with -benchtime already.
Change-Id: I6539cbba6abbdb6b275502e122f4e16856d8b9e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/481375
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Rob Pike <r@golang.org>
fieldType is a struct with only a string and an integer,
so its size will barely be three times that of a pointer.
The indirection doesn't save us any memory or append/grow cost,
but it does cause a significant amount of allocations at init time.
goos: linux
goarch: amd64
pkg: encoding/gob
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics
│ old │ new │
│ sec/op │ sec/op vs base │
EndToEndPipe-16 730.9n ± 5% 741.6n ± 5% ~ (p=0.529 n=10)
EncodingGob 173.7µ ± 0% 171.1µ ± 0% -1.46% (p=0.000 n=10)
geomean 11.27µ 11.26µ -0.01%
│ old │ new │
│ B/op │ B/op vs base │
EndToEndPipe-16 1.766Ki ± 0% 1.766Ki ± 0% ~ (p=1.000 n=10) ¹
EncodingGob 38.27Ki ± 0% 34.30Ki ± 0% -10.38% (p=0.000 n=10)
geomean 8.221Ki 7.782Ki -5.33%
¹ all samples are equal
│ old │ new │
│ allocs/op │ allocs/op vs base │
EndToEndPipe-16 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹
EncodingGob 642.0 ± 0% 615.0 ± 0% -4.21% (p=0.000 n=10)
geomean 35.83 35.07 -2.13%
¹ all samples are equal
Change-Id: I852a799834d2e9b7b915da74e871a4052d13892e
Reviewed-on: https://go-review.googlesource.com/c/go/+/479400
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
After the previous commit, both calls use the non-pointer type,
so we can deduplicate. No noticeable difference in init cost.
Change-Id: I0f0fb91d42655787cb58b4442ad3da4194560af4
Reviewed-on: https://go-review.googlesource.com/c/go/+/479399
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
wireType itself is just a struct with seven pointer fields,
so an indirection doesn't feel necessary to noticeably reduce the amount
of memory that typeInfo takes for each Go type registered in gob.
The indirection does add a small amount of overhead though,
particularly one extra allocation when registering a type,
which is done a number of times as part of init.
For consistency, also update wireTypeUserInfo to not use a pointer.
Measuring via one of the end-to-end benchmarks and benchinit:
goos: linux
goarch: amd64
pkg: encoding/gob
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics
│ old │ new │
│ sec/op │ sec/op vs base │
EndToEndPipe-16 736.8n ± 5% 733.9n ± 5% ~ (p=0.971 n=10)
EncodingGob 177.6µ ± 0% 173.6µ ± 0% -2.27% (p=0.000 n=10)
geomean 11.44µ 11.29µ -1.34%
│ old │ new │
│ B/op │ B/op vs base │
EndToEndPipe-16 1.766Ki ± 0% 1.766Ki ± 0% ~ (p=1.000 n=10) ¹
EncodingGob 38.47Ki ± 0% 38.27Ki ± 0% -0.50% (p=0.000 n=10)
geomean 8.241Ki 8.220Ki -0.25%
¹ all samples are equal
│ old │ new │
│ allocs/op │ allocs/op vs base │
EndToEndPipe-16 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹
EncodingGob 652.0 ± 0% 642.0 ± 0% -1.53% (p=0.000 n=10)
geomean 36.11 35.83 -0.77%
¹ all samples are equal
Change-Id: I528080b7d990ed595683f155a1ae25dcd26394b2
Reviewed-on: https://go-review.googlesource.com/c/go/+/479398
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
The reflect method was added in Go 1.13, in 2019.
gob's own version dates all the way back to 2011.
The behavior appears to be the same, and all tests still pass.
gob does have special cases like always encoding arrays even when they
are the zero value, but that is done via the sendZero boolean field.
Change-Id: I9057b7436963e231fdbf2f6c4b1edb58a2b13305
Reviewed-on: https://go-review.googlesource.com/c/go/+/479397
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Doing this work at init time does make the first encode or decode call
slightly faster, but the cost is still paid upfront.
However, not all programs which directly or indirectly import
encoding/gob end up encoding or decoding any values.
For example, a program might only be run with the -help flag,
or it might only use gob encoding when a specific mode is enabled.
Moreover, any work done at init time needs to happen sequentially and
before the main function can start, blocking the entire program.
Using benchinit, we see a moderate saving at init time:
goos: linux
goarch: amd64
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics
│ old │ new │
│ sec/op │ sec/op vs base │
EncodingGob 188.9µ ± 0% 175.4µ ± 0% -7.15% (p=0.000 n=10)
│ old │ new │
│ B/op │ B/op vs base │
EncodingGob 39.78Ki ± 0% 38.46Ki ± 0% -3.32% (p=0.000 n=10)
│ old │ new │
│ allocs/op │ allocs/op vs base │
EncodingGob 668.0 ± 0% 652.0 ± 0% -2.40% (p=0.000 n=10)
Change-Id: I75a5df18c9b1d02566e5885a966360d8a525913a
Reviewed-on: https://go-review.googlesource.com/c/go/+/479396
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
A GeneralizedTime value may contain an optional fractional seconds
element (according to X.680 46.2, restricted by X.690 11.7.3). This
change adds support for this fractional part, up to nine digits, so that
Unmarshal won't fail when decoding a DER encoded GeneralizedTime value
with fractional digits. Also, test cases related to this change have
been added.
X.680 and X.690 can be found at:
https://www.itu.int/rec/T-REC-X.680https://www.itu.int/rec/T-REC-X.690Fixes#15842
Change-Id: If217c007e01b686db508a940e9e2ed3bfb901879
Reviewed-on: https://go-review.googlesource.com/c/go/+/108355
Run-TryBot: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
I noticed the one in path/filepath while reading the docs,
and the other ones were found via some quick grepping.
Change-Id: I386f2f74ef816a6d18aa2f58ee6b64dbd0147c9e
Reviewed-on: https://go-review.googlesource.com/c/go/+/478795
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
I edited dec_helpers.go without realizing that it is a generated file.
Fix the generator to generate the current version (which generates
a small comment change).
Change-Id: I70e3bc78eb0728d23c08972611218f288dc1d29c
Reviewed-on: https://go-review.googlesource.com/c/go/+/479117
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Andrew Ekstedt <andrew.ekstedt@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Fixes#59172
Change-Id: I54d5e724f10117a40ec5dd58c810f6bbb2475933
GitHub-Last-Rev: d1a986698c
GitHub-Pull-Request: golang/go#59173
Reviewed-on: https://go-review.googlesource.com/c/go/+/478215
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Use the explicit API for acquiring an empty available buffer,
rather than the hack that's implemented in terms of Bytes and Len.
Change-Id: If286ed42693acd61ffe28dc849ed4b76c3ae4434
Reviewed-on: https://go-review.googlesource.com/c/go/+/476337
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
The folded name logic (despite all attempts to optimize it)
was fundamentally an O(n) operation where every field in a struct
needed to be linearly scanned in order to find a match.
This made unmashaling of unknown fields always O(n).
Instead of optimizing the comparison for each field,
make it such that we can look up a name in O(1).
We accomplish this by maintaining a map keyed by pre-folded names,
which we can pre-calculate when processing the struct type.
Using a stack-allocated buffer, we can fold the input name and
look up its presence in the map.
Also, instead of mapping from names to indexes,
map directly to a pointer to the field information.
The memory cost of this is the same and avoids an extra slice index.
The new logic is both simpler and faster.
Performance:
name old time/op new time/op delta
CodeDecoder 2.47ms ± 4% 2.42ms ± 2% -1.83% (p=0.022 n=10+9)
UnicodeDecoder 259ns ± 2% 248ns ± 1% -4.32% (p=0.000 n=10+10)
DecoderStream 150ns ± 1% 149ns ± 1% ~ (p=0.516 n=10+10)
CodeUnmarshal 3.13ms ± 2% 3.09ms ± 2% -1.37% (p=0.022 n=10+9)
CodeUnmarshalReuse 2.50ms ± 1% 2.45ms ± 1% -1.96% (p=0.001 n=8+9)
UnmarshalString 67.1ns ± 5% 64.5ns ± 5% -3.90% (p=0.005 n=10+10)
UnmarshalFloat64 60.1ns ± 4% 58.4ns ± 2% -2.89% (p=0.002 n=10+8)
UnmarshalInt64 51.0ns ± 4% 49.2ns ± 1% -3.53% (p=0.001 n=10+8)
Issue10335 80.7ns ± 2% 79.2ns ± 1% -1.82% (p=0.016 n=10+8)
Issue34127 28.6ns ± 3% 28.8ns ± 3% ~ (p=0.388 n=9+10)
Unmapped 177ns ± 2% 177ns ± 2% ~ (p=0.956 n=10+10)
Change-Id: I478b2b958f5a63a69c9a991a39cd5ffb43244a2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/471196
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Use append for HTMLEscape similar to Indent and Compact.
Move it to indent.go alongside Compact, as it shares similar logic.
In a future CL, we will modify appendCompact to be written in terms
of appendHTMLEscape, but we need to first move the JSON decoder logic
out of the main loop of appendCompact.
Change-Id: I131c64cd53d5d2b4ca798b37349aeefe17b418c7
Reviewed-on: https://go-review.googlesource.com/c/go/+/471198
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
v.SetZero() is faster than v.Set(reflect.Zero(v.Type()))
and was recently added in Go 1.20.
Benchmark numbers are largely unchanged since this mainly
affects the unmarshaling of large numbers of JSON nulls,
which our benchmarks do not heavily exercise.
Change-Id: I464f60f63c9027e63a99fd5da92e7ab782018329
Reviewed-on: https://go-review.googlesource.com/c/go/+/471195
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
With native support for fuzzing in the Go toolchain,
rely instead on the fuzz tests declared in fuzz_test.go.
Change-Id: I601842cd0bc7e64ea3bfdafbbbc3534df11acf59
Reviewed-on: https://go-review.googlesource.com/c/go/+/471197
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
The Grow method is generally a more efficient way to grow a slice.
The older approach of using reflect.MakeSlice has to
waste effort zeroing the elements overwritten by the older slice
and has to allocate the slice header on the heap.
Performance:
name old time/op new time/op delta
CodeDecoder 2.41ms ± 2% 2.42ms ± 2% ~
CodeUnmarshal 3.12ms ± 3% 3.13ms ± 3% ~
CodeUnmarshalReuse 2.49ms ± 3% 2.52ms ± 3% ~
name old alloc/op new alloc/op delta
CodeDecoder 2.00MB ± 1% 1.99MB ± 1% ~
CodeUnmarshal 3.05MB ± 0% 2.92MB ± 0% -4.23%
CodeUnmarshalReuse 1.68MB ± 0% 1.68MB ± 0% -0.32%
name old allocs/op new allocs/op delta
CodeDecoder 77.1k ± 0% 77.0k ± 0% -0.09%
CodeUnmarshal 92.7k ± 0% 91.3k ± 0% -1.47%
CodeUnmarshalReuse 77.1k ± 0% 77.0k ± 0% -0.07%
The Code benchmarks (which are the only ones that uses slices)
are largely unaffected. There is a slight reduction in allocations.
A histogram of slice lengths from the Code testdata is as follows:
≤1: 392
≤2: 256
≤4: 252
≤8: 152
≤16: 126
≤32: 78
≤64: 62
≤128: 46
≤256: 18
≤512: 10
≤1024: 8
A bulk majority of slice lengths are 8 elements or under.
Use of reflect.Value.Grow performs better for larger slices since
it can avoid the zeroing of memory and has a faster growth rate.
However, Grow grows starting from 1 element,
with a 2x growth rate until some threshold (currently 512),
Starting from 1 ensures better utilization of the heap,
but at the cost of more frequent regrowth early on.
In comparison, the previous logic always started
with a minimum of 4 elements, which leads to a wasted capacity
of 75% for the highly frequent case of a single element slice.
The older code always had a growth rate of 1.5x,
and so wastes less memory for number of elements below 512.
All in all, there are too many factors that hurt or help performance.
Rergardless, the simplicity of favoring reflect.Value.Grow
over manually managing growth rates is a welcome simplification.
Change-Id: I62868a7f112ece3c2da3b4f6bdf74d397110243c
Reviewed-on: https://go-review.googlesource.com/c/go/+/471175
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
As part of the effort to rely less on bytes.Buffer,
switch most operations to use more natural append-like operations.
This makes it easier to swap bytes.Buffer out with a buffer type
that only needs to support a minimal subset of operations.
As a simplification, we can remove the use of the scratch buffer
and use the available capacity of the buffer itself as the scratch.
Also, declare an inlineable mayAppendQuote function to conditionally
append a double-quote if necessary.
Performance:
name old time/op new time/op delta
CodeEncoder 405µs ± 2% 397µs ± 2% -1.94% (p=0.000 n=20+20)
CodeEncoderError 453µs ± 1% 444µs ± 4% -1.83% (p=0.000 n=19+19)
CodeMarshal 559µs ± 4% 548µs ± 2% -2.02% (p=0.001 n=19+17)
CodeMarshalError 724µs ± 3% 716µs ± 2% -1.13% (p=0.030 n=19+20)
EncodeMarshaler 24.9ns ±15% 22.9ns ± 5% ~ (p=0.086 n=20+17)
EncoderEncode 14.0ns ±27% 15.0ns ±20% ~ (p=0.365 n=20+20)
There is a slight performance gain across the board due to
the elimination of the scratch buffer. Appends are done directly
into the unused capacity of the underlying buffer,
avoiding an additional copy. See #53685
Updates #27735
Change-Id: Icf6d612a7f7a51ecd10097af092762dd1225d49e
Reviewed-on: https://go-review.googlesource.com/c/go/+/469558
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
This is part of the effort to reduce direct reliance on bytes.Buffer
so that we can use a buffer with better pooling characteristics.
Unify these two methods as a single version that uses generics
to reduce duplicated logic. Unfortunately, we lack a generic
version of utf8.DecodeRune (see #56948), so we cast []byte to string.
The []byte variant is slightly slower for multi-byte unicode since
casting results in a stack-allocated copy operation.
Fortunately, this code path is used only for TextMarshalers.
We can also delete TestStringBytes, which exists to ensure
that the two duplicate implementations remain in sync.
Performance:
name old time/op new time/op delta
CodeEncoder 399µs ± 2% 409µs ± 2% +2.59% (p=0.000 n=9+9)
CodeEncoderError 450µs ± 1% 451µs ± 2% ~ (p=0.684 n=10+10)
CodeMarshal 553µs ± 2% 562µs ± 3% ~ (p=0.075 n=10+10)
CodeMarshalError 733µs ± 3% 737µs ± 2% ~ (p=0.400 n=9+10)
EncodeMarshaler 24.9ns ±12% 24.1ns ±13% ~ (p=0.190 n=10+10)
EncoderEncode 12.3ns ± 3% 14.7ns ±20% ~ (p=0.315 n=8+10)
name old speed new speed delta
CodeEncoder 4.87GB/s ± 2% 4.74GB/s ± 2% -2.53% (p=0.000 n=9+9)
CodeEncoderError 4.31GB/s ± 1% 4.30GB/s ± 2% ~ (p=0.684 n=10+10)
CodeMarshal 3.51GB/s ± 2% 3.46GB/s ± 3% ~ (p=0.075 n=10+10)
CodeMarshalError 2.65GB/s ± 3% 2.63GB/s ± 2% ~ (p=0.400 n=9+10)
name old alloc/op new alloc/op delta
CodeEncoder 327B ±347% 447B ±232% +36.93% (p=0.034 n=9+10)
CodeEncoderError 142B ± 1% 143B ± 0% ~ (p=1.000 n=8+7)
CodeMarshal 1.96MB ± 2% 1.96MB ± 2% ~ (p=0.468 n=10+10)
CodeMarshalError 2.04MB ± 3% 2.03MB ± 1% ~ (p=0.971 n=10+10)
EncodeMarshaler 4.00B ± 0% 4.00B ± 0% ~ (all equal)
EncoderEncode 0.00B 0.00B ~ (all equal)
name old allocs/op new allocs/op delta
CodeEncoder 0.00 0.00 ~ (all equal)
CodeEncoderError 4.00 ± 0% 4.00 ± 0% ~ (all equal)
CodeMarshal 1.00 ± 0% 1.00 ± 0% ~ (all equal)
CodeMarshalError 6.00 ± 0% 6.00 ± 0% ~ (all equal)
EncodeMarshaler 1.00 ± 0% 1.00 ± 0% ~ (all equal)
EncoderEncode 0.00 0.00 ~ (all equal)
There is a very slight performance degradation for CodeEncoder
due to an increase in allocation sizes. However, the number of allocations
did not change. This is likely due to remote effects of the growth rate
differences between bytes.Buffer and the builtin append function.
We shouldn't overly rely on the growth rate of bytes.Buffer anyways
since that is subject to possibly change in #51462.
As the benchtime increases, the alloc/op goes down indicating
that the amortized memory cost is fixed.
Updates #27735
Change-Id: Ie35e480e292fe082d7986e0a4d81212c1d4202b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/469556
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
This is part of the effort to reduce direct reliance on bytes.Buffer
so that we can use a buffer with better pooling characteristics.
Avoid direct use of bytes.Buffer in Compact and Indent and
instead modify the logic to rely only on append.
This avoids reliance on the bytes.Buffer.Truncate method,
which makes switching to a custom buffer implementation easier.
Performance:
name old time/op new time/op delta
EncodeMarshaler 25.5ns ± 8% 25.7ns ± 9% ~ (p=0.724 n=10+10)
name old alloc/op new alloc/op delta
EncodeMarshaler 4.00B ± 0% 4.00B ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
EncodeMarshaler 1.00 ± 0% 1.00 ± 0% ~ (all equal)
Updates #27735
Change-Id: I8cded03fab7651d43b5a238ee721f3472530868e
Reviewed-on: https://go-review.googlesource.com/c/go/+/469555
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Bryan Mills <bcmills@google.com>
This reverts CL 461958 and CL 465855.
Reason for revert: This introduced an irreconcilable inconsistency with Encode
Fixes#58391.
Change-Id: Ifd01a04d433b24c092b73e627b8149a5851c2bca
Reviewed-on: https://go-review.googlesource.com/c/go/+/469615
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Ian Lance Taylor <iant@google.com>
By convention, use `err` as prefix for variables of type `error`.
Change-Id: I9401d5d47e994a27be245b2c8b1edd55cdd52db1
Reviewed-on: https://go-review.googlesource.com/c/go/+/467536
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
CL 461958 fixed a potential panic,
but also introduced an observable regression where
invalid input could be detected before the panic occurs.
Adjust the check to preserve prior behavior,
while also preventing the panic.
Change-Id: I52819f88a6a64883fbc9fd512697c38c29ca0ccd
Reviewed-on: https://go-review.googlesource.com/c/go/+/465855
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Ian Lance Taylor <iant@google.com>
The namespace defined by xmlns="value" can be overridden in every included tag
by the empty namespace xmlns="" without a prefix.
Method to calculate indent of XML handles depth of tag and its associated namespace is
still active even when no indent is required.
An XMLName field in a struct means that namespace must be enforced even if empty.
This occurs only on an inner tag as an override of any non-empty namespace of its outer tag.
An attribute is added to have the required namespace display.
Fixes#7113
Change-Id: I57f2308e98c66f04108ab136d350bdc3a6091e98
Reviewed-on: https://go-review.googlesource.com/c/go/+/108796
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Updates #57237
Change-Id: Ib626610130cae9c1d1aff5dd2a5035ffde0e127f
Reviewed-on: https://go-review.googlesource.com/c/go/+/463985
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: xie cui <523516579@qq.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Updates #57237
Change-Id: I149c8b7eeac91b87b5810250f96d48ca87135807
Reviewed-on: https://go-review.googlesource.com/c/go/+/463218
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Run-TryBot: xie cui <523516579@qq.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
hex.Decode never checks the length of dst and triggers a panic
if there are insufficient bytes in the slice.
There isn't document on what the behavior *should* be in this case.
Two possibilities:
1. Error dst has insufficient space (as done in this change)
2. Reduce the length of the decode to min(dst, src)
Option 1 was chosen because it seems the least surprising or
subtle.
Change-Id: I3bf029e3d928202de716830434285e3c165f26dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/461958
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Benjamin Prosnitz <bprosnitz@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
https://go.dev/cl/460543 stopped using the "expect" parameter in
bootstrapType, but we forgot to actually remove it.
While here, staticcheck correctly points out that we can use the copy
builtin to fill builtinIdToTypeSlice, now that it and idToType are an
array and slice respectively.
Change-Id: I48078415ab9bdd5633cf41f33ab4dc78eb30b48a
Reviewed-on: https://go-review.googlesource.com/c/go/+/462301
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Rob Pike <r@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Avoid unnecessary allocations when calling reflect.TypeOf;
we can use nil pointers, which fit into an interface without allocating.
This saves about 1% of CPU time.
The builtin types are limited to typeIds between 0 and firstUserId,
and since firstUserId is 64, builtinIdToType does not need to be a map.
We can simply use an array of length firstUserId, which is simpler.
This saves about 1% of CPU time.
idToType is similar to firstUserId in that it is a map keyed by typeIds.
The difference is that it can grow with the user's types.
However, each added type gets the next available typeId,
meaning that we can use a growing slice, similar to the case above.
nextId then becomes the current length of the slice.
This saves about 1% of CPU time.
typeInfoMap is stored globally as an atomic.Value,
where each modification loads the map, makes a whole copy,
adds the new element, and stores the modified copy.
This is perfectly fine when the user registers types,
as that can happen concurrently and at any point in the future.
However, during init time, we sequentially register many types,
and the overhead of copying maps adds up noticeably.
During init time, use a regular global map instead,
which gets replaced by the atomic.Value when our init work is done.
This saves about 2% of CPU time.
Finally, avoid calling checkId in bootstrapType;
we have just called setTypeId, whose logic for getting nextId is simple,
so the extra check doesn't gain us much.
This saves about 1% of CPU time.
Using benchinit, which transforms GODEBUG=inittrace=1 data into Go
benchmark compatible output, results in a nice improvement:
name old time/op new time/op delta
EncodingGob 175µs ± 0% 162µs ± 0% -7.45% (p=0.016 n=5+4)
name old alloc/op new alloc/op delta
EncodingGob 39.0kB ± 0% 36.1kB ± 0% -7.35% (p=0.016 n=5+4)
name old allocs/op new allocs/op delta
EncodingGob 588 ± 0% 558 ± 0% -5.10% (p=0.000 n=5+4)
Updates #26775.
Change-Id: I28618e8b96ef440480e666ef2cd5c4a9a332ef21
Reviewed-on: https://go-review.googlesource.com/c/go/+/460543
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
End-of-line comments are not doc comments,
so Deprecated notes in them are not recognized
as deprecation notices. Rewrite the comments.
Change-Id: I275fa9aec403132fda45853e52daa22bc06fcd36
Reviewed-on: https://go-review.googlesource.com/c/go/+/453617
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Ian Lance Taylor <iant@google.com>
This reverts CL 105636.
Reason for revert: Fails with existing XML data. At this stage in the release cycle we should revert, and try again next time with some way to support existing XML.
For #8068
Change-Id: Ia84cbf3a84878ac7190f72998545dee22c36c45e
Reviewed-on: https://go-review.googlesource.com/c/go/+/453996
Auto-Submit: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Change-Id: I69065f8adf101fdb28682c55997f503013a50e29
Reviewed-on: https://go-review.googlesource.com/c/go/+/449757
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Joedian Reid <joedian@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Joedian Reid <joedian@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Add test.
Fixes#20396
Change-Id: I89e9013eb338f831e1908e390b284794df78fb6b
Reviewed-on: https://go-review.googlesource.com/c/go/+/103875
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Non-regression tests are added.
Fixes#8068
Change-Id: Icb36c910bbf4955743b7aa8382002b2d9246fadc
Reviewed-on: https://go-review.googlesource.com/c/go/+/105636
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Comparing opening and closing tag is done using the prefix when available.
Documentation states that Token returns URI in the Space part of the Name.
Translation has been moved for the End tag before the namespace is removed
from the stack.
After closing a tag using a namespace, the valid namespace must be taken
from the opening tag. Tests added.
Fixes#20685
Change-Id: I4d90b19f7e21a76663f0ea1c1db6c6bf9fd2a389
Reviewed-on: https://go-review.googlesource.com/c/go/+/107255
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Test added.
Fixes#8535
Change-Id: Ic89c2781e81d963a653180812748b3fc95fb7fae
Reviewed-on: https://go-review.googlesource.com/c/go/+/106575
Run-TryBot: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Change-Id: Ic16824482142d4de4d0b949459e36505ee944ff7
Reviewed-on: https://go-review.googlesource.com/c/go/+/448175
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Dan Kortschak <dan@kortschak.io>
Auto-Submit: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dan Kortschak <dan@kortschak.io>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Wasm can't handle the recusion for XML nested to depth 10,000.
Cut it off at 5,000 instead. This fixes TestCVE202228131 on trybots
in certain conditions.
Also disable TestCVE202230633 to fix 'go test -v encoding/xml' on gomotes.
Also rename errExeceededMaxUnmarshalDepth [misspelled and unwieldy]
to errUnmarshalDepth.
For #56498.
Change-Id: I7cc337ccfee251bfd9771497be0e5272737114f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/446639
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
The slice decode helpers weren't aware of partially allocated slices.
Also add large slice support for []byte.
Change-Id: I5044587e917508887c7721f8059d364189831693
Reviewed-on: https://go-review.googlesource.com/c/go/+/443777
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
And add a link. Resolves a comment left on http://go.dev/cl/436096
after it was submitted.
Change-Id: I2847d29134ffb4fee2b0ea37842cdf57df55ec0c
Reviewed-on: https://go-review.googlesource.com/c/go/+/442816
Reviewed-by: Julie Qiu <julieqiu@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Change-Id: I375233dc700adbc58a6d4af995d07b352bf85b11
GitHub-Last-Rev: ef12920523
GitHub-Pull-Request: golang/go#55994
Reviewed-on: https://go-review.googlesource.com/c/go/+/437715
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
asn1 allocates due to reflect.TypeOf(new(big.Int)) in init time.
We could replace it with (*big.Int)(nil).
Before:
init encoding/asn1 @1.0 ms, 0.009 ms clock, 224 bytes, 7 allocs
After:
init encoding/asn1 @0.70 ms, 0.002 ms clock, 192 bytes, 6 allocs
Fixes#55973
Change-Id: I7c3cc0f48631af73cf34ad3c731c380f46c72359
Reviewed-on: https://go-review.googlesource.com/c/go/+/435257
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: hopehook <hopehook@golangcn.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
When decoding a struct, if a positive delta is large enough to overflow
when added to fieldnum, we would panic due to the resulting negative index.
Instead, catch this problem and produce an error like we do with
negative delta integers. If fieldnum ends up being negative or smaller
than state.fieldnum, the addition overflowed.
While here, remove an unnecessary break after an error call,
since those error functions cause a panic.
Fixes#55337.
Change-Id: I7a0e4f43e5c81a703e79c1597e3bb3714cc832c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/432715
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gopher Robot <gobot@golang.org>
Alan Donovan
Daniel Martí
Constantin Konstantinidis
cui fliter
Mateusz Poliwczak
Michael Teichgraeber
Ian Lance Taylor
Johan Abildskov
Joe Tsai
Oleksandr Redko
cuiweixie
Benjamin Prosnitz
Russ Cox
Dan Kortschak
Roland Shoemaker
Tomasz Jezierski