Refactor: reduce the clock anchor to one offset reading - #2125
Closed
doraemonmj wants to merge 1 commit into
Closed
Conversation
Map device timestamps onto the Host clock from a single paired reading. The anchor supplies an offset and nothing else, because the scale is the platform's counter frequency. A second reading only contributed a residual the calibration interval was too short to measure: across four Ranks the implied frequency swung between -813 and +2105 ppm with inconsistent sign, while the correction it produced stayed an order of magnitude below the sampling noise it was computed from. One reading pins the two clock domains rather than bounding an interval the records must fall inside, so a timestamp either side of it maps by the same arithmetic. That removes the placement constraint that made each runtime open its interval at a different point, and both now read at the launch boundary. Anchor the clocks for any enabled chip swimlane instead of level 4 alone. A capture below that level is therefore placed on the Host timeline too, rather than on its own relative one, and the seam a fail-soft conversion used to leave unmeasured is gone for single-card captures as well. Cross-Rank merging still requires level 4, now because _validate_l3_rank_data asks for the orchestrator phase records only that level carries. Drop the closing reading and the coverage check it fed: with the anchor a pin there is no interval left for a timestamp to fall outside of. CallConfig::capture_clock_anchors stays on the wire as the ChipWorker marker it already was, but no longer gates anything on its own. Cover the mapping either side of the anchor, including the toward-zero truncation that keeps the two directions symmetric.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (21)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChip swimlane profiling now captures one HostOffset reading when enabled. Python alignment maps device cycles using that offset and nominal frequency. Runtime finalization APIs no longer use device-completion flags. Documentation and tests reflect the new behavior. ChangesClock correlation and swimlane profiling
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The clock-anchor refactor is ready to merge; no actionable current behavior or compatibility risk remains. Sequence Diagram(s)sequenceDiagram
participant DeviceRunnerBase
participant ClockCorrelationSession
participant SwimlaneConverter
participant ClockAlignment
DeviceRunnerBase->>ClockCorrelationSession: begin at launch boundary
ClockCorrelationSession->>ClockCorrelationSession: capture HostOffset sample
SwimlaneConverter->>ClockAlignment: build alignment from clock anchors
ClockAlignment->>ClockAlignment: select valid anchor
SwimlaneConverter->>ClockAlignment: map device cycles to Host time
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 13.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 18 files. (3 skipped: 3 unsupported.)
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
1 participant
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.
Map device timestamps onto the Host clock from a single paired reading. The
anchor supplies an offset and nothing else, because the scale is the platform's
counter frequency.
Follow-up to the L3 multi-rank swimlane (#2058 / #2099); closes no issue of its
own.
What the device records are missing is an origin, not a scale
A capture carries its two domains explicitly —
device_clock_domain: device_syscnt_cyclesagainstorchestrator_clock_domain: host_monotonic_ns—and
clock_freq_hzis known a priori from the platform. So every intervalinside the device domain is already computable: a duration, a gap between two
tasks, the span of a scheduler phase are all
cycles x 1e9 / clock_freq_hzandneed no calibration at all.
What the device side cannot supply is where any of that sits on the Host clock.
Its timestamps are syscnt counts against a device-boot epoch with no defined
relation to
CLOCK_MONOTONIC. The records give spans without a start.The same shape shows up one layer up, in the host trace.
to_host_swimlanekeeps every
clk=devspan out of Perfetto's visibletraceEventsand parks itin
unalignedDeviceSpans, because "Chrome Trace JSON has one timestamp axis, soraw
clk=devevents cannot be rendered alongside host events without either afalse clock alignment or a huge empty interval". Those spans,
device_wallamong them, carry lengths and nothing that says where to put them. An offset is
the missing half in both places.
That leaves exactly one unknown scalar, and
metadata.timeline_relation: host_orchestration_precedes_deviceis what the format could assert without it:an ordering, never a distance. An anchor measures that one scalar. A second
reading would be measuring the scale, which was never missing.
How accurate the stitch has to be
A swimlane is read to see where work sits relative to other work: which Rank
started late, how long a submit gap is, whether a device segment overlaps the
host orchestration that queued it. Those features are milliseconds wide — the
device segments in the captures below run 0.1 to 10.7 ms, the submit gaps 1.5 to
3.0 ms. Placing a block within tens of microseconds is comfortably inside what
the picture needs, and the error is declared rather than assumed: each block
carries
max_uncertainty_ns(14.0-28.5 us in these runs) and a cross-domaincomparison costs the sum of two (49.3 us for four Ranks).
This is the reason the residual correction is not worth its cost. It is not
merely that a second reading is noisy — it is that the quantity it corrects,
1.8-8.1 us, sits below the bound the consumer already tolerates.
Why one reading is enough in practice
The second reading only contributed a residual the calibration interval was too
short to measure. Across four Ranks of a
TestAllreduceTwophaseP4capture theimplied frequency swung between -813 and +2105 ppm with inconsistent sign,
while the correction it produced stayed an order of magnitude below the
sampling noise it was computed from (30-58 us).
Supplying an offset pins the two clock domains rather than bounding an interval
the records must fall inside, so a timestamp either side of it maps by the same
arithmetic. That removes the placement constraint that made each runtime open
its interval at a different point, and both now read at the launch boundary.
What changes
capture below that level is placed on the Host timeline too, rather than on
its own relative one, so the seam a fail-soft conversion used to leave
unmeasured is gone for single-card captures as well. Cross-Rank merging still
requires level 4, now because
_validate_l3_rank_dataasks for theorchestrator phase records only that level carries.
there is no interval left for a timestamp to fall outside of.
CallConfig::capture_clock_anchorsstays on the wire as the ChipWorkermarker it already was, but no longer gates anything on its own.
Verification
pytest tests/ut/py— 2121 passed, 24 skipped.Onboard a2a3, both runtimes at level 4:
TestAllreduceTwophaseP4, 4 cards. All four Ranks calibrated;global_origin_nsequals the minimum per-Rank origin, so the anchor suppliesthe offset and the earliest event still sets the origin. The four Ranks end
within an 11 us window (7798-7809 us) — a collective rendezvous is what a
correct alignment should show, and the agreement sits inside the declared
49.3 us cross-Rank bound. No negative timestamps.
TestBatchPagedAttentionHostBuildGraph, 1 card.layout: clock_aligned,no unaligned device spans, 4096 AICore tasks all after the anchor. The host
orchestration lane (0-3362 us) and the device lanes (38208-40831 us) sit on
one axis; the fail-soft layout would have placed the device segment at
307.9-345.7 us, 36 ms off, with the error unmeasurable by construction.
timestamps exactly.
New unit tests cover the mapping either side of the anchor and the toward-zero
truncation that keeps the two directions symmetric.