Skip to content

perf: reuse caller-supplied destination map in map[string]interface{} decode#77

Open
xe-nvdk wants to merge 3 commits into
v6from
perf/map-dest-reuse
Open

perf: reuse caller-supplied destination map in map[string]interface{} decode#77
xe-nvdk wants to merge 3 commits into
v6from
perf/map-dest-reuse

Conversation

@xe-nvdk

@xe-nvdk xe-nvdk commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

Closes #61 (the safe subset).

decodeMapStringInterfacePtr allocated a fresh map via DecodeMap() even when the caller passed a non-nil destination — unlike the map[string]string path, which has always decoded into an existing map. This mirrors that behavior: entries merge into the caller's map, msgpack nil still sets it to nil, and a nil destination allocates as before (with the maxMapSize clamp).

Callers that reuse a destination (clear(m); Unmarshal(data, &m)) — e.g. a decode hot loop in Arc ingest — now get zero map allocations per decode.

Why not pooling (what the issue proposed first)

sync.Pool-ing decoded maps is unsound: the caller retains the returned map, so pooling requires an explicit return API (the issue's own notes flag this, and it's the same ownership hazard class as PR #68/issue #66). Destination reuse delivers the allocation win with no ownership hazard. The issue's alternative proposal (UnmarshalReuse-style reuse of existing maps in v) is exactly what this implements, without a new API.

Behavior change (documented in CHANGELOG)

For a non-nil map[string]interface{} destination, decode now merges instead of replaces — diverging from upstream but consistent with the fork's (and upstream's) existing map[string]string semantics. Applies to the Decode() fast path, struct fields, and named map types. Pass a nil map to keep replace semantics.

Benchmarks (Apple M3 Max, benchstat, count=6, identical benchmark on both sides)

Decoding a 4-key map into a reused destination:

v6 this PR delta
ns/op 256.9n 198.2n -22.9%
B/op 416 80 -80.8%
allocs/op 12 10 -16.7%

Nil-destination path: one extra branch, no measurable change.

Testing

  • TestMapStringInterfaceReuse: merge into pre-populated dst, msgpack nil → nil, nil dst allocates, empty map leaves pre-populated dst untouched.
  • TestMapStringInterfaceStructFieldReuse: struct-field merge semantics.
  • go test ./... and go test -race ./... pass.
  • Internal review pass (correctness+compat, quality+perf+security) completed; all findings addressed in the follow-up commit.

🤖 Generated with Claude Code

Ignacio Van Droogenbroeck added 3 commits June 12, 2026 15:27
… decode

decodeMapStringInterfacePtr allocated a fresh map via DecodeMap() even
when the caller passed a non-nil destination, unlike the
map[string]string path (decodeMapStringStringPtr) which decodes into an
existing map. Mirror that behavior: entries are merged into the
caller's map, msgpack nil still sets it to nil, and a nil destination
allocates as before.

Callers that reuse a destination (clear(m); Unmarshal(data, &m)) now
get zero map allocations per decode.

This is the safe subset of #61: pooling decoded maps is unsound because
the caller retains them (see issue notes); destination reuse achieves
the allocation win without ownership hazards.

Closes #61
… (review follow-up)

- Struct fields of type map[string]interface{} now get the same merge
  semantics map[string]string fields always had — covered by test.
- Empty msgpack map into a pre-populated destination leaves it
  untouched — covered by test.
- CHANGELOG: state the benchmark baseline explicitly and that the
  merge semantics apply to all decode paths for the type.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request optimizes the decoding of map[string]interface{} by reusing caller-supplied destination maps and merging entries in place, rather than allocating a new map on every decode. This change includes a new benchmark and unit tests to verify the map reuse and merging behavior. I have no feedback to provide on these changes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: pool map allocations in decode path

1 participant