Repository navigation
Docs: record why the host-log claim budget is not the knob it looks like - #2108
Merged
Merged
Conversation
`kProducerClaimAttempts = 1024` is the most knob-shaped constant in the host-log queue, so a run reporting drops invites reaching for it first. It is the wrong lever, and the reason takes a measurement to establish rather than an argument. A producer wins its MPSC slot on the first attempt 57–78% of the time, and the worst count across every workload shape tried is 86 — twelve times under the bound. A bound of 16, the alternative considered, would have turned 787 successful writes at 64 threads into drops to save a worst case of ~100 µs that never occurs. Every loss lands in `queue_full` instead, and that is structural: a full queue exits before spending any attempt, so the two causes are nearly mutually exclusive. The entry also records the first conclusion, which was wrong. A saturating benchmark reports `claim_exhausted == 0` and looks like an answer, but saturation routes producers through the queue-full early exit and barely visits the claim loop, so it says nothing about the attempt distribution below the bound. Pacing the producers is what puts that path under test — and only the per-cause breakdown hw-native-sys#2029 added makes either reading possible, since a single total cannot separate a queue that is too small from a budget that is too tight from a destination that is broken. Two properties of the implementation go in alongside, neither required by hw-native-sys#1792 item 6 and neither covered by a test: a producer preempted between claiming a position and publishing it parks the writer on that position, so later published records cannot drain and other producers begin dropping; and the losses under contention fall on the slowest producers, which is the opposite of the useful bias for diagnostics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
kProducerClaimAttempts = 1024is the most knob-shaped constant in the host-logqueue #2029 introduced, so the first response to a run reporting drops is to reach
for it. It is the wrong lever, and establishing that took a measurement rather
than an argument — so this records the measurement instead of leaving the next
person to redo it.
Docs-only. No code changes.
What it says
Verdict: do not change it. A producer wins its MPSC slot on the first
attempt 57–78% of the time, and the worst count across every workload shape
tried is 86 — twelve times under the bound.
Unpaced 64 threads: max 86.
A bound of 16 — the specific alternative considered — would have turned 787
successful writes (0.06%) at 64 threads into
claim_exhausteddrops, to save aworst-case CPU burn of ~1.6 µs instead of ~100 µs that never occurs.
Every loss lands in
queue_full, at 4, 16 and 64 threads. That is structural,not incidental: a full queue exits on
difference < 0before spending anyattempt, so
queue_fullandclaim_exhaustedare nearly mutually exclusive byconstruction. The constraint on loss is
kQueueCapacity— the writer's drainrate.
And the conclusion that was wrong first
The entry keeps the failed reading, because it is the one a reader is likely to
repeat. A saturating benchmark reports
claim_exhausted == 0and looks likean answer — but saturation routes producers through the queue-full early exit and
barely visits the claim loop, so it says nothing about the attempt distribution
below the bound. That distribution is what decides whether the loop holds the
calling thread and whether a smaller bound would fit. Pacing the producers is
what puts the claim path under test.
Two questions were being conflated: does the budget cause drops (the counter
answers it) and how many attempts does a producer spend (the counter cannot).
Only the per-cause breakdown #2029 added makes either answerable, since a single
total cannot separate a queue that is too small from a budget that is too tight
from a destination that is broken.
Two properties recorded, not fixed
Neither is required by #1792 item 6 — which asked for "never block; drop and
count" and gets both — and neither is covered by a test:
pop()is strictly in-order, so a producerpreempted between claiming position P and publishing
sequence = P+1parksthe writer on P; records at P+1 and beyond cannot drain even though they
are published, the queue fills behind the gap, and other producers begin
dropping. The writer sleeping rather than spinning is tested
(
WriterSleepsWhenAdjacentProducersPublishOutOfOrder); the queue not backingup is not — different properties.
is the opposite of the useful bias for diagnostics: the thread that is stuck is
often the one worth observing.
Caveats stated in the entry
The 45–94% drop rates it quotes come from deliberately pathological unpaced
workloads and are not representative; what they establish is the queue's role
— it absorbs bursts, not sustained overload, which is what its own comment claims
and now has a number behind. Everything is one machine (320-core aarch64 at load
~80) and two workload shapes, so the defensible reading is "1024 has ~12× headroom
over the worst observed and 16 does not", not "1024 is optimal". A value covering
everything observed with margin would be ~128–256; that is stated, along with why
shrinking to it still is not worth doing.
Testing
Docs-only, so no build or suite is affected.
markdownlint-cli2,check_english_onlyandcheck_retired_namesclean; both relative links(
../logging.md,../dfx/host-trace.md) resolve.Indexed in
docs/investigations/README.mdperdiscipline.md§4 — an unlinkedentry is invisible, and the index is the only discovery surface.
Follows #2029 (item 6 of #1792).