delta/go-git.git/src/encoding/json/decode.go, branch dev.go2go

encoding/json: reduce allocated space in Unmarshal

2021-02-24T14:12:16+00:00

The decodeState type is a large part of the allocated space during Unmarshal.
The errorContext field is infrequently used, and only on error.
Extract it into a pointer and allocate it separate when necessary.

name                old time/op    new time/op    delta
UnmarshalString-8      115ns ± 5%     114ns ± 3%     ~     (p=0.170 n=15+15)
UnmarshalFloat64-8     113ns ± 2%     106ns ± 1%   -6.42%  (p=0.000 n=15+14)
UnmarshalInt64-8      93.3ns ± 1%    86.9ns ± 4%   -6.89%  (p=0.000 n=14+15)

name                old alloc/op   new alloc/op   delta
UnmarshalString-8       192B ± 0%      160B ± 0%  -16.67%  (p=0.000 n=15+15)
UnmarshalFloat64-8      180B ± 0%      148B ± 0%  -17.78%  (p=0.000 n=15+15)
UnmarshalInt64-8        176B ± 0%      144B ± 0%  -18.18%  (p=0.000 n=15+15)

name                old allocs/op  new allocs/op  delta
UnmarshalString-8       2.00 ± 0%      2.00 ± 0%     ~     (all equal)
UnmarshalFloat64-8      2.00 ± 0%      2.00 ± 0%     ~     (all equal)
UnmarshalInt64-8        1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Change-Id: I53f3f468e6c65f77a12e5138a2626455b197012d
Reviewed-on: https://go-review.googlesource.com/c/go/+/271338
Trust: Josh Bleecher Snyder 
Trust: Daniel Martí 
Run-TryBot: Josh Bleecher Snyder 
TryBot-Result: Go Bot 
Reviewed-by: Daniel Martí

Revert "encoding/json: implement Is on all errors"

2020-09-14T21:33:20+00:00

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 
Reviewed-by: Emmanuel Odeke

encoding/json: implement Is on all errors

2020-09-13T03:19:24+00:00

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: 930cda5384c987a0b31f277ba3b4ab690ea74ac3
GitHub-Pull-Request: golang/go#41360
Reviewed-on: https://go-review.googlesource.com/c/go/+/254537
Run-TryBot: Emmanuel Odeke 
TryBot-Result: Gobot Gobot 
Reviewed-by: Emmanuel Odeke 
Trust: Emmanuel Odeke

Revert "encoding/json: don't reuse slice elements when decoding"

2020-07-02T22:08:11+00:00

This reverts https://golang.org/cl/191783.

Reason for revert: Broke too many programs which depended on the previous
behavior, even when it was the opposite of what the documentation said.

We can attempt to fix the original issue again for 1.16, while keeping
those programs in mind.

Fixes #39427.

Change-Id: I7a7f24b2a594c597ef625aeff04fff29aaa88fc6
Reviewed-on: https://go-review.googlesource.com/c/go/+/240657
Run-TryBot: Dmitri Shuralyov 
TryBot-Result: Gobot Gobot 
Reviewed-by: Emmanuel Odeke 
Reviewed-by: Ian Lance Taylor

encoding/json: revert "avoid work when unquoting strings, take 2"

2020-06-15T16:10:18+00:00

This reverts golang.org/cl/190659 and golang.org/cl/226218, minus the
regression tests in the latter.

The original work happened in golang.org/cl/151157, which was reverted
in golang.org/cl/190909 due to a crash found by fuzzing.

We tried a second time in golang.org/cl/190659, which shipped with Go
1.14. A bug was found, where strings would be mangled in certain edge
cases. The fix for that was golang.org/cl/226218, which was backported
into Go 1.14.4.

Unfortunately, a second regression was just reported in #39555, which is
a similar case of strings getting mangled when decoding under certain
conditions. It would be possible to come up with another small patch to
fix that edge case, but instead, let's just revert the entire
optimization, as it has proved to do more harm than good. Moreover, it's
hard to argue or prove that there will be no more such regressions.

However, all the work wasn't for nothing. First, we learned that the way
the decoder unquotes tokenized strings isn't simple; initially, we had
wrongly assumed that each string was unquoted exactly once and in order.
Second, we have gained a number of regression tests which will be useful
to prevent the same mistakes in the future, including the test cases we
add in this CL.

Fixes #39555.

