Skip to content

drpcstream: introduce shared BufferPool for ring buffer#55

Open
shubhamdhama wants to merge 1 commit into
mainfrom
shubham/buffer-pool-for-ringbuffer
Open

drpcstream: introduce shared BufferPool for ring buffer#55
shubhamdhama wants to merge 1 commit into
mainfrom
shubham/buffer-pool-for-ringbuffer

Conversation

@shubhamdhama

@shubhamdhama shubhamdhama commented Apr 17, 2026

Copy link
Copy Markdown

Add a BufferPool backed by sync.Pool that is shared across all streams
within a Manager. The ring buffer now obtains buffers from the pool on
Enqueue and transfers ownership to the caller on Dequeue, which advances
the tail immediately. This removes the two-step Dequeue/Done protocol
and simplifies Close (no longer needs to wait for held buffers).

The pool is a required parameter in the Stream constructor, created once
per Manager and passed to all streams it creates.

@shubhamdhama

Copy link
Copy Markdown
Author

I had an idea which I ran through claude and below is the summary of it. I'm not planning to do it but in future we can re-consider if profiling shows any gain.

Buffer pool: further optimization ideas

Right now the data copy chain for an incoming message looks like this:

  1. PacketAssembler.AppendFrame: copies frame data into pa.pk.Data via append
  2. ringBuffer.Enqueue: copies pkt.Data into a pooled buffer via append
  3. RawRecv copies the pooled buffer out, or MsgRecv unmarshals from it

We could eliminate copy #2 by having the packet assembler get its buffer from the pool directly. The assembler already has a TODO for buffer reuse. Instead of reusing its own backing array across packets (lines 84-87), it would pool.Get a buffer, assemble into it, hand it off through the ring buffer, and pool.Get a fresh one for the next packet.

Another idea: size-bucketed pools (e.g. 1KiB, 16KiB, 32KiB) so that Enqueue's append doesn't have to reallocate when messages are larger than the default capacity. You could even have a pool.Append(buf, data...) method that detects when the buffer needs to grow and fetches from the right bucket.

I think we should keep the pool simple for now. sync.Pool already gives you natural high-water-mark behavior: a buffer that grew to 32KiB stays at 32KiB when returned, so after warm-up the pool self-tunes to the workload's size distribution. Size buckets would add real maintenance cost (choosing boundaries, handling cross-bucket transitions) for a gain that append + sync.Pool already provides. Latency here is dominated by network IO anyway, not memcpy.

The assembler integration is the more interesting optimization since it removes a full copy per message. Worth revisiting once we have benchmarks to measure the actual impact.

@shubhamdhama shubhamdhama force-pushed the shubham/enable-stream-multiplexing branch from a17330d to b91bf1b Compare April 17, 2026 14:57
@shubhamdhama shubhamdhama force-pushed the shubham/buffer-pool-for-ringbuffer branch from cafa1dc to b3d2355 Compare April 17, 2026 14:57
@shubhamdhama shubhamdhama force-pushed the shubham/enable-stream-multiplexing branch from b91bf1b to a58986c Compare April 17, 2026 15:00
@shubhamdhama shubhamdhama force-pushed the shubham/buffer-pool-for-ringbuffer branch from b3d2355 to 38c84dc Compare April 17, 2026 15:00
Base automatically changed from shubham/enable-stream-multiplexing to stream-multiplexing April 17, 2026 16:30
@shubhamdhama shubhamdhama force-pushed the shubham/buffer-pool-for-ringbuffer branch from 38c84dc to f2f767f Compare May 11, 2026 11:03
Add a BufferPool backed by sync.Pool that is shared across all streams
within a Manager. The ring buffer now obtains buffers from the pool on
Enqueue and transfers ownership to the caller on Dequeue, which advances
the tail immediately. This removes the two-step Dequeue/Done protocol
and simplifies Close (no longer needs to wait for held buffers).

The pool is a required parameter in the Stream constructor, created once
per Manager and passed to all streams it creates.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@shubhamdhama shubhamdhama force-pushed the shubham/buffer-pool-for-ringbuffer branch from f2f767f to a305254 Compare June 9, 2026 07:36
@shubhamdhama shubhamdhama changed the base branch from stream-multiplexing to main June 9, 2026 07:37
Comment thread drpcstream/buffer_pool.go
// stream receive path. Buffers obtained via Get should be returned via
// Put when no longer needed. Forgetting to Put is safe (GC reclaims)
// but reduces reuse.
type BufferPool struct {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we use type adapter here?

Comment thread drpcstream/ring_buffer.go
}

rb.buf[rb.head] = append(rb.buf[rb.head][:0], data...)
b := rb.pool.Get()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These can me move outside the lock now.

Comment thread drpcstream/ring_buffer.go
// TODO(shubham): remove this method once a shared buffer pool is introduced.
// With a pool, Dequeue will advance the tail immediately and the caller will
// return the buffer to the pool directly.
func (rb *ringBuffer) Done() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given the buffer pool changes, we can retain the contract of ringBuffer and keep Done() to release the buffer back to the pool. That way, the consumer doesn't have to know about the internals of ring buffer whether it is using the pool or fixed buffers.

Comment thread drpcstream/stream.go
data = append([]byte(nil), data...)
s.recvQueue.Done()
data = append([]byte(nil), *b...)
s.pool.Put(b)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should continue to use s.recvQueue.Done(). Refer to other comments for context.

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