perf: wake the batch processor when a buffer nears capacity - #326
Open
drake-nominal wants to merge 1 commit into
Open
perf: wake the batch processor when a buffer nears capacity#326drake-nominal wants to merge 1 commit into
drake-nominal wants to merge 1 commit into
Conversation
drake-nominal
force-pushed
the
perf/enqueue-many
branch
from
August 27, 2026 01:07
e95d480 to
7bc9ba8
Compare
`batch_processor` parks for the full `max_request_delay` after each flush, and the only thing that cut that short was a write that no longer fit in either buffer -- by which point the writer is already blocked on the condvar, waiting out the remainder of the delay for a flush that was not otherwise due. Waking it as a buffer approaches capacity starts the flush while writers can still make progress, so `max_request_delay` becomes a ceiling on staleness rather than a floor on flush interval. Two properties of the threshold matter, and both were chosen from measurement rather than taste: It is edge-triggered. A level test re-`unpark`s on every write once past the threshold, which is pure overhead when a writer is saturating the buffer anyway. It sits at seven eighths of capacity, not half. `max_request_delay` exists to bound how often requests are issued, and waking early spends some of that budget. Writing 200,000 points through a 10,000 point buffer: no early wake 21 requests, median 10,000 points threshold at 1/2 34 requests, median 5,000 points (+62% requests, half the size) threshold at 3/4 26 requests, median 7,500 points threshold at 7/8 22 requests, median 9,000 points At seven eighths the request shape is indistinguishable from not waking early, while still giving the flush a head start. Blocking latency, six interleaved rounds at `max_request_delay=1.0s`: max_points_per_batch=6,480 3,878us -> 1,817us 2.13x max_points_per_batch=12,960 4,011us -> 2,500us 1.60x Every measurement of the new build beat every measurement of the old in both rows. Sustained throughput is unchanged (6.44 -> 6.64 Mpoints/s, within noise); the 3.6% regression an earlier half-capacity threshold showed is gone. Note on a concern raised in review: unparking a thread twice is harmless. `Thread::unpark` stores a single token rather than a counter, verified directly -- after two unparks the first `park_timeout` returns in ~1us and the second waits its full timeout. Wakeups cannot queue up or spin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drake-nominal
force-pushed
the
perf/flush-on-fill-v2
branch
from
August 27, 2026 02:27
e321ce6 to
5ea0e99
Compare
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.
Core-crate change. Rebased onto
mainnow that #320, #322 and #323 have landed; one commit.Addressing the review concern first
The worry was that this risks double-unparking the processor thread. It does not, and cannot.
Thread::unparkstores a single token, not a counter. Verified directly rather than from memory:Two unparks leave one token. The first
park_timeoutconsumes it and returns immediately; thesecond waits its full timeout. Wakeups cannot queue up, and the processor cannot spin.
Chasing that concern did surface a real one that had not been flagged, so it was worth raising.
The real cost, and what changed because of it
max_request_delaybounds how often requests are issued — the post-flush park is what keepsrequests large. Waking the processor early spends some of that budget. Writing 200,000 points
through a 10,000-point buffer:
At half capacity this issued 62% more requests at half the size — meaningfully more gRPC round
trips at the backend. That is a worse trade than the latency it bought, and it is what the earlier
3.6% throughput regression was really measuring.
At seven eighths the request shape is indistinguishable from not waking early at all, and the
regression is gone.
Results at the current threshold
Blocking latency, six interleaved rounds,
max_request_delay=1.0s:max_points_per_batchEvery sample of the new build beat every sample of the old in both rows.
When it helps: whenever a buffer fills before the flush timer fires — a high write rate, or a
max_points_per_batchsmall relative to rate x delay. When it does nothing: when the buffernever reaches seven eighths, which is the common case at the default 250,000 batch size with a
modest write rate.
Tests
Three deterministic unit tests on the threshold predicate — fires on the crossing write, does not
fire below it, and does not fire again once past it. Deliberately not timing-based: this repo
already has a flaky timing test (
test_time_flush, ~3/30 on main) and I did not want to addanother.
🤖 Generated with Claude Code