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:
📝 WalkthroughWalkthroughThe PR adds conditional host tracing for hierarchical execution, exposes host-span emission through Python bindings, instruments scheduler decision points, and adds a ChangesHost tracing and swimlane timeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant TaskInterface
participant HierarchicalScheduler
participant SimplerLog
participant StraceTiming
Worker->>TaskInterface: bind host-span sink
Worker->>HierarchicalScheduler: build graph and dispatch work
HierarchicalScheduler->>TaskInterface: emit host spans
TaskInterface->>SimplerLog: write STRACE records
StraceTiming->>SimplerLog: read STRACE records
StraceTiming-->>Worker: write host swimlane JSON
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
| if HOST_STRACE_ENABLED: | ||
| # The parent and every later fork inherit one RTLD_GLOBAL logger | ||
| # and the binding's resolved sink pointer. Loading it after the | ||
| # first fork would silently lose child-process host spans. | ||
| _initialize_simpler_log(binaries) | ||
| if not _bind_host_span_sink(): | ||
| raise RuntimeError("libsimpler_log.so does not export simpler_log_emit_host_span") | ||
| self._host_trace_enabled = True |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Initialize host tracing for L4 parents and disable it when the sink is absent.
An L4 worker has no direct device_ids, so this block does not bind the sink in the L4 parent. Its native scheduler then emits no l3.submit or l3.dispatch spans.
If _bind_host_span_sink() returns False, do not fail Worker.init(). The stated contract requires quiet disablement for unsupported topologies.
Move the logger preload and sink bind before all local forks for every hierarchical worker. Set _host_trace_enabled only when the bind succeeds.
🤖 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 `@python/simpler/worker.py` around lines 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.
| lk.unlock(); | ||
| #if SIMPLER_HOST_STRACE | ||
| const int64_t trace_end_ns = simpler::host_trace::now_ns(); | ||
| const RunId run_id = trace_run_id(ring_, d.task_slot); | ||
| const std::string attrs = trace_dispatch_attrs(ring_, d, endpoint_->caps(), "scheduler"); | ||
| simpler::host_trace::emit( | ||
| "l3.dispatch", run_id, trace_callable_hash(ring_, d.task_slot), 0, trace_start_ns, | ||
| trace_end_ns - trace_start_ns, attrs.c_str() | ||
| ); | ||
| #endif |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Capture dispatch trace metadata before notifying the worker.
Line 465 makes d available to the worker. The worker can complete and retire d.task_slot before Lines 469-473 read ring_. A reused slot can then produce the wrong run_id or callable hash in l3.dispatch.
Read the run ID, callable hash, and attributes before cv_.notify_one(). Emit the span after unlocking with the captured values.
Proposed fix
+ const RunId trace_run = trace_run_id(ring_, d.task_slot);
+ const uint64_t trace_hash = trace_callable_hash(ring_, d.task_slot);
+ const std::string trace_attrs = trace_dispatch_attrs(ring_, d, endpoint_->caps(), "scheduler");
cv_.notify_one();
lk.unlock();
`#if` SIMPLER_HOST_STRACE
const int64_t trace_end_ns = simpler::host_trace::now_ns();
- const RunId run_id = trace_run_id(ring_, d.task_slot);
- const std::string attrs = trace_dispatch_attrs(ring_, d, endpoint_->caps(), "scheduler");
simpler::host_trace::emit(
- "l3.dispatch", run_id, trace_callable_hash(ring_, d.task_slot), 0, trace_start_ns,
- trace_end_ns - trace_start_ns, attrs.c_str()
+ "l3.dispatch", trace_run, trace_hash, 0, trace_start_ns,
+ trace_end_ns - trace_start_ns, trace_attrs.c_str()
);
`#endif`🤖 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 `@src/common/hierarchical/worker_manager.cpp` around lines 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.
| extern "C" void simpler_log_emit_host_span(const SimplerHostSpan *span) { | ||
| if (span == nullptr || span->abi_version != SIMPLER_HOST_SPAN_ABI_VERSION || | ||
| span->struct_size < sizeof(SimplerHostSpan) || span->name == nullptr) { | ||
| return; | ||
| } | ||
| HostLogger::get_instance().log( | ||
| LogLevel::TIMING, "emit_host_span", | ||
| "[STRACE] v=1 pid=%d tid=%ld inv=%llu hid=%llx depth=%d name=%s ts=%lld dur=%lld %s", | ||
| static_cast<int>(getpid()), host_trace_tid(), static_cast<unsigned long long>(span->invocation_id), | ||
| static_cast<unsigned long long>(span->callable_hash), span->depth, span->name, | ||
| static_cast<long long>(span->timestamp_ns), static_cast<long long>(span->duration_ns), | ||
| span->attributes == nullptr ? "" : span->attributes | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bound host-span fields before writing the STRACE record.
Line 230 writes span->name and span->attributes without a length bound. _emit_host_span accepts arbitrary Python strings in python/bindings/task_interface.cpp Lines 972-981.
A large field forces the heap path and can require multiple write() calls. Forked child processes do not share HostLogger::mutex_. Their writes can interleave and corrupt a host-span record, which prevents --swimlane from parsing the affected span.
Limit the encoded name and attributes so every host-span record fits within PIPE_BUF. Escape line and field delimiters according to the trace parser format.
🤖 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 `@src/common/log/host_log.cpp` around lines 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.
49281b0 to
ec8a590
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 submit-to-dispatch flows while preserving the existing call-tree output. Keep unaligned device timestamps outside the visible host axis and cover single-frame, prepared-frame, and conversion paths.
1 participant
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