feat: add Snapshot() for non-destructive FIFO copy#10
Merged
Conversation
1.duplicated wrap branch -> share via segments() 2.keep cache locality -> copy+clear per segment
1.gap between Peek and Drain -> add Snapshot 2.wrap region needs bulk copy -> use segments
1.no thread-safe non-destructive read -> add it 2.no user code under lock -> delegate-only impl
1.no perf baseline for Snapshot -> add benches 2.wrap branch could regress -> add Wrap variant
1.unreleased section empty -> add Snapshot 2.link to tracking issue -> reference #9
1.cap shadows builtin -> rename to bufCap
1.refactor dropped early return -> restore guard 2.pre-refactor was true no-op -> match contract
1.pointer T sharing implicit -> note shallowness 2.O(N) lock hold not stated -> note in GoDoc
1.new Snapshot benches absent in docs -> add rows 2.narrative claim was stale -> note alloc cost
There was a problem hiding this comment.
Pull request overview
This PR adds a new non-destructive bulk-read primitive, Snapshot(), to both core data structures (RingBuffer[T] and the concurrent RingQueue[T]) to complement Peek() and Drain() while preserving FIFO order and buffer contents.
Changes:
- Added
Snapshot()toRingBuffer[T]andRingQueue[T], including API docs and comprehensive tests/benchmarks. - Refactored
RingBufferbulk operations by extracting a wrap-awaresegments()helper used byDrain()/Clear()/Snapshot(). - Updated benchmark sync targets, benchmark docs tables, and the changelog to reflect the new operation.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ringbuffer.go | Refactors drain/clear via segments() and introduces RingBuffer.Snapshot(). |
| ringbuffer_test.go | Adds Snapshot test matrix (I1–I7) and Snapshot benchmarks. |
| ringqueue.go | Adds RingQueue.Snapshot() with mutex-held copy semantics. |
| ringqueue_test.go | Adds Snapshot tests (G1–G5) and a queue Snapshot benchmark. |
| cmd/benchsync/main.go | Registers Snapshot benchmarks in default benchsync targets. |
| cmd/benchsync/main_test.go | Updates golden output to include Snapshot benchmarks. |
| docs/ringbuffer.md | Updates benchmark table and notes to include Snapshot performance. |
| docs/ringqueue.md | Updates benchmark table to include Snapshot. |
| CHANGELOG.md | Adds Unreleased changelog entry for Snapshot APIs. |
| .gitignore | Ignores .mx/. |
1.G4 monotonicity passes if len(snap) < 2 2.no direct test of RingQueue wrap path -> G6
1.bench table mentioned it -> Methods missed it 2.shallow + lock semantics user-facing -> note
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
Snapshot()to bothRingBuffer[T]andRingQueue[T]— a non-destructive FIFO copy that fills the gap betweenPeek()(one item) andDrain()(all items, mutating). Foundational primitive for replay / inspect / filter-without-consuming use cases.Related issues
Closes #9.
Changes
refactor: extract RingBuffer segments helper— pull wrap-detection out ofDrain/Clearinto an unexportedsegments()helper. Behavior-preserving;BenchmarkRingBuffer_Drainshows no regression (~1290 ns/op vs ~1296 ns/op baseline). Drain's per-segmentcopy → clearinterleaving preserves cacheline locality.feat: add RingBuffer Snapshot— non-destructive FIFO copy usingsegments(). Tests I1–I7 cover empty, non-wrap FIFO, wrap-around FIFO, non-mutation, caller / buffer isolation, full-capacity round-trip.feat: add RingQueue Snapshot— lock + delegate toRingBuffer.Snapshot(). No user code ever runs underq.mu. Tests G1–G5 cover empty, FIFO, non-mutation, concurrent enqueue + snapshot (race-clean, monotonicity invariant), and snapshot-after-close.test: add Snapshot benchmarks— three benchmarks:BenchmarkRingBuffer_Snapshot(non-wrap),BenchmarkRingBuffer_Snapshot_Wrap(split region),BenchmarkRingQueue_Snapshot. All 1 alloc/op; wrap and non-wrap within noise, confirming the wrap path stays on bulkcopy.doc: changelog Snapshot entry— Unreleased section.style: rename bench const cap—capshadowed the builtin; renamed tobufCap.fix: preserve Clear no-op on empty buffer— restored theif rb.size == 0 { return }guard that the refactor inadvertently dropped, matching pre-refactor contract.doc: clarify Snapshot shallow copy and lock cost— note that independence is shallow for pointer/pointer-containingT; documentRingQueue.Snapshot's O(N) lock-hold cost.sync: register Snapshot in bench docs tables— added rows for the new benchmarks tocmd/benchsync/defaultTargetssomake bench-synckeepsdocs/ringbuffer.mdanddocs/ringqueue.mdcurrent.Out of scope (deferred — see #9 for rationale):
AppendTo,ForEach,At,MaxAge,Resize.Checklist
Required
make checkpasses (fmt→lint→test)commit-message-instructions.mdConditional
make benchbefore/after); Drain refactor shows no regression, Snapshot benches added