Change-Id: I66a6919c2dd6d9789232482ba6cf3814eaa70f61
Reviewed-on: https://go-review.googlesource.com/c/go/+/237838
Run-TryBot: Daniel Martí 
TryBot-Result: Gobot Gobot 
Reviewed-by: Andrew Bonventre

Revert "encoding/json: reuse values when decoding map elements"

2020-05-28T22:17:10+00:00

This reverts golang.org/cl/179337.

Reason for revert: broke a few too many reasonably valid Go programs.
The previous behavior was perhaps less consistent, but the docs were
never very clear about when the decoder merges with existing values,
versus replacing existing values altogether.

Fixes #39149.

Change-Id: I1c1d857709b8398969fe421aa962f6b62f91763a
Reviewed-on: https://go-review.googlesource.com/c/go/+/234559
Run-TryBot: Daniel Martí 
TryBot-Result: Gobot Gobot 
Reviewed-by: Andrew Bonventre

encoding/json: reuse values when decoding map elements

2020-05-08T21:19:55+00:00

When we decode into a struct, each input key-value may be decoded into
one of the struct's fields. Particularly, existing data isn't dropped,
so that some sub-fields can be decoded into without zeroing all other
data.

However, decoding into a map behaved in the opposite way. Whenever a
key-value was decoded, it completely replaced the previous map element.
If the map contained any non-zero data in that key, it's dropped.

Instead, try to reuse the existing element value if possible. If the map
element type is a pointer, and the value is non-nil, we can decode
directly into it. If it's not a pointer, make a copy and decode into
that copy, as map element values aren't addressable.

This means we have to parse and convert the map element key before the
value, to be able to obtain the existing element value. This is fine,
though. Moreover, reporting errors on the key before the value follows
the input order more closely.

Finally, add a test to explore the four combinations, involving pointer
and non-pointer, and non-zero and zero values. A table-driven test
wasn't used, as each case required different checks, such as checking
that the non-nil pointer case doesn't end up with a different pointer.

Fixes #31924.

Change-Id: I5ca40c9963a98aaf92f26f0b35843c021028dfca
Reviewed-on: https://go-review.googlesource.com/c/go/+/179337
Run-TryBot: Daniel Martí 
TryBot-Result: Gobot Gobot 
Reviewed-by: Ian Lance Taylor

encoding/json: don't mangle strings in an edge case when decoding

2020-05-08T17:12:27+00:00

The added comment contains some context. The original optimization
assumed that each call to unquoteBytes (or unquote) followed its
corresponding call to rescanLiteral. Otherwise, unquoting a literal
might use d.safeUnquote from another re-scanned literal.

Unfortunately, this assumption is wrong. When decoding {"foo": "bar"}
into a map[T]string where T implements TextUnmarshaler, the sequence of
calls would be as follows:

1) rescanLiteral "foo"
2) unquoteBytes "foo"
3) rescanLiteral "bar"
4) unquoteBytes "foo" (for UnmarshalText)
5) unquoteBytes "bar"

Note that the call to UnmarshalText happens in literalStore, which
repeats the work to unquote the input string literal. But, since that
happens after we've re-scanned "bar", we're using the wrong safeUnquote
field value.

In the added test case, the second string had a non-zero number of safe
bytes, and the first string had none since it was all non-ASCII. Thus,
"safely" unquoting a number of the first string's bytes could cut a rune
in half, and thus mangle the runes.

A rather simple fix, without a full revert, is to only allow one use of
safeUnquote per call to unquoteBytes. Each call to rescanLiteral when
we have a string is soon followed by a call to unquoteBytes, so it's no
longer possible for us to use the wrong index.

Also add a test case from #38126, which is the same underlying bug, but
affecting the ",string" option.

Before the fix, the test would fail, just like in the original two issues:

--- FAIL: TestUnmarshalRescanLiteralMangledUnquote (0.00s)
decode_test.go:2443: Key "开源" does not exist in map: map[开���:12345开源]
decode_test.go:2458: Unmarshal unexpected error: json: invalid use of ,string struct tag, trying to unmarshal "\"aaa\tbbb\"" into string

Fixes #38105.
For #38126.

Change-Id: I761e54924e9a971a4f9eaa70bbf72014bb1476e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/226218
Run-TryBot: Daniel Martí
TryBot-Result: Gobot Gobot
Reviewed-by: Joe Tsai

encoding/json: don't reuse slice elements when decoding

2020-05-07T05:01:03+00:00

