Found by branch review of conveyor-engine before merge (the in-flight XMILE conveyor+queue implementation; specs docs/design/conveyors.md + docs/design/queues.md). Filed from a review report that exceeded its 15-finding cap.
Summary
container_value (src/simlin-engine/src/conveyor_compile.rs:1861) duplicates container_value_from_slice (same file, :180) arm-for-arm. container_value_from_slice(&state.slat_contents(), kind) is bit-identical on every arm, so the duplicate is deletable and its sole caller publish_container_values should mirror the queue path's publish_queue_container_values.
Cost
The empty-container reducer conventions (Sum -> 0 via additive identity; Mean/Min/Max/Stddev -> NaN), the 1-based Slat(j) bounds rule, and the population-variance Stddev divisor N live in two copies (conveyor_compile.rs:180-209 and :1861-1891) that queues and conveyors read separately. A semantic fix to one silently forks the other.
The replacement is provably equivalent: contents() = slats.iter().map(content).sum() and slat_contents().iter().sum() fold the same sequence from 0.0 (bit-identical); belt_len() == slat_contents().len(); slat_content(j) == slat_contents().get(j).copied() (VecDeque logical order == iteration order); Mean/Min/Max/Stddev already run over state.slat_contents(). The Vec-allocation concern is neutral-to-favorable: the queue twin (queue_compile.rs:788-795) hoists one batch_contents() allocation per plan out of the per-container loop and skips empty container sets, which is fewer allocations than the current conveyor arm re-allocating slat_contents() per reducer container.
Evidence
- helpers on
ConveyorState: contents() (conveyor.rs:269), belt_len() (:275), slat_content (:293), slat_contents (:299)
- duplicate arm example:
container_value ContainerKind::Sum => state.contents() (conveyor_compile.rs:1871) and let slats = state.slat_contents(); (:1873) vs container_value_from_slice ContainerKind::Sum => vec.iter().sum() (:190) and the identical Mean/Min/Max/Stddev block (:191-204)
- sole caller:
curr[c.off] = container_value(state, &c.kind); (conveyor_compile.rs:1907)
- queue precedent:
let batches = state.batch_contents(); ... curr[c.off] = container_value_from_slice(&batches, &c.kind); (queue_compile.rs:792-794)
Suggested direction
Delete container_value and have publish_container_values call container_value_from_slice(&state.slat_contents(), kind), hoisting the slat_contents() allocation out of the per-container loop as the queue path already does.
Found by branch review of
conveyor-enginebefore merge (the in-flight XMILE conveyor+queue implementation; specsdocs/design/conveyors.md+docs/design/queues.md). Filed from a review report that exceeded its 15-finding cap.Summary
container_value(src/simlin-engine/src/conveyor_compile.rs:1861) duplicatescontainer_value_from_slice(same file,:180) arm-for-arm.container_value_from_slice(&state.slat_contents(), kind)is bit-identical on every arm, so the duplicate is deletable and its sole callerpublish_container_valuesshould mirror the queue path'spublish_queue_container_values.Cost
The empty-container reducer conventions (Sum -> 0 via additive identity; Mean/Min/Max/Stddev -> NaN), the 1-based
Slat(j)bounds rule, and the population-variance Stddev divisorNlive in two copies (conveyor_compile.rs:180-209and:1861-1891) that queues and conveyors read separately. A semantic fix to one silently forks the other.The replacement is provably equivalent:
contents() = slats.iter().map(content).sum()andslat_contents().iter().sum()fold the same sequence from0.0(bit-identical);belt_len() == slat_contents().len();slat_content(j) == slat_contents().get(j).copied()(VecDeque logical order == iteration order); Mean/Min/Max/Stddev already run overstate.slat_contents(). The Vec-allocation concern is neutral-to-favorable: the queue twin (queue_compile.rs:788-795) hoists onebatch_contents()allocation per plan out of the per-container loop and skips empty container sets, which is fewer allocations than the current conveyor arm re-allocatingslat_contents()per reducer container.Evidence
ConveyorState:contents()(conveyor.rs:269),belt_len()(:275),slat_content(:293),slat_contents(:299)container_valueContainerKind::Sum => state.contents()(conveyor_compile.rs:1871) andlet slats = state.slat_contents();(:1873) vscontainer_value_from_sliceContainerKind::Sum => vec.iter().sum()(:190) and the identical Mean/Min/Max/Stddev block (:191-204)curr[c.off] = container_value(state, &c.kind);(conveyor_compile.rs:1907)let batches = state.batch_contents(); ... curr[c.off] = container_value_from_slice(&batches, &c.kind);(queue_compile.rs:792-794)Suggested direction
Delete
container_valueand havepublish_container_valuescallcontainer_value_from_slice(&state.slat_contents(), kind), hoisting theslat_contents()allocation out of the per-container loop as the queue path already does.