hbg: record Graph off the ring so the first invocation is one GRAPH task - #1731
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:
📝 WalkthroughWalkthroughChangesGraph recording flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant rt_submit_graph_impl
participant PTO2OrchestratorState
participant DeviceScheduler
Caller->>rt_submit_graph_impl: invoke graph body
rt_submit_graph_impl->>PTO2OrchestratorState: record shadow nodes
PTO2OrchestratorState-->>rt_submit_graph_impl: emit or report unsupported
alt Recording succeeds
rt_submit_graph_impl->>PTO2OrchestratorState: submit outer GRAPH task
PTO2OrchestratorState->>DeviceScheduler: dispatch internal nodes
else Recording is unsupported
rt_submit_graph_impl->>PTO2OrchestratorState: rerun body through ordinary submission
else Cached definition exists
rt_submit_graph_impl->>PTO2OrchestratorState: submit cached GRAPH task
PTO2OrchestratorState->>DeviceScheduler: dispatch internal nodes
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.md`:
- Around line 7-18: Add or update the a5 host build graph GRAPH_EXECUTION.md to
match the a2a3 copy byte-for-byte, including the invocation, Definition caching,
scheduler expansion, and unsupported-construct fallback behavior described in
the diff. Preserve identical content between both documentation files.
In
`@src/a2a3/runtime/host_build_graph/runtime/orchestrator_core/pto_orchestrator.cpp`:
- Around line 1482-1484: Update the recording path in the node construction flow
around logical_block_num and total_required_subtasks to validate block_num is
positive and the computed subtask count fits the int16_t range before assigning
or casting it. For invalid inputs, mark recording unsupported so graph_end falls
back to the ordinary path, preserving prepare_task’s PTO2_ERROR_INVALID_ARGS
behavior.
In `@src/a2a3/runtime/host_build_graph/runtime/pto_ring_buffer.h`:
- Around line 253-272: Update reserve_heap_scratch to perform only a forward,
non-wrapping heap bump: return nullptr when the aligned request exceeds the
remaining space from heap_top_ to heap_size_, and leave allocator state
unchanged in that case. Do not route scratch reservations through the wrapping
behavior of try_bump_heap; preserve the zero-size behavior and keep
restore_heap_top compatible with successful reservations.
🪄 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: 723ccbef-4134-46d8-91c3-bb5f5a271df9
📒 Files selected for processing (7)
src/a2a3/runtime/host_build_graph/docs/GRAPH_EXECUTION.mdsrc/a2a3/runtime/host_build_graph/orchestration/pto_orchestration_api.hsrc/a2a3/runtime/host_build_graph/runtime/orchestrator_core/pto_orchestrator.cppsrc/a2a3/runtime/host_build_graph/runtime/orchestrator_core/pto_runtime2.cppsrc/a2a3/runtime/host_build_graph/runtime/pto_orchestrator.hsrc/a2a3/runtime/host_build_graph/runtime/pto_ring_buffer.hsrc/a2a3/runtime/host_build_graph/runtime/pto_runtime2.h
2111315 to
d9bb5ba
Compare
Fixes hw-native-sys#1713 On a cache miss the host_build_graph recording pass used to run the Graph body through the ordinary submit path: every internal task took a ring task-window slot, a ring-heap allocation, and (on replay) an upload, while being recorded. Only cache hits got the "one outer GRAPH slot, internal nodes off the ring, nothing uploaded" benefit, so the first layer of every per-run decode loop re-paid the full N-slot ring cost because the Definition cache starts empty each run. The recording pass now records the layout off the ring and executes the first invocation as a Graph, matching the replay path: - submit_task / submit_dummy_task / alloc_tensors route to a new graph_record_submit_node during recording. It builds each node's metadata, materialized outputs, and scalar provenance exactly as the ordinary submit path would (tensor-source classification for fanins, graph_classify_scalar for static-vs-boundary scalars), but reserves output buffers from heap scratch (PTO2TaskAllocator::reserve_heap_scratch, released in graph_end) instead of claiming a task-window slot. No ring slot, tensormap entry, fanin-pool entry, or upload is produced for an internal node. It also mirrors prepare_task's block_num / subtask-count contract: an out-of-contract value marks the recording unsupported so graph_end falls back rather than baking a truncated count into the Definition. - reserve_heap_scratch is strictly forward: it never takes try_bump_heap's wrap branches (which mutate heap_tail_ / the rebase anchor that restore_heap_top cannot undo) and returns nullptr on exhaustion, which the recorder treats as unsupported. - A Graph replays as a flat DAG, so a scope inside a Graph body is a no-op during recording and does not touch the ring scope stack. - graph_end rolls the heap back to the recording watermark, compacts the Definition, caches it, and emits the single outer GRAPH task via graph_submit_definition. It returns whether the outer task was emitted; when the recording is unsupported or the outer task cannot be placed, the body re-runs on the ordinary path so its work is still submitted. The first invocation now occupies one ring slot and one heap block instead of one per internal node. a2a3-only (Graph Execution is not in the a5 tree). Verified: full a2a3sim host_build_graph suite; the test_graph_cache C++ unit test; and all graph_execution scenes onboard on a2a3, including the dynamic boundary-scalar case and the manual qwen3-14B three-layer decode whose layer body uses manual scopes, explicit dependencies, and ScratchArena boundary views.
…RAPH task Ports a2a3 host_build_graph PR hw-native-sys#1731 to A5. Mirrors hw-native-sys#1731 line-for-line. With hw-native-sys#1777 in place, the scalar-source machinery hw-native-sys#1731's recording path depends on now exists on A5, so the port is direct: - PTO2TaskAllocator gains reserve_heap_scratch / restore_heap_top. - The recording pass routes submit_task / submit_dummy_task / alloc_tensors to graph_record_submit_node, which records each node's layout off the ring — output buffers come from heap scratch (released in graph_end) instead of a task-window slot. No ring slot, tensormap, fanin-pool entry, or upload is produced for an internal node. - A scope inside a Graph body is a no-op during recording. - graph_end rolls the heap back to the recording watermark, compacts and caches the Definition, and emits the single outer GRAPH task. It now returns bool; when the outer task cannot be placed, the body re-runs on the ordinary path so its work is still submitted. - The graph_end ops-table entry changes void -> bool in both mirrored ops structs. The first invocation now occupies one ring slot and one heap block instead of one per internal node. Verified: full no-hardware C++ unit suite (92/92) and the A5 graph_execution a5sim scenes (3/3).
…RAPH task (#1780) Ports a2a3 host_build_graph PR #1731 to A5. Mirrors #1731 line-for-line. With #1777 in place, the scalar-source machinery #1731's recording path depends on now exists on A5, so the port is direct: - PTO2TaskAllocator gains reserve_heap_scratch / restore_heap_top. - The recording pass routes submit_task / submit_dummy_task / alloc_tensors to graph_record_submit_node, which records each node's layout off the ring — output buffers come from heap scratch (released in graph_end) instead of a task-window slot. No ring slot, tensormap, fanin-pool entry, or upload is produced for an internal node. - A scope inside a Graph body is a no-op during recording. - graph_end rolls the heap back to the recording watermark, compacts and caches the Definition, and emits the single outer GRAPH task. It now returns bool; when the outer task cannot be placed, the body re-runs on the ordinary path so its work is still submitted. - The graph_end ops-table entry changes void -> bool in both mirrored ops structs. The first invocation now occupies one ring slot and one heap block instead of one per internal node. Verified: full no-hardware C++ unit suite (92/92) and the A5 graph_execution a5sim scenes (3/3).
Fixes #1713.
Problem
On a cache miss, the
host_build_graphrecording pass ran the Graph body through the ordinary submit path: every internal task took a ring task-window slot and a ring-heap allocation while being recorded. Only cache hits got the "one outer GRAPH slot, internal nodes off the ring, nothing uploaded" benefit. Because the Definition cache is per-run (starts empty each run), the recording layer was re-paid every run — for a repeated decoder loop, layer 0 of each run pushed the full N internal tasks through the ring while later layers replayed.Change (a2a3-only; Graph Execution is not in the a5 tree)
The recording pass now records the layout off the ring and executes the first invocation as a Graph, matching the replay path:
submit_task/submit_dummy_task/alloc_tensorsroute to a newgraph_record_submit_nodeduring recording. It builds each node's metadata and materialized outputs exactly asPTO2TaskPayload::initwould, but reserves output buffers from heap scratch (PTO2TaskAllocator::reserve_heap_scratch, released ingraph_end) instead of claiming a task-window slot, and derives internal fanins from tensor-source classification plus explicit dependencies. No ring slot, tensormap entry, fanin-pool entry, or upload is produced for an internal node.graph_endrolls the heap back to the recording watermark, compacts the Definition, caches it, and emits the single outer GRAPH task viagraph_submit_definition. It now returns whether the outer task was emitted; when the recording is unsupported or the outer task cannot be placed, the body re-runs on the ordinary path so its work is still submitted (thegraph_endops-table entry changes fromvoidtoboolin both mirrored ops structs).The first invocation now occupies one ring slot and one heap block instead of one per internal node.
Verification
a2a3simhost_build_graphsuite: 20 passed (non-graph paths are unchanged — every new branch guards on an in-progress recording).tests/st/a2a3/host_build_graph/graph_executionscenes pass onboard on a2a3, including the manualqwen3-14Bthree-layer decode, whose layer body usesMANUALscopes, explicitset_dependencies, andScratchArenaboundary-view intermediates — exercising the exact constructs the shadow-record path must reproduce.🤖 Generated with Claude Code