The previous behavior directly contradicted the docs that have been in
place for years:

	To unmarshal a JSON array into a slice, Unmarshal resets the
	slice length to zero and then appends each element to the slice.

We could use reflect.New to create a new element and reflect.Append to
then append it to the destination slice, but benchmarks have shown that
reflect.Append is very slow compared to the code that manually grows a
slice in this file.

Instead, if we're decoding into an element that came from the original
backing array, zero it before decoding into it. We're going to be using
the CodeDecoder benchmark, as it has a slice of struct pointers that's
decoded very often.

Note that we still reuse existing values from arrays being decoded into,
as the documentation agrees with the existing implementation in that
case:

	To unmarshal a JSON array into a Go array, Unmarshal decodes
	JSON array elements into corresponding Go array elements.

The numbers with the benchmark as-is might seem catastrophic, but that's
only because the benchmark is decoding into the same variable over and
over again. Since the old decoder was happy to reuse slice elements, it
would save a lot of allocations by not having to zero and re-allocate
said elements:

	name           old time/op    new time/op    delta
	CodeDecoder-8    10.4ms ± 1%    10.9ms ± 1%   +4.41%  (p=0.000 n=10+10)

	name           old speed      new speed      delta
	CodeDecoder-8   186MB/s ± 1%   178MB/s ± 1%   -4.23%  (p=0.000 n=10+10)

	name           old alloc/op   new alloc/op   delta
	CodeDecoder-8    2.19MB ± 0%    3.59MB ± 0%  +64.09%  (p=0.000 n=10+10)

	name           old allocs/op  new allocs/op  delta
	CodeDecoder-8     76.8k ± 0%     92.7k ± 0%  +20.71%  (p=0.000 n=10+10)

We can prove this by moving 'var r codeResponse' into the loop, so that
the benchmark no longer reuses the destination pointer. And sure enough,
we no longer see the slow-down caused by the extra allocations:

	name           old time/op    new time/op    delta
	CodeDecoder-8    10.9ms ± 0%    10.9ms ± 1%  -0.37%  (p=0.043 n=10+10)

	name           old speed      new speed      delta
	CodeDecoder-8   177MB/s ± 0%   178MB/s ± 1%  +0.37%  (p=0.041 n=10+10)

	name           old alloc/op   new alloc/op   delta
	CodeDecoder-8    3.59MB ± 0%    3.59MB ± 0%    ~     (p=0.780 n=10+10)

	name           old allocs/op  new allocs/op  delta
	CodeDecoder-8     92.7k ± 0%     92.7k ± 0%    ~     (all equal)

I believe that it's useful to leave the benchmarks as they are now,
because the decoder does reuse memory in some cases. For example,
existing map elements are reused. However, subtle changes like this one
need to be benchmarked carefully.

Finally, add a couple of tests involving both a slice and an array of
structs.

Fixes #21092.

Change-Id: I8b1194f25e723a31abd146fbfe9428ac10c1389d
Reviewed-on: https://go-review.googlesource.com/c/go/+/191783
Reviewed-by: Ian Lance Taylor 
Run-TryBot: Ian Lance Taylor 
TryBot-Result: Gobot Gobot

encoding/json: avoid work when unquoting strings, take 2

2019-10-31T12:02:57+00:00

This is a re-submission of CL 151157, since it was reverted in CL 190909
due to an introduced crash found by a fuzzer. The revert CL included
regression tests, while this CL includes a fixed version of the original
change.

In particular, what we forgot in the original optimization was that we
still need the length and trailing quote checks at the beginning of
unquoteBytes. Without those, we could end up in a crash later on.

We can work out how many bytes can be unquoted trivially in
rescanLiteral, which already iterates over a string's bytes.

Removing the extra loop in unquoteBytes simplifies the function and
speeds it up, especially when decoding simple strings, which are common.

While at it, we can remove the check that s[0]=='"', since all call
sites already meet that condition.

name           old time/op    new time/op    delta
CodeDecoder-8    10.6ms ± 2%    10.5ms ± 1%  -1.01%  (p=0.004 n=20+10)

name           old speed      new speed      delta
CodeDecoder-8   183MB/s ± 2%   185MB/s ± 1%  +1.02%  (p=0.003 n=20+10)

Updates #28923.

Change-Id: I8c6b13302bcd86a364bc998d72451332c0809cde
Reviewed-on: https://go-review.googlesource.com/c/go/+/190659
Run-TryBot: Daniel Martí 
TryBot-Result: Gobot Gobot 
Reviewed-by: Peter Weinberger