Move change batching out of the delivery queue - #1163
Draft
dwcullop wants to merge 1 commit into
Draft
Conversation
SharedDeliveryQueue took an optional callback that fired once per drain cycle, and CacheParentSubscription used it as its only emit point. That put a batching policy inside a class whose job is serialization, and it batched more than it should: DrainPending drains whatever is queued, so work another thread enqueued mid-drain landed in the same emission. CacheParentSubscription now tracks its own delivery frame. Each notification increments a depth counter, and the accumulated changes are emitted when it returns to zero. A child that emits synchronously during parent processing is delivered inline by the queue's reentrant path, so it nests inside the parent's frame rather than emitting separately. One upstream notification plus everything it triggers synchronously still produces one downstream changeset, which the six operators built on this class already relied on. What changes is that a second thread's work is no longer folded into the same emission. It gets its own frame. No lock is needed around the depth counter. The queue has already serialized delivery, so only one thread is ever inside these methods, and the queue's lock provides the barrier between drains on different threads. With that moved, the callback has no consumers, so the field, the overload that took it and the invoke site are all gone. SharedDeliveryQueue now only serializes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9582bb33-26d3-4aa5-8dd7-57dc55304680
This was referenced Aug 6, 2026
JakenVeina
reviewed
Aug 6, 2026
| private void OnDrainComplete() | ||
| private void DeliverParent(IChangeSet<TParent, TKey> changes) | ||
| { | ||
| ++_frameDepth; |
Collaborator
There was a problem hiding this comment.
Would it be crazy to say that this should be a StartFrame() or BeginFrame() method, just for the clarity that it pairs with EndFrame()?
Would it be even crazier to wrap up this logic in a private struct FrameTracker : IDisposable that does the tracking, and lets you just collapse it all to a using?
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.
SharedDeliveryQueuetakes a callback that fires once per drain cycle, andCacheParentSubscriptionuses it as its only emit point. That is a batching policy living inside a class whose job is serialization. Raised on #1114.It also batches more than intended.
DrainPendingdrains whatever is queued, so work another thread enqueued mid-drain lands in the same emission.Change
CacheParentSubscriptiontracks its own delivery frame:A child that emits synchronously during parent processing is delivered inline by the queue's reentrant path, so it nests inside the parent's frame instead of emitting separately. One upstream notification plus everything it triggers synchronously still produces one downstream changeset.
The callback then has no consumers, so the field, the overload taking it, and the invoke site all go.
SharedDeliveryQueueonly serializes now.No lock around the depth counter. The queue has already serialized delivery, so only one thread is ever inside those methods, and the queue's lock is the barrier between drains.
The batching is load-bearing
EmitChangesis abstract and that callback was its only caller, so the six operators built on this class stop emitting entirely without a replacement.Emitting per notification instead fails 34 tests, all on changeset granularity.
ClearingParentEmitsSingleChangeSetgives 15 where it expects 2.OrderOfChangesIsPreservedgives 11 where it expects 2, and that one is more than a count: a singleEditdoingClear()thenAddOrUpdate()has to arrive as one changeset, or the collection is observably emptied, which is a state that never existed upstream.What actually changes
A second thread's work is no longer folded into the same emission. It gets its own frame.
Related
#1162 fixes the queue delivering out of receipt order. Independent, but this change exposes that one: once emission is per frame, delivery order becomes changeset order. Same file, so whichever lands second needs a trivial merge.