Add: ProfilerBase::quiesce() to drain without retiring the threads - #2091
Merged
Conversation
stop() couples two things: draining the pipeline and retiring the worker threads. Its drain guarantee is in fact *paid for* by the threads exiting — joining mgmt is what proves its final sweep landed in the host shards, and joining the collectors is what proves those shards were consumed. That coupling is why the DFX collectors are started and joined per run: there is no way to reach a known-empty state without tearing them down. quiesce() supplies the missing half — the same guarantee, threads intact — using a two-phase epoch handshake in place of the joins. The phases are ordered rather than concurrent. A collector reporting its shard empty before mgmt has finished sweeping would be reporting on a queue mgmt is about to push into, so collect_quiesce_epoch_ is published only once every drain ack for that epoch has landed. The caller having stopped the device-side producers is what makes a drain worker's "one full sweep found nothing" conclusive; that already holds where the DFX teardown runs. The collector-side ack sits above the has_seen_buffer guard on purpose: a shard that never received a buffer is a valid run shape, and an ack behind that guard would leave quiesce() waiting on a silent subsystem forever. The new silent-collector test hangs if it is moved. stop()'s implementation is unchanged, so no existing behavior moves. Also drops the L1/L2 shorthand from this file's comments. It denoted the device-side ring and the host ready queue shards, but L1 and L2 are already taken by the hierarchy model (die and chip runtime), so the same coordinates meant two unrelated things.
|
Warning Review limit reachedNext included review available in 45 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 |
7 tasks
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
stop()couples two things: draining the collector pipeline and retiring itsworker threads. Its drain guarantee is in fact paid for by the threads
exiting — joining mgmt is what proves its final sweep landed in the host ready
queue shards, and joining the collectors is what proves those shards were
consumed. Its own comment spells out the ordering that depends on this.
That coupling is why the DFX collectors are started and joined per run
(#2078): there is no way to reach a known-empty state without tearing them
down.
quiesce()supplies the missing half — the same guarantee, threadsintact — using a two-phase epoch handshake in place of the joins.
stop()'s implementation is untouched; the only lines this PR removes from itare comment text. So nothing that exists today changes behaviour, and this
lands as a standalone primitive ahead of the collector-residency work.
Two things the design is forced into
The phases are ordered, not concurrent. A collector reporting its shard
empty before mgmt finished sweeping would be reporting on a queue mgmt is about
to push into — so
collect_quiesce_epoch_is published only once every drainack for that epoch has landed.
The alternative — having the caller drain the device-side ring itself — is not
available: mgmt is already consuming it, and a second consumer breaks the SPSC
invariant. What makes a drain worker's "one full sweep found nothing"
conclusive is the caller having stopped the device-side producers, which is
already true where the DFX teardown runs.
The collector ack sits above the
has_seen_bufferguard. A shard thatnever received a buffer is a valid run shape (the loop's existing comment says
so), and an ack behind that guard would leave
quiesce()waiting on a silentsubsystem forever.
Testing
test_profiler_base9/9 (was 6) — three new cases: drain-without-retire,silent-collector completion, and no-op before
start()/ afterstop()below
has_seen_bufferand rebuilding makesQuiesceCompletesOnASilentCollectorhang (ctest 30s timeout) while the preceding case still passes — so the test
targets that failure specifically rather than passing incidentally
quiesce()yet;stop()is unchanged, sothe runtime paths are untouched by this PR
QuiesceDrainsWithoutRetiringThreadsdeliberately does not poll withwait_for_collected:quiesce()must have delivered everything by the time itreturns, so polling would hide a handshake that reports too early.
Also
Drops the
L1/L2shorthand from this file's comments. It denoted thedevice-side ring and the host ready queue shards, but
L1andL2are alreadytaken by the hierarchy model (
docs/hierarchical-level-runtime.md: L1 = chipdie, L2 = chip runtime), so the same coordinates carried two unrelated
meanings. Comment-only; the file now has zero
L1/L2matches.Step 1 of the plan in
#2078 (comment).