commit 11b2853e6f322306a55519d03671e256b966e8ca
author Daniel Martí <mvdan@mvdan.cc> Thu Aug 29 14:24:16 2019 +0200
committer Brad Fitzpatrick <bradfitz@golang.org> Thu May 07 05:01:03 2020 +0000
tree c37d309af5668a65ad51e4ff2fd674f06c13930c
parent 85162292af601a1bebb8ec3d63314e39b648829f

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

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 <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>