Add: expose L3/L4 host scheduling swimlane - #1730
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a host-span ABI and logger bridge, instruments hierarchical execution, preloads the logger before extension loading, publishes runtime binaries atomically, and adds filtered host swimlane JSON output with real PID/TID lanes and unaligned device spans. ChangesHost tracing and swimlane output
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythonWorker
participant HierarchicalScheduler
participant HostLogger
participant StraceTiming
PythonWorker->>HostLogger: emit l3.graph_build
HierarchicalScheduler->>HostLogger: emit submit, dispatch, frame, activation, completion spans
HostLogger-->>StraceTiming: write STRACE records
StraceTiming-->>StraceTiming: filter spans and build PID/TID lanes
StraceTiming-->>StraceTiming: write flow events and unalignedDeviceSpans
Possibly related PRs
Poem
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/worker.py`:
- Around line 6475-6482: Update Worker.init() so
_initialize_simpler_log(binaries) and _bind_host_span_sink() run before any
local forks for every hierarchical worker, including L4 parents without
device_ids. If the sink bind returns false, quietly disable host tracing instead
of raising; set self._host_trace_enabled only when the bind succeeds.
In `@simpler_setup/tools/strace_timing.py`:
- Line 414: Rename the loop variable in the span.attrs iteration to avoid
shadowing the imported field symbol and resolve Ruff F402, updating its
references within the loop accordingly.
In `@src/common/hierarchical/worker_manager.cpp`:
- Around line 466-475: Capture the dispatch trace metadata before the worker is
notified: in the dispatch flow around cv_.notify_one(), read and store the run
ID, callable hash, and attributes from ring_ while d.task_slot is still valid.
After unlocking, emit l3.dispatch using only those captured values, preserving
the existing timing and notification behavior.
In `@src/common/log/host_log.cpp`:
- Around line 223-235: Update simpler_log_emit_host_span to bound the encoded
span->name and span->attributes fields so each formatted STRACE record remains
within PIPE_BUF, including truncation before logging. Escape newline and
field-delimiter characters using the format expected by the trace parser, while
preserving the existing validation and metadata fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8861214-fbc6-497d-9951-312cbf8715f6
📒 Files selected for processing (17)
docs/dfx/host-trace.mdpython/bindings/CMakeLists.txtpython/bindings/task_interface.cpppython/simpler/task_interface.pypython/simpler/worker.pysimpler_setup/tools/README.mdsimpler_setup/tools/strace_timing.pysrc/common/hierarchical/host_trace.cppsrc/common/hierarchical/host_trace.hsrc/common/hierarchical/orchestrator.cppsrc/common/hierarchical/worker_manager.cppsrc/common/log/host_log.cppsrc/common/log/include/common/host_span.htests/ut/cpp/CMakeLists.txttests/ut/cpp/hierarchical/test_scheduler.cpptests/ut/py/test_strace_timing.pytests/ut/py/test_worker/test_host_worker.py
ec8a590 to
966d864
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
simpler_setup/tools/strace_timing.py (1)
425-436: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThread labels depend on which span is parsed first.
_host_thread_nameuses a single representative span per(pid, tid), chosen by parse order. If a thread emits anl3.*span without aroleattribute first, the fallback at Line 431 labels the laneworkereven when the thread is the facade or the scheduler.Consider selecting the representative from the first span that carries a
roleattribute, and falling back to parse order only when no span on that lane has a role.♻️ Proposed change
for pid, tid in host_threads: - representative = next(span for span in host_spans if span.pid == pid and span.tid == tid) + lane_spans = [span for span in host_spans if span.pid == pid and span.tid == tid] + representative = next( + (span for span in lane_spans if "role" in attrs_by_id[id(span)]), + lane_spans[0], + )Also applies to: 477-487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simpler_setup/tools/strace_timing.py` around lines 425 - 436, Update the representative-span selection for each (pid, tid) lane before calling _host_thread_name: prefer the first span carrying a role attribute, while retaining parse-order selection when no span has a role. Ensure facade and scheduler roles take precedence over inferred l3.* worker labels regardless of span parse order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@simpler_setup/runtime_compiler.py`:
- Around line 347-348: Add a narrow, documented S603 noqa to the subprocess.run
call in _strip, stating that strip_bin comes from the controlled toolchain
selection; no direct change is needed at simpler_setup/runtime_compiler.py lines
347-348 beyond this suppression. Add documented S603 and S607 noqa exceptions to
the fixed test compiler invocation in tests/ut/py/test_runtime_builder.py lines
631-635, noting its test-only toolchain dependency.
In `@src/common/log/include/common/host_span.h`:
- Around line 35-50: Update the test capture sink’s implementation file to
define SIMPLER_LOG_HOST_SPAN_IMPL before including common/host_span.h, so its
simpler_log_emit_host_span definition is emitted strong. Document that test
sinks must set this macro, consistent with the existing host_log.cpp
implementation contract.
---
Nitpick comments:
In `@simpler_setup/tools/strace_timing.py`:
- Around line 425-436: Update the representative-span selection for each (pid,
tid) lane before calling _host_thread_name: prefer the first span carrying a
role attribute, while retaining parse-order selection when no span has a role.
Ensure facade and scheduler roles take precedence over inferred l3.* worker
labels regardless of span parse order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6311d561-68de-49da-837e-5fdba48f82d0
📒 Files selected for processing (23)
.claude/rules/project-layout.mddocs/dfx/host-trace.mdpython/bindings/CMakeLists.txtpython/bindings/task_interface.cpppython/simpler/__init__.pypython/simpler/_log.pypython/simpler/_log_preload.pypython/simpler/task_interface.pypython/simpler/worker.pysimpler_setup/runtime_compiler.pysimpler_setup/tools/README.mdsimpler_setup/tools/strace_timing.pysrc/common/hierarchical/orchestrator.cppsrc/common/hierarchical/worker_manager.cppsrc/common/log/host_log.cppsrc/common/log/include/common/host_span.hsrc/common/log/include/common/host_span_scope.htests/ut/cpp/CMakeLists.txttests/ut/cpp/a5/test_host_log_off.cpptests/ut/cpp/hierarchical/test_scheduler.cpptests/ut/py/test_runtime_builder.pytests/ut/py/test_strace_timing.pytests/ut/py/test_worker/test_host_worker.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/common/hierarchical/orchestrator.cpp
- simpler_setup/tools/README.md
- tests/ut/py/test_strace_timing.py
- python/simpler/worker.py
- tests/ut/cpp/hierarchical/test_scheduler.cpp
- tests/ut/py/test_worker/test_host_worker.py
- src/common/hierarchical/worker_manager.cpp
- docs/dfx/host-trace.md
966d864 to
a8bec15
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
simpler_setup/tools/strace_timing.py (1)
462-487: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGroup the host spans once instead of rescanning per lane.
Line 466 rescans
host_spansfor every pid. Line 478 rescans it for every thread. Both are O(pids × spans) and O(threads × spans). A long capture contains many spans and many worker threads, so the cost grows quadratically.♻️ Proposed refactor to a single grouping pass
- host_pids = sorted({span.pid for span in host_spans}) - host_threads = sorted({(span.pid, span.tid) for span in host_spans}) + spans_by_pid: dict = {} + first_by_thread: dict = {} + for span in host_spans: + spans_by_pid.setdefault(span.pid, []).append(span) + first_by_thread.setdefault((span.pid, span.tid), span) + host_pids = sorted(spans_by_pid) + host_threads = sorted(first_by_thread) for pid in host_pids: - process_spans = [span for span in host_spans if span.pid == pid] + process_spans = spans_by_pid[pid] role = "host" if any(span.name.startswith("l3.") for span in process_spans) else "chip child" @@ for pid, tid in host_threads: - representative = next(span for span in host_spans if span.pid == pid and span.tid == tid) + representative = first_by_thread[(pid, tid)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simpler_setup/tools/strace_timing.py` around lines 462 - 487, Refactor the host metadata generation around host_pids and host_threads to build a single grouping of host_spans by (pid, tid), reusing it for both process and thread metadata. Derive each process’s spans and role from that grouped data, and obtain each thread representative without repeated scans of host_spans; preserve the existing event names and _host_thread_name behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@simpler_setup/tools/strace_timing.py`:
- Around line 503-520: Update the submit/dispatch correlation around the submits
and dispatches collections to retain all submit spans for each _flow_key rather
than overwriting repeated task-slot entries. For each dispatch, select the
latest matching submit whose start timestamp is at or before the dispatch
timestamp, and use that submit as the source while preserving the existing
unmatched-dispatch behavior.
- Around line 522-548: The task dispatch events in the dispatch-event
construction should use a numeric or valid hexadecimal flow id instead of the
descriptive string built as flow_id. Preserve that descriptive identifier by
adding it to the event args, and ensure both the start and finish events reuse
the same valid id.
In `@tests/ut/py/test_runtime_builder.py`:
- Around line 636-638: Move the noqa suppression for S607 from the
subprocess.run statement line to the diagnostic line containing the "gcc"
argument, or collapse the call so the suppression covers both diagnostics;
preserve the existing subprocess.run behavior and S603 suppression.
---
Nitpick comments:
In `@simpler_setup/tools/strace_timing.py`:
- Around line 462-487: Refactor the host metadata generation around host_pids
and host_threads to build a single grouping of host_spans by (pid, tid), reusing
it for both process and thread metadata. Derive each process’s spans and role
from that grouped data, and obtain each thread representative without repeated
scans of host_spans; preserve the existing event names and _host_thread_name
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dbf09d94-043f-4f91-be92-1016a6683cc2
📒 Files selected for processing (23)
.claude/rules/project-layout.mddocs/dfx/host-trace.mdpython/bindings/CMakeLists.txtpython/bindings/task_interface.cpppython/simpler/__init__.pypython/simpler/_log.pypython/simpler/_log_preload.pypython/simpler/task_interface.pypython/simpler/worker.pysimpler_setup/runtime_compiler.pysimpler_setup/tools/README.mdsimpler_setup/tools/strace_timing.pysrc/common/hierarchical/orchestrator.cppsrc/common/hierarchical/worker_manager.cppsrc/common/log/host_log.cppsrc/common/log/include/common/host_span.hsrc/common/log/include/common/host_span_scope.htests/ut/cpp/CMakeLists.txttests/ut/cpp/a5/test_host_log_off.cpptests/ut/cpp/hierarchical/test_scheduler.cpptests/ut/py/test_runtime_builder.pytests/ut/py/test_strace_timing.pytests/ut/py/test_worker/test_host_worker.py
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/ut/cpp/CMakeLists.txt
- python/simpler/init.py
- python/bindings/CMakeLists.txt
- python/simpler/worker.py
- src/common/hierarchical/orchestrator.cpp
- .claude/rules/project-layout.md
- src/common/log/include/common/host_span.h
- simpler_setup/tools/README.md
- python/simpler/_log_preload.py
- tests/ut/py/test_strace_timing.py
- tests/ut/py/test_worker/test_host_worker.py
- src/common/log/include/common/host_span_scope.h
- src/common/hierarchical/worker_manager.cpp
- tests/ut/cpp/a5/test_host_log_off.cpp
- docs/dfx/host-trace.md
- src/common/log/host_log.cpp
- tests/ut/cpp/hierarchical/test_scheduler.cpp
- python/bindings/task_interface.cpp
- python/simpler/task_interface.py
- python/simpler/_log.py
8376461 to
009b7c8
Compare
Re-review of
|
009b7c8 to
03d3aa0
Compare
|
@ChaoWao Addressed the review points in the updated single commit:
I left item 8 unchanged because all current attributes are numeric/enums; there is no free-text Validation: 1340 Python UT passed (6 skipped), 93 non-hardware C++ tests passed, pre-commit passed, and both one-device and two-device L3 a2a3sim scenes passed. For the performance question, local hardware precheck could not identify the silicon, so I did not bypass it. I compared merge-base |
03d3aa0 to
9e72937
Compare
Emit graph build, submit, dispatch, frame publication, activation, and completion spans through the process-global logger. Add a real-pid/tid Perfetto view with occurrence-matched submit-to-dispatch flows and numeric flow IDs while preserving every established marker family in the existing views, selecting repeated phases by timestamp, and keeping unaligned device timestamps off the host axis. Load the logger, seed the parent threshold before fork, and bind its C ABI entry point into the extension-local nullable sink slot, while keeping logger-free topologies quiet and free of unresolved link symbols. Replace rebuilt shared libraries by atomic rename so existing mappings remain valid. Capture dispatch metadata before endpoint publication can retire its slot, then format and emit outside the admission lock. Percent-encode and bound STRACE records so shared- pipe writes remain atomic. Cover the native scheduler stages, Python graph callback, runtime binary placement, parser conversion, and hostile marker fields with regression tests. Co-authored-by: Chao Wang <26245345+ChaoWao@users.noreply.github.com>
|
@ChaoWao Follow-up with an onboard L3 before/after measurement on the current head. Setup
The architecture precheck could not query Results
The order-balanced pooled median delta is +1.783 ms/run (+2.37%). With 64 child tasks per run, that is a rough end-to-end estimate of +27.9 us per child task. The PR logs also confirm the exact marker volume. In each 70-round PR process (20 warmup + 50 measured), they contain:
This independent/immediate-dispatch workload therefore emits 193 host-span records per run ( The means are more outlier-sensitive than the medians here, but both order directions show a positive median delta (+1.2% and +2.9%). So the default-on tracing cost is measurable on this L3 workload, although it is small relative to the full task latency. |
The host swimlane named a thread's lane from the first span that thread emitted. One OS thread emits several roles: the scheduler loop is the sole caller of both dispatch_ready() and manager->progress(), and within submit_dispatch the l3.frame_submit scope closes inside submit_progress, before the l3.dispatch record emitted after the admission lock is released. Its first span therefore always carried role=worker, so the one lane the swimlane exists to show was labelled "worker 0". Rank the roles a thread actually emitted instead, scheduler above worker. Seed the process logger in _start_hierarchical unconditionally. Gating on device_ids skipped exactly the pod processes: init() rejects device_ids above L3, so that branch cannot be taken in an L4 process — which still drives next-level Workers and still emits their spans. Setting the simpler logger to NUL could not silence them. A Worker with no chip binaries seeds the copy the package preloaded at import. Also: - Decode the emitter's percent-escapes in strace_timing, so a value that needed encoding reads back as its original text rather than as %XX. - Drop the whole trailing escape when the truncation marker lands on a full field, rather than its last byte: %0A became the undecodable %0~. - Carry parsed attributes alongside their span instead of in a dict keyed on id(), which was correct only while the span list stayed alive. - Scope the host-trace doc to every hierarchical worker rather than to those with local chip children, and record that the l3.* names cannot distinguish a pod from the L3s under it (hw-native-sys#1793) and that one process contributes at most two host lanes. Cover the scheduler lane naming with the real emission order, the chipless seeding, percent-decoding, the escape-boundary truncation, and l3.submit's emit site, whose attributes the dispatch flow arrows pair on.
2 participants
Summary
SIMPLER_HOST_STRACEand remains disabled for unsupported topologies.l3.graph_build,l3.submit,l3.dispatch,l3.frame_submit,l3.activate, andl3.completewith run, task-slot, worker, dispatch, and endpoint attributes.strace_timing.py --swimlaneto render real OS pid/tid lanes, process/thread labels, and submit-to-dispatch flow arrows without changing the established--trace-outview.clk=devtimestamps inunalignedDeviceSpansinstead of putting unrelated device and host clocks on one visible Chrome Trace axis, which avoids an empty multi-day Perfetto viewport without inventing a clock offset.Usage
Open
host_swimlane.jsonin Perfetto orchrome://tracing.Testing
pytest tests/ut/py/test_strace_timing.py tests/ut/py/test_worker/test_host_worker.py -q— 226 passedtest_schedulerwith host tracing enabled — 63 passedtest_schedulerbuilds withSIMPLER_HOST_STRACE=0pytest examples/workers/l3/child_memory -q --platform a2a3sim— 1 passedFixes #1708