diff --git a/CHANGELOG.md b/CHANGELOG.md index 6caf060..34883b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - **decode:** `PeekCode` bsr fast path — peeks directly at `bsr.data[bsr.pos]` instead of `ReadByte` + `UnreadByte` (two interface calls) ([#59](https://github.com/Basekick-Labs/msgpack/issues/59)) - **encode:** pool `OmitEmpty` filtered field slices via `sync.Pool` — when fields are actually omitted, the allocated `[]*field` slice is now returned to a pool for reuse instead of being GC'd ([#58](https://github.com/Basekick-Labs/msgpack/issues/58)) - **encode/decode:** pool and pre-allocate interned-string dict — `SetInternedStringsDictCap(n)` pre-sizes the dict to avoid map rehashing and slice growth; pooled encoders/decoders now reuse dict storage across `Reset()` (cleared in place) instead of discarding it, and `Put*()` drops oversized dicts to keep the pool lean ([#66](https://github.com/Basekick-Labs/msgpack/issues/66)) +- **decode:** hoist `newValue()` allocations out of `decodeTypedMapValue` loop — reuses a single key slot and value slot across all map entries, zeroing between iterations. Takes typed-map decode from 2N `reflect.New()` calls to 2 per map ([#65](https://github.com/Basekick-Labs/msgpack/issues/65)) (BenchmarkLargeMapIntInt **-50% allocs/op**, **-50% B/op**, **-10% ns/op** for 1000-entry `map[int]int`) --- diff --git a/bench_test.go b/bench_test.go index 7211aa7..77acc88 100644 --- a/bench_test.go +++ b/bench_test.go @@ -192,6 +192,15 @@ func BenchmarkMapIntInt(b *testing.B) { benchmarkEncodeDecode(b, src, &dst) } +func BenchmarkLargeMapIntInt(b *testing.B) { + src := make(map[int]int, 1000) + for i := 0; i < 1000; i++ { + src[i] = i + } + var dst map[int]int + benchmarkEncodeDecode(b, src, &dst) +} + func BenchmarkStringSlice(b *testing.B) { src := []string{"hello", "world"} var dst []string diff --git a/decode_map.go b/decode_map.go index de26b12..7ba8e10 100644 --- a/decode_map.go +++ b/decode_map.go @@ -291,18 +291,34 @@ func (d *Decoder) decodeTypedMapN(n int) (interface{}, error) { } func (d *Decoder) decodeTypedMapValue(v reflect.Value, n int) error { + if n == 0 { + return nil + } var ( typ = v.Type() keyType = typ.Key() valueType = typ.Elem() ) + // Hoist the key and value slots out of the loop so we pay two + // reflect.New allocations per map decode instead of 2N. SetMapIndex + // copies the key and value, so the stored entries are unaffected by + // subsequent mutation of mk/mv. + // + // Zeroing each iteration is required for correctness, not just + // hygiene: value decoders like decodeSliceValue reuse an existing + // slice's backing array when v.Cap() >= n, which would otherwise let + // iteration 2 clobber iteration 1's already-stored slice. + mk := d.newValue(keyType).Elem() + mv := d.newValue(valueType).Elem() + keyZero := reflect.Zero(keyType) + valueZero := reflect.Zero(valueType) for i := 0; i < n; i++ { - mk := d.newValue(keyType).Elem() + mk.Set(keyZero) if err := d.DecodeValue(mk); err != nil { return err } - mv := d.newValue(valueType).Elem() + mv.Set(valueZero) if err := d.DecodeValue(mv); err != nil { return err } diff --git a/msgpack_test.go b/msgpack_test.go index 3be8238..6de7319 100644 --- a/msgpack_test.go +++ b/msgpack_test.go @@ -553,3 +553,65 @@ func TestPtrValueDecode(t *testing.T) { require.NotNil(t, foo.Bar) require.Equal(t, *foo.Bar, bar2) } + +// TestDecodeTypedMapSliceValuesAreIndependent is the regression test for #65. +// decodeTypedMapValue hoists the key/value reflect.Value slots out of the +// per-entry loop and zeroes them each iteration. Without the zeroing step, +// decodeSliceValue would reuse the previous iteration's backing array +// (since it reuses capacity when v.Cap() >= n), corrupting entries already +// written to the map. +func TestDecodeTypedMapSliceValuesAreIndependent(t *testing.T) { + in := map[int][]int{ + 1: {10, 11, 12}, + 2: {20, 21, 22}, + 3: {30, 31, 32}, + } + + b, err := msgpack.Marshal(in) + require.Nil(t, err) + + var out map[int][]int + require.Nil(t, msgpack.Unmarshal(b, &out)) + require.Equal(t, in, out) + + // Mutating one entry must not touch any other. If the decode loop + // reused a single backing array, the entries would alias and this + // assertion would fail. + for i := range out[1] { + out[1][i] = -1 + } + require.Equal(t, []int{20, 21, 22}, out[2]) + require.Equal(t, []int{30, 31, 32}, out[3]) +} + +// TestDecodeTypedMapStructValuesAreIndependent covers the struct-value +// case of the same hoist bug. The struct carries a slice field so the +// aliasing mechanism fires through the hoisted value slot: decodeStructValue +// writes every field present in the payload, but the slice field itself is +// decoded via decodeSliceValue, which reuses the existing backing array +// when capacity is sufficient. Without per-iteration zeroing of the outer +// struct's slot, iteration 2's slice field would overwrite iteration 1's. +func TestDecodeTypedMapStructValuesAreIndependent(t *testing.T) { + type kv struct { + A int + B []string + } + in := map[int]kv{ + 1: {1, []string{"one", "uno"}}, + 2: {2, []string{"two", "dos"}}, + 3: {3, []string{"three", "tres"}}, + } + + b, err := msgpack.Marshal(in) + require.Nil(t, err) + + var out map[int]kv + require.Nil(t, msgpack.Unmarshal(b, &out)) + require.Equal(t, in, out) + + for i := range out[1].B { + out[1].B[i] = "MUTATED" + } + require.Equal(t, []string{"two", "dos"}, out[2].B) + require.Equal(t, []string{"three", "tres"}, out[3].B) +} diff --git a/types_test.go b/types_test.go index 00590e7..91faff6 100644 --- a/types_test.go +++ b/types_test.go @@ -548,6 +548,14 @@ var ( {in: mapStringInterface{"foo": "bar"}, out: new(mapStringInterface)}, {in: map[stringAlias]interfaceAlias{"foo": "bar"}, out: new(map[stringAlias]interfaceAlias)}, {in: map[int]string{1: "string"}, out: new(map[int]string)}, + // Map value types whose decoder reuses existing state (slice + // backing array, nested map, struct fields). Regression coverage + // for #65: decodeTypedMapValue hoists key/value slots out of the + // loop, so these types would clobber earlier entries if the + // per-iteration zeroing were removed. + {in: map[int][]int{1: {1, 2, 3}, 2: {4, 5, 6}, 3: {7, 8, 9}}, out: new(map[int][]int)}, + {in: map[string][]string{"a": {"x", "y"}, "b": {"z"}}, out: new(map[string][]string)}, + {in: map[string]map[string]int{"o1": {"i": 1}, "o2": {"i": 2}}, out: new(map[string]map[string]int)}, {in: (*Object)(nil), out: new(*Object)}, {in: &Object{42}, out: new(Object)},