Conversation
|
I'll analyze this and get back to you. |
26316d9 to
95df94e
Compare
Reviewing PR #4085
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4085 +/- ##
==========================================
- Coverage 79.78% 79.61% -0.17%
==========================================
Files 477 481 +4
Lines 36663 36670 +7
==========================================
- Hits 29250 29196 -54
- Misses 7404 7465 +61
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
95df94e to
63775f5
Compare
Reviewing PR #4085
|
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
63775f5 to
2abc4f5
Compare
|
Claude finished @EgeCaner's task in 11m 19s —— View job Reviewing PR #4085
SummarySolid design and unusually thorough test suite (races, cancellation, lag resync, zero-allocation guards, No blockers or important issues found. One nit posted inline:
Also confirmed via grep that no code outside Note: |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| // SubscribeWith starts a subscription delivering O values: transform maps the ring's | ||
| // [ring.EventOrLag] iterator to any output iterator, for instance a lag policy yielding T | ||
| // or a mapper yielding a derived type. | ||
| func (s Subscribable[T]) SubscribeWith[O any]( |
There was a problem hiding this comment.
SubscribeWith has no test coverage anywhere in this PR — broadcast_test.go only exercises Subscribe(). It's public API with real behavioral nuance (the transform closure controls how/whether lag notifications survive, and it runs inside the pump goroutine, so a panicking or blocking transform takes the whole subscription down with it). Given this is called out in codecov as one of the coverage gaps in this file, a test subscribing with a transform (e.g. dropping lags, or mapping to a derived type) plus asserting Unsubscribe still terminates the pump goroutine would close the gap and pin the transform-composition behavior.

User description
What
Rewrites
utils/broadcastasbroadcaster/broadcast, a bounded fan-out event stream over aring buffer, split into two layers: a reusable MPMC ring (
broadcaster/broadcast/ring) and athin fan-out API over it. The old package is deleted in this PR; it had no callers outside its
own tests.
Overwrite-on-full semantics are unchanged: publishers never block on consumers, and a reader
that falls a full ring behind gets a
LaggedErrortelling it where to resume instead of staledata.
Nothing consumes the new package yet. Putting it and
feedbehind one interface, andmigrating the producers and RPC subscriptions, is the next PR.
What changes against
utils/broadcastSeparate publisher and subscriber types.
BroadcastmintsPublisherandSubscribablevalue handles instead of being one object that both sends and subscribes — the shape
feed.Feedalready has, so the next PR can put both behind one interface.No
Close.Close, thedonechannel andErrClosed/ErrFutureSeq/ErrInvalidSequenceare gone; only aSubscriptionhas a lifecycle, ended byUnsubscribe.No global lock. The old
Sendheld a broadcast-wide mutex:It covered two things.
Closewoke readers by broadcasting the slot at the current tail, whichis only correct if the tail cannot move under it. And sequence assignment was split —
Sendread the tail, stored, and only then incremented — so without the mutex two publishers claim
the same tail and the sequence between them is never published. The package doc conceded the
cost: "producer-side throughput is effectively single-writer (MPMC-safe but not truly parallel
on Send)".
Closeis gone, and the claim is now atomic:tail.Add(1)before the store, withSlot.Writeignoring a store whose sequence is not newer, since a writer can be lapped in between.
Publishers proceed in parallel.
Reading is an iterator. The old
Subscription.rundid the whole protocol in one loop —park on a slot, detect an overwrite, build the lag notification, push into the channel, watch
two stop signals. It now lives in its own package behind
RingBuffer.Iterator(ctx) iter.Seq[EventOrLag[T]], with the channel pump on top of it andSubscribeWithfor a transform in between.SendtakesT, not*T. The pointer parameter avoided copying a largeT, but thetypes we publish are already pointers, which makes it
**core.Block: the caller passes theaddress of its own pointer, and behind an interface — where the next PR takes it — that address
escapes, so every
Sendallocates. By value it is one word for a pointer, two for aninterface, and nothing escapes.
Sendalso returns nothing, andEventOrLagaccessors arecomma-ok (
AsEvent() (T, bool)) instead of theErrNoEvent/ErrNoLagsentinels.Benchmarks
Apple M3 Max,
GOMAXPROCS=14, Go 1.27.0. One publisher sending as fast as it can, payload*int, ring capacity 1024, all subscribers draining for the whole run. Go's default durationper configuration,
-count=3, median. All four transports run under one harness, with the oldpackage restored from
mainto measure it.feedisfeed.Feed.oldisutils/broadcast.new chanisSubscription.Recv().new iteris ranging the ring'siter.Seqdirectly.Each cell is publish rate / delivery rate per subscriber.
Feed's publisher collapses with subscriber count:
Sendholds one mutex and touches everysubscriber's channel on every message, so it falls from 24.5 M/s at one subscriber to 7 k/s at
1024. Past 128 subscribers nothing is dropped only because the publisher is too slow for anyone
to fall behind.
Old publishes faster than
new chanfrom 32 subscribers up. Per-subscriber delivery is withina few percent of it from 128 up and ahead of it at 32. At 1024 subscribers the two deliver the
same in total, 32.6 M/s against 33.5 M/s, with old putting more of the budget into publishing,
so each of its subscribers sees 5.5% of the stream against 18.7%.
Ranging the iterator skips the channel hop and is fast at both ends: 1.8 M/s published, 189 k/s
per subscriber, 193 M/s delivered across all of them.
Multiple publishers
Same harness, publisher count varied. Cells are publish rate / delivery rate per subscriber.
Feed and old are flat in publisher count — 1.6 to 1.5 M/s at 32 subscribers, 1.0 M/s to 960 k/s
at 1024 — which is the serialised
Send. The new ring scales with publishers instead, but onlyovertakes old in absolute terms at 32 subscribers, and there the extra rate comes straight out
of delivery. At 1024 subscribers old still publishes more and the two deliver the same. This is
the shape of the serialisation, not a throughput win.
PR Type
Enhancement, Tests, Other
Description
Move utils/broadcast to broadcaster/broadcast
Remove global lock for per-slot locks
Split API into Publisher and Subscribable handles
Introduce iter.Seq and zero-allocation EventOrLag
File Walkthrough
6 files
Introduce high-level Broadcast API wrapping ring bufferAdd Publisher type for sending to ring bufferAdd zero-allocation EventOrLag tagged union and LaggedErrorImplement MPMC ring buffer with atomic sequences and iteratorsImplement fixed-position Slot with per-slot mutex and conditionImplement Subscribable and Subscription wrapping ring iterators6 files
Add benchmarks for high-level broadcast throughputAdd comprehensive tests for broadcast semanticsAdd benchmarks for concurrent ring buffer reads and writesAdd unit tests for ring buffer operations and sequencesRemove old broadcast benchmarksRemove old broadcast tests2 files
Remove old broadcast implementationRemove old broadcast error definitions