Arm the resident DFX collectors under the execution claim - #2163
Open
ChaoWao wants to merge 1 commit into
Open
Conversation
The collectors are resident objects that serve every run, so opening a run's collection window is destructive: begin_run() drops the previous run's records and counters and republishes the level the device reads. That ran during prepare_execution, which on onboard can run for a successor while a predecessor is still executing — it would have reset the collectors out from under the live run. Move it to launch, where the run holds the execution claim and is therefore the only run touching those collectors. begin_run() now sits beside the start() it belongs with, in start_shared_collectors_for_run() on both bases, which collapses five per-runner copies into one per platform. The level cannot move with it: chip-swimlane sizes its orch phase pool from the level and args-dump writes it into DumpDataHeader, and both happen in initialize(), which must stay in prepare because its device pointers go into kernel_args before it uploads. So initialize() takes the level as an argument instead of reading what begin_run() latched, which also removes an ordering constraint between two calls that no longer share state. Launch and drain read the run's configuration from its own PreparedExecution rather than from the runner, which needs no new plumbing: PreparedExecution already carried the run's CallConfig by value, so it now resolves DfxRunConfig from it at construction. The two shared-collector bracket helpers and the host-phase-records writer take it as an argument. Six runner members are dead as a result — enable_chip_swimlane_, enable_dump_args_, enable_pmu_, enable_scope_stats_, pmu_event_type_, capture_clock_anchors_ — with every remaining occurrence a declaration or a setter assignment. They are removed with their setters. Leaving a write-only enable_pmu_ on the runner is the exact shape of the defect this change is about: it reads as the source of truth and is not one. chip_swimlane_level_ stays because a device-context query answers from it, and output_prefix_ because host_phase_pool_arm() reads it. a5's PMU degradation becomes a per-run value. When init_pmu fails, a5 disables PMU for that run rather than failing it; that flag was the one DFX value prepare still wrote runner-wide, and it now lands on the run's own dfx, which its arming and its teardown both read. a2a3 fails the whole run on the same error — an undocumented arch divergence, filed as part of hw-native-sys#2162 and left alone here. This does not remove the diagnostics depth-one gate. What remains behind it is pool construction, not per-run arming: a prepare whose collector shape differs from the resident one calls finalize_collectors(), which frees device memory the predecessor is using. That is a narrower condition than "any diagnostic is on" and is the next change on this line. Sim takes the same shape but not the same fix: it acquires the execution claim at the top of simpler_prepare_run, so it never prepares a successor against a live predecessor. Keeping the two runner shapes identical is the point — see hw-native-sys#2162. Verified per DFX channel in the shapes _st-sim-{a2a3,a5}.yml uses; a bare sweep enables no channel and cannot fail on a collector defect. 12 channel runs, 23 cases, green on both sim platforms. The negative control fires on the relocated code: with begin_run() omitted from start_shared_collectors_for_run(), all 5 a5sim swimlane cases fail. Full sim sweeps green on both platforms (33 and 29 cases), pyut 2216 passed / 7 skipped, cpput 135/135 from a cleared build dir — which is what caught the three test call sites of the changed initialize() — and a2a3 onboard smokes green under task-submit across PMU, chip_swimlane + dep_gen, hbg dep_gen and args_dump: 11 cases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (18)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDFX configuration now lives on each ChangesPer-run DFX lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to Runs that change chip-swimlane diagnostic level may continue using the prior run’s collector level, producing incomplete or incorrect diagnostics. This should be corrected before merge. Sequence Diagram(s)sequenceDiagram
participant CallConfig
participant PreparedExecution
participant DeviceRunner
participant Collectors
CallConfig->>PreparedExecution: create DfxRunConfig
PreparedExecution->>DeviceRunner: provide prepared.dfx
DeviceRunner->>Collectors: begin_run and start
DeviceRunner->>Collectors: teardown with dfx
Collectors-->>DeviceRunner: write configured artifacts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. A rabbit reads each line, 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.
Summary
The DFX collectors are resident objects that serve every run, so opening a run's collection window is destructive:
begin_run()drops the previous run's records and counters and republishes the level the device reads. That ran duringprepare_execution, which on onboard can run for a successor while a predecessor is still executing.This moves it to launch, where the run holds the execution claim and is the only run touching those collectors — and carries the run's own DFX configuration through launch and drain instead of reading the runner's members.
Part of #2078.
What changed
begin_run()× 5 collectors × 4 runnersstart_shared_collectors_for_run()on each base, beside thestart()it belongs withinitialize()for chip-swimlane / args-dumpbegin_run()latchedPreparedExecution::dfx, not runner membersdfx.pmu_enabled = false— a per-run value, no longer a runner-wide writeinitialize()could not move withbegin_run(): chip-swimlane sizes its orch phase pool from the level and args-dump writes it intoDumpDataHeader, and its device pointers must be inkernel_argsbefore that uploads at the end of prepare. Passing the level in removes the ordering constraint between two calls that no longer share state.PreparedExecutionalready carried the run'sCallConfigby value, so carryingDfxRunConfigneeded no new plumbing.Why the dead members go in this commit
enable_chip_swimlane_,enable_dump_args_,enable_pmu_,enable_scope_stats_,pmu_event_type_andcapture_clock_anchors_are now write-only — every remaining occurrence is a declaration or a setter assignment, which is what let the compiler confirm the removal. A write-onlyenable_pmu_on the runner is the exact shape of the defect this change is about: it reads as the source of truth and is not one.chip_swimlane_level_stays (a device-context query answers from it) andoutput_prefix_stays (host_phase_pool_arm()reads it).What this does not do
The diagnostics depth-one gate stays. What remains behind it is pool construction, not per-run arming: a prepare whose collector shape differs from the resident one calls
finalize_collectors(), which frees device memory the predecessor is using. That is a much narrower condition than "any diagnostic is on", and turning the gate into it is the next change on this line.Sim takes the same shape but not the same fix. It acquires the execution claim at the top of
simpler_prepare_run, so it never prepares a successor against a live predecessor. Keeping the two runner shapes identical is the point — see #2162.Testing
Verified per DFX channel in the shapes
_st-sim-{a2a3,a5}.ymluses. A bare sweep enables no channel and so cannot fail on a collector defect.begin_run()omitted fromstart_shared_collectors_for_run(), all 5 a5sim swimlane cases failinitialize()task-submitacross PMU, chip_swimlane + dep_gen, hbg dep_gen and args_dump: 11 casesRelated: #2078, #2162