Change: decode a chip child's mailbox blob once per dispatch - #1755
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 removes blob-based tensor materialization. Chip and pipelined task paths now decode ChangesTask argument materialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TaskBlob
participant ChipWorker
participant ImportRegistry
participant materialize_task_args
TaskBlob->>ChipWorker: read_args_from_blob()
ChipWorker->>ImportRegistry: materialize_args(args)
ImportRegistry-->>ChipWorker: resolved identities and address spaces
ChipWorker->>materialize_task_args: build runtime arguments
materialize_task_args-->>ChipWorker: ChipStorageTaskArgs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bindings/task_interface.cpp`:
- Around line 2395-2401: Update the documentation at
python/bindings/task_interface.cpp:2395-2401 to describe materialize_task_args
as the sole TaskArgs-to-ChipStorageTaskArgs conversion path, without implying it
is the only way to construct ChipStorageTaskArgs. Update the mailbox description
at tests/st/a2a3/tensormap_and_ringbuffer/test_l3_tensor_dispatch.py:14-16 to
call the payload a TaskArgs blob and state that the child builds
ChipStorageTaskArgs.
🪄 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: ffd37bab-4fa3-4f42-bec3-a894a382db8b
📒 Files selected for processing (4)
python/bindings/task_interface.cpppython/simpler/buffer.pypython/simpler/worker.pytests/st/a2a3/tensormap_and_ringbuffer/test_l3_tensor_dispatch.py
3178779 to
ab85729
Compare
Both chip-child dispatch paths read the same mailbox bytes twice: once through `ImportRegistry.materialize_blob` to reach each tensor's descriptor and build the identity -> local-base map, then again through `materialize_tensor_blob` to produce the `ChipStorageTaskArgs` the runtime ABI reads. Each element therefore paid two 144 B copies and two full `validate_tensor` passes, and at the 256-arg ceiling that is 36 KiB of copying plus 256 validations per dispatch. The bytes now decode once, into the wire `TaskArgs`, and both the mapping pass and the POD build read that object -- the three-step shape the L2 leaf already uses. `read_args_from_blob` and `materialize_task_args` both predate this change, so no binding grows a parameter; `materialize_tensor_blob` is deleted and `materialize_blob` collapses into `materialize_args`, whose body it duplicated except for the leading decode. Validation coverage is unchanged: `TaskArgsView::tensors` is the sole gate on a blob element and the surviving decode runs it over the same elements. `materialize_task_args` is now the only path to the chip POD, so the `resolved` map's shape is documented on it rather than by reference to the deleted function.
2 participants
One decode per dispatch
Both chip-child dispatch paths read the same mailbox bytes twice: once through
ImportRegistry.materialize_blobto reach each tensor's descriptor and build the identity ->local-base map, then again through
materialize_tensor_blobto produce theChipStorageTaskArgsthe runtime ABI reads.Per tensor that removes one 144 B copy and one full
validate_tensor; at the 256-arg ceiling(
CHIP_MAX_TENSOR_ARGS) it is 36 KiB of copying plus 256 validations per dispatch.No binding grows a parameter.
read_args_from_blobandmaterialize_task_argsboth alreadyexisted — the L2-leaf path runs this exact three-step shape today.
materialize_tensor_blobisdeleted outright, and
materialize_blobcollapses intomaterialize_args, whose body itduplicated except for the leading decode. Two functions out, none in.
Validation coverage is unchanged.
TaskArgsView::tensorsis the sole gate on a blob element,and the surviving decode runs it over the same elements the deleted one did.
materialize_task_argsis now the only path to the chip POD, so theresolvedmap's shape isdocumented on it rather than by reference to a function that no longer exists.
Verification
pyut— 1304 passed, 6 skipped, identical to the base commit's own runa2a3simfull scene suite — rc=0, 0 failures, includingTestPostForkHostBufferZeroCopy(19.2 s), which exercises this pathtest_l3_tensor_dispatchon real a2a3 silicon — PASS, under atask-submitdevice lock. This is the end-to-end owner-writes-blob / chip-child-materializes case
a5simfull scene suite — running locally at push time; CI covers itper tensor, and the scene tests carry 3-5 tensors, so any delta would sit inside host
dispatch noise. The case for the change is one fewer decode path and two fewer functions;
a measurable win needs a large-arg-count workload, which none of the suites here is
The static check the change has to satisfy:
Context
Deferred from #1729 review item 6, and again by #1747 on the grounds that closing it would need
materialize_tensor_blobto take an already-parsed view — a signature change on a hot binding.That turned out not to be the shape of the fix: the parsed-view consumer is
materialize_task_args,which already exists, so the hot path moves to it and the re-reading function is simply deleted.