Skip to content

decoder: intentional unconditional allocation in large-array branch for TrackOutputBytes correctness#26

Closed
Copilot wants to merge 2 commits into
reduce-allocationsfrom
copilot/sub-pr-22-yet-again
Closed

decoder: intentional unconditional allocation in large-array branch for TrackOutputBytes correctness#26
Copilot wants to merge 2 commits into
reduce-allocationsfrom
copilot/sub-pr-22-yet-again

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 11, 2026

A reviewer suggested reusing a caller-provided slice's backing array in the large-array decode path to avoid unnecessary allocations. This PR addresses that feedback by confirming the unconditional reflect.MakeSlice allocation is intentional and correct.

Why the current implementation is correct

  • Accounting integrity: In the large-array branch, TrackOutputBytes is called per-element as the slice grows. Reusing an existing slice's backing array would silently bypass tracking for the pre-allocated capacity, creating an accounting gap in cumulative output-size limit enforcement.
  • Behavioral consistency: The code prior to the MaxOutputBytes feature also allocated unconditionally via reflect.MakeSlice; this preserves that contract.
// Large arrays: cap initial allocation, track and grow per element.
v.Set(reflect.MakeSlice(v.Type(), 0, maxPrealloc)) // intentional — all capacity must go through TrackOutputBytes
for i := 0; i < sliceLen; i++ {
    if err := d.TrackOutputBytes(elemSize); err != nil {
        return n, err
    }
    v.Set(reflect.Append(v, zeroElem))
    ...
}

Reusing v's existing backing array (when v.Cap() >= maxPrealloc) would skip TrackOutputBytes for those pre-existing bytes, defeating the limit enforcement.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

tamirms and others added 2 commits March 11, 2026 12:53
… limits

Add a MaxOutputBytes field to DecodeOptions that tracks cumulative
decoded output size across a single decode operation. Before each
allocation (array element, union arm, optional field, opaque data),
the size is added to a running total; if it exceeds MaxOutputBytes
the decode is aborted with ErrOutputBytesExceeded.

Key changes:
- Add TrackOutputBytes method and TrackOutputBytesOf[T] generic helper
- Cap array pre-allocation at 256 elements, growing via append beyond that
- Track allocations in DecodeFixedOpaque, decodeArray, decodeUnion,
  decodeMap, and allocPtrIfNil
- Bump minimum Go version to 1.25 for generics support

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI changed the title [WIP] Address feedback on MaxOutputBytes decode option implementation decoder: intentional unconditional allocation in large-array branch for TrackOutputBytes correctness Mar 11, 2026
@tamirms tamirms force-pushed the reduce-allocations branch from 112f917 to c677e1b Compare March 11, 2026 18:52
@tamirms
Copy link
Copy Markdown

tamirms commented Mar 11, 2026

Closing — improvements from this PR have been incorporated into #22 where applicable. Thank you!

@tamirms tamirms closed this Mar 11, 2026
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.

2 participants