Skip to content

Simplify Decoder API to use byte slices#21

Draft
tamirms wants to merge 2 commits into
masterfrom
simplify-api
Draft

Simplify Decoder API to use byte slices#21
tamirms wants to merge 2 commits into
masterfrom
simplify-api

Conversation

@tamirms

@tamirms tamirms commented Jan 6, 2026

Copy link
Copy Markdown

Summary

  • Replace the io.Reader-based Decoder with a new Decoder that operates directly on byte slices
  • Add Decode(v DecoderFrom) convenience method for decoder reuse with Reset()

Key API Changes

Before After
Unmarshal(io.Reader, v) Unmarshal([]byte, v)
NewDecoder(io.Reader) NewDecoder([]byte)

New Decoder Reuse Pattern

decoder := xdr.NewDecoder(nil)
// Decode multiple values, reusing the decoder
decoder.Reset(data1)
n, err := decoder.Decode(&value1)

decoder.Reset(data2)
n, err = decoder.Decode(&value2)

Benchmarks show ~10% improvement with decoder reuse.

Breaking Changes

  • Unmarshal now takes []byte instead of io.Reader
  • NewDecoder now takes []byte instead of io.Reader
  • Minimum Go version is now 1.22

Alternatively, I could introduce these changes in an xdr4 package to avoid breaking changes. However, I think we are the only users of this library and I preferred to remove existing code to make the codebase easier to maintain. But I am open to reconsidering this decision.

@leighmcculloch

leighmcculloch commented Jan 7, 2026

Copy link
Copy Markdown
Member
Before After
Unmarshal(io.Reader, v) Unmarshal([]byte, v)
NewDecoder(io.Reader) NewDecoder([]byte)

Would this mean that it's no longer possible to stream decode some XDR types? I'm thinking of a bucket which is a stream of framed bucket entries. How would I decode bucket entries with it being a byte slice? Does that mean the entire bucket has to be loaded into memory in a byte slice? There may be some cases where it would be preferred to still be able to use a reader.

@leighmcculloch

Copy link
Copy Markdown
Member

Benchmarks show ~10% improvement with decoder reuse.

Do we understand what's causing this improvement? I'm wondering if this improvement a result of removing the decoder interface, or if it's to do with buffering. Because we can do buffered reads using the bufio types and retain the reader interface.

This refactors the Decoder to work directly with byte slices instead of
io.Reader for improved performance, and simplifies the DecoderFrom interface.

API Changes:
- Decoder now takes []byte instead of io.Reader
- Unmarshal/UnmarshalWithOptions take []byte instead of io.Reader
- Add DecoderFrom interface for types to implement fast-path decoding
- Add Decoder methods: Reset(), Remaining(), Position(), MaxDepth()
- Remove MaxInputLen from DecodeOptions (now uses Remaining())
- Remove redundant maxDepth parameter from DecoderFrom interface

Depth Tracking:
- Add EnterScope()/LeaveScope() methods for stateful depth tracking
- Add currentDepth field to Decoder struct
- Allows generated code to track recursion depth across nested DecodeFrom
  calls without passing maxDepth as a parameter

Performance:
- Eliminate io.Reader overhead with direct buffer access

Bug Fixes:
- Fix setUnionArmsToNil to nil individual fields instead of whole struct
- Support value-type union arms in encode/decode (not just pointers)
- Fix error messages to show actual bytes written on partial writes

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@tamirms tamirms marked this pull request as draft January 8, 2026 09:34
Change DecoderFrom interface to accept maxDepth parameter directly,
eliminating the need for EnterScope/LeaveScope methods and the
currentDepth field in Decoder. This reduces overhead by:

- Removing defer calls for LeaveScope
- Eliminating struct field access for depth tracking
- Enabling better inlining opportunities

Implementations should decrement maxDepth when calling DecodeFrom
on nested types and return an error if maxDepth reaches 0.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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