[go] encoding/json: don't reuse slice/array elements when decoding
420 views
Skip to first unread message
Daniel Martí (Gerrit)
Aug 29, 2019, 8:37:11 AM8/29/19
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Daniel Martí has uploaded this change for review.
encoding/json: don't reuse slice/array 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.
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
---
M src/encoding/json/decode.go
M src/encoding/json/decode_test.go
2 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index df1c085..d2a35e9 100644
--- a/src/encoding/json/decode.go
+++ b/src/encoding/json/decode.go
@@ -176,8 +176,7 @@
d.scanWhile(scanSkipSpace)
// We decode rv not rv.Elem because the Unmarshaler interface
// test must be applied at the top level of the value.
- err := d.value(rv)
- if err != nil {
+ if err := d.value(rv); err != nil {
return d.addErrorContext(err)
}
return d.savedError
@@ -527,6 +526,7 @@
}
i := 0
+ initialCap := v.Cap()
for {
// Look ahead for ] - can only happen on first iteration.
d.scanWhile(scanSkipSpace)
@@ -534,7 +534,6 @@
break
}
- // Get element of array, growing if necessary.
if v.Kind() == reflect.Slice {
// Grow slice if necessary
if i >= v.Cap() {
@@ -550,19 +549,19 @@
v.SetLen(i + 1)
}
}
-
+ var into reflect.Value
if i < v.Len() {
- // Decode into element.
- if err := d.value(v.Index(i)); err != nil {
- return err
- }
- } else {
- // Ran out of fixed array: skip.
- if err := d.value(reflect.Value{}); err != nil {
- return err
+ into = v.Index(i)
+ if i < initialCap {
+ // This element is from the original backing
+ // array, so we need to zero it before decoding.
+ into.Set(reflect.Zero(v.Type().Elem()))
}
}
i++
+ if err := d.value(into); err != nil {
+ return err
+ }
// Next token must be , or ].
if d.opcode == scanSkipSpace {
@@ -578,16 +577,17 @@
if i < v.Len() {
if v.Kind() == reflect.Array {
- // Array. Zero the rest.
- z := reflect.Zero(v.Type().Elem())
+ // Zero the remaining elements.
+ zero := reflect.Zero(v.Type().Elem())
for ; i < v.Len(); i++ {
- v.Index(i).Set(z)
+ v.Index(i).Set(zero)
}
} else {
v.SetLen(i)
}
}
- if i == 0 && v.Kind() == reflect.Slice {
+ if v.Kind() == reflect.Slice && v.IsNil() {
+ // Don't allow the resulting slice to be nil.
v.Set(reflect.MakeSlice(v.Type(), 0, 0))
}
return nil
diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go
index 8dcb08c..22d898e 100644
--- a/src/encoding/json/decode_test.go
+++ b/src/encoding/json/decode_test.go
@@ -2093,7 +2093,10 @@
// slices, and arrays.
// Issues 4900 and 8837, among others.
func TestPrefilled(t *testing.T) {
- // Values here change, cannot reuse table across runs.
+ type T struct {
+ A, B int
+ }
+ // Values here change, cannot reuse the table across runs.
var prefillTests = []struct {
in string
ptr interface{}
@@ -2129,6 +2132,16 @@
ptr: &[...]int{1, 2},
out: &[...]int{3, 0},
},
+ {
+ in: `[{"A": 3}]`,
+ ptr: &[]T{{A: -1, B: -2}, {A: -3, B: -4}},
+ out: &[]T{{A: 3}},
+ },
+ {
+ in: `[{"A": 3}]`,
+ ptr: &[...]T{{A: -1, B: -2}, {A: -3, B: -4}},
+ out: &[...]T{{A: 3}, {}},
+ },
}
for _, tt := range prefillTests {
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Aug 29, 2019, 8:37:27 AM8/29/19
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
TryBots beginning. Status page: https://farmer.golang.org/try?commit=49c491d2
Gobot Gobot (Gerrit)
Aug 29, 2019, 8:41:35 AM8/29/19
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/49c491d2/freebsd-amd64-12_0_699287ae.log
Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Gobot Gobot (Gerrit)
Aug 29, 2019, 8:47:20 AM8/29/19
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
10 of 21 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/49c491d2/freebsd-amd64-12_0_699287ae.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/49c491d2/linux-386_de9ed32b.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/49c491d2/linux-amd64_96ad644d.log
Failed on nacl-amd64p32: https://storage.googleapis.com/go-build-log/49c491d2/nacl-amd64p32_f21dd1bd.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/49c491d2/openbsd-amd64-64_94d7e005.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/49c491d2/windows-amd64-2016_1f6fe870.log
Failed on js-wasm: https://storage.googleapis.com/go-build-log/49c491d2/js-wasm_5b1628de.log
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/49c491d2/android-amd64-emu_96f12e3b.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/49c491d2/windows-386-2008_576e7c62.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/49c491d2/linux-amd64-race_81ca6fef.log
Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Patch set 1:TryBot-Result -1
Daniel Martí (Gerrit)
Aug 29, 2019, 8:49:40 AM8/29/19
to goph...@pubsubhelper.golang.org, Gobot Gobot, golang-co...@googlegroups.com
Hmm, I tested json itself, but not json users within std.
Daniel Martí (Gerrit)
Aug 30, 2019, 2:58:15 PM8/30/19
to Gobot Gobot, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Daniel Martí uploaded patch set #2 to this change.
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.
2 files changed, 33 insertions(+), 19 deletions(-)
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Aug 30, 2019, 2:58:48 PM8/30/19
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gobot Gobot (Gerrit)
Aug 30, 2019, 3:12:33 PM8/30/19
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
TryBots are happy.
Patch set 2:TryBot-Result +1
Daniel Martí (Gerrit)
Aug 31, 2019, 4:38:57 AM8/31/19
to goph...@pubsubhelper.golang.org, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, Gobot Gobot, golang-co...@googlegroups.com
I think this is ready for a review. Adding people from the original thread.
Matt Layher (Gerrit)
Sep 23, 2019, 11:52:40 AM9/23/19
to Daniel Martí, goph...@pubsubhelper.golang.org, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, Gobot Gobot, golang-co...@googlegroups.com
Patch set 2:Code-Review +1
Daniel Martí (Gerrit)
Feb 23, 2020, 6:41:59 AM2/23/20
to goph...@pubsubhelper.golang.org, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, Gobot Gobot, golang-co...@googlegroups.com
Adding Russ and Joe as per the owners doc.
Just waiting for a +2; we all agree the change needs to be done, it has tests and benchmark numbers, and it already got a +1 last cycle :)
Patch set 3:Run-TryBot +1
Gobot Gobot (Gerrit)
Feb 23, 2020, 6:42:13 AM2/23/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
Gobot Gobot (Gerrit)
Feb 23, 2020, 6:52:42 AM2/23/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
TryBots are happy.
Patch set 3:TryBot-Result +1
Ian Lance Taylor (Gerrit)
Feb 23, 2020, 8:42:56 AM2/23/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Gobot Gobot, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
1 comment:
File src/encoding/json/decode.go:
Patch Set #3, Line 582:
if err := d.value(into); err != nil {We lost the comment about how if we're past the end of a fixed array we skip the value by decoding it into a zero reflect.Value. I think it would be better to keep that comment one way or another.
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Feb 26, 2020, 10:55:20 AM2/26/20
to Antonio Troina, Gobot Gobot, Matt Layher, Russ Cox, Joe Tsai, goph...@pubsubhelper.golang.org, Dmitry Vyukov, Brad Fitzpatrick, Ian Lance Taylor, golang-co...@googlegroups.com
Daniel Martí uploaded patch set #4 to this change.
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.
2 files changed, 36 insertions(+), 19 deletions(-)
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Feb 26, 2020, 10:55:31 AM2/26/20
to goph...@pubsubhelper.golang.org, Ian Lance Taylor, Gobot Gobot, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
1 comment:
Patch Set #3, Line 582:
if err := d.value(into); err != nil {
We lost the comment about how if we're past the end of a fixed array we skip the value by decoding i […]
Done
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Feb 26, 2020, 10:57:48 AM2/26/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Ian Lance Taylor, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
Gobot Gobot (Gerrit)
Feb 26, 2020, 11:36:25 AM2/26/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Ian Lance Taylor, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
TryBots are happy.
Patch set 4:TryBot-Result +1
Lucas Bremgartner (Gerrit)
Apr 2, 2020, 3:02:40 PM4/2/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Gobot Gobot, Ian Lance Taylor, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
LGTM
Patch set 4:Code-Review +1
Ian Lance Taylor (Gerrit)
May 6, 2020, 8:33:20 PM5/6/20
to Daniel Martí, goph...@pubsubhelper.golang.org, Lucas Bremgartner, Gobot Gobot, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
Patch set 5:Run-TryBot +1Code-Review +2
Gobot Gobot (Gerrit)
May 6, 2020, 8:33:36 PM5/6/20
to Daniel Martí, Ian Lance Taylor, goph...@pubsubhelper.golang.org, Lucas Bremgartner, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
Gobot Gobot (Gerrit)
May 6, 2020, 8:45:05 PM5/6/20
to Daniel Martí, Ian Lance Taylor, goph...@pubsubhelper.golang.org, Lucas Bremgartner, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Brad Fitzpatrick, Dmitry Vyukov, golang-co...@googlegroups.com
TryBots are happy.
Patch set 5:TryBot-Result +1
Brad Fitzpatrick (Gerrit)
May 7, 2020, 1:01:09 AM5/7/20
to Daniel Martí, Brad Fitzpatrick, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Gobot Gobot, Ian Lance Taylor, Lucas Bremgartner, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Dmitry Vyukov, golang-co...@googlegroups.com
Brad Fitzpatrick submitted this change.
Approvals:
Ian Lance Taylor: Looks good to me, approved; Run TryBots
Gobot Gobot: TryBots succeeded
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.
Reviewed-on: https://go-review.googlesource.com/c/go/+/191783
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
Run-TryBot: Ian Lance Taylor <ia...@golang.org>
TryBot-Result: Gobot Gobot <go...@golang.org>
---
M src/encoding/json/decode.go
M src/encoding/json/decode_test.go
2 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index b434846..6fa2ea4 100644
--- a/src/encoding/json/decode.go
+++ b/src/encoding/json/decode.go
@@ -177,8 +177,7 @@
d.scanWhile(scanSkipSpace)
// We decode rv not rv.Elem because the Unmarshaler interface
// test must be applied at the top level of the value.
- err := d.value(rv)
- if err != nil {
+ if err := d.value(rv); err != nil {
return d.addErrorContext(err)
}
return d.savedError
@@ -525,6 +524,7 @@
return nil
}
v = pv
+ initialSliceCap := 0
// Check type of target.
switch v.Kind() {
@@ -541,8 +541,9 @@
d.saveError(&UnmarshalTypeError{Value: "array", Type: v.Type(), Offset: int64(d.off)})
d.skip()
return nil
- case reflect.Array, reflect.Slice:
- break
+ case reflect.Slice:
+ initialSliceCap = v.Cap()
+ case reflect.Array:
}
i := 0
@@ -553,7 +554,6 @@
break
}
- // Get element of array, growing if necessary.
if v.Kind() == reflect.Slice {
// Grow slice if necessary
if i >= v.Cap() {
@@ -569,19 +569,22 @@
v.SetLen(i + 1)
}
}
-
+ var into reflect.Value
if i < v.Len() {
- // Decode into element.
- if err := d.value(v.Index(i)); err != nil {
- return err
- }
- } else {
- // Ran out of fixed array: skip.
- if err := d.value(reflect.Value{}); err != nil {
- return err
+ into = v.Index(i)
+ if i < initialSliceCap {
+ // Reusing an element from the slice's original
+ // backing array; zero it before decoding.
+ into.Set(reflect.Zero(v.Type().Elem()))
}
}
i++
+ // Note that we decode the value even if we ran past the end of
+ // the fixed array. In that case, we decode into an empty value
+ // and do nothing with it.
+ if err := d.value(into); err != nil {
+ return err
+ }
// Next token must be , or ].
if d.opcode == scanSkipSpace {
@@ -597,16 +600,17 @@
if i < v.Len() {
if v.Kind() == reflect.Array {
- // Array. Zero the rest.
- z := reflect.Zero(v.Type().Elem())
+ // Zero the remaining elements.
+ zero := reflect.Zero(v.Type().Elem())
for ; i < v.Len(); i++ {
- v.Index(i).Set(z)
+ v.Index(i).Set(zero)
}
} else {
v.SetLen(i)
}
}
- if i == 0 && v.Kind() == reflect.Slice {
+ if v.Kind() == reflect.Slice && v.IsNil() {
+ // Don't allow the resulting slice to be nil.
v.Set(reflect.MakeSlice(v.Type(), 0, 0))
}
return nil
diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go
index 3c5fd14..a00cc15 100644
--- a/src/encoding/json/decode_test.go
+++ b/src/encoding/json/decode_test.go
@@ -2099,7 +2099,10 @@
// slices, and arrays.
// Issues 4900 and 8837, among others.
func TestPrefilled(t *testing.T) {
- // Values here change, cannot reuse table across runs.
+ type T struct {
+ A, B int
+ }
+ // Values here change, cannot reuse the table across runs.
var prefillTests = []struct {
in string
ptr interface{}
@@ -2135,6 +2138,16 @@
ptr: &[...]int{1, 2},
out: &[...]int{3, 0},
},
+ {
+ in: `[{"A": 3}]`,
+ ptr: &[]T{{A: -1, B: -2}, {A: -3, B: -4}},
+ out: &[]T{{A: 3}},
+ },
+ {
+ in: `[{"A": 3}]`,
+ ptr: &[...]T{{A: -1, B: -2}, {A: -3, B: -4}},
+ out: &[...]T{{A: 3, B: -2}, {}},
+ },
}
for _, tt := range prefillTests {
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Jul 1, 2020, 7:31:24 AM7/1/20
to Brad Fitzpatrick, goph...@pubsubhelper.golang.org, Gobot Gobot, Ian Lance Taylor, Lucas Bremgartner, Russ Cox, Joe Tsai, Matt Layher, Antonio Troina, Dmitry Vyukov, golang-co...@googlegroups.com
Daniel Martí has created a revert of this change.
To view, visit change 191783. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages