feat(hiroz): add a payload pool for reusable message allocations - #341
Open
YuanYuYuan wants to merge 13 commits into
Open
YuanYuYuan wants to merge 13 commits into
YuanYuYuan wants to merge 13 commits into
Conversation
This was referenced Sep 2, 2026
There was a problem hiding this comment.
🟡 Changes recommended
The re-entrancy test uses separate pools and retains the relevant mutex across publishing, so it does not validate the documented same-pool behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a fixed-capacity Arc<T> payload pool for allocation reuse on intra-process publishing paths.
Changes:
- Introduces pool acquisition, statistics, and stuck-slot detection.
- Exports the API publicly and through the prelude.
- Adds unit and publisher compatibility tests.
File summaries
| File | Description |
|---|---|
crates/hiroz/src/payload_pool.rs |
Implements the reusable payload pool. |
crates/hiroz/src/lib.rs |
Exposes the new module. |
crates/hiroz/src/prelude.rs |
Re-exports pool types. |
crates/hiroz-tests/tests/payload_pool_compat.rs |
Tests publisher and QoS integration. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
YuanYuYuan
force-pushed
the
feat/payload-pool
branch
from
September 2, 2026 07:51
1511f16 to
d1e9881
Compare
YuanYuYuan
force-pushed
the
feat/payload-pool
branch
2 times, most recently
from
September 2, 2026 09:29
fa0544f to
c1ec1f1
Compare
… path The intra-process bus removes serialization and transport; what remains per send is the allocation. A pool of pre-built Arc<T> removes that too. acquire() never allocates and returns None on exhaustion, so an invisible fallback cannot make a pool look like it is working. It tests Arc::get_mut rather than strong_count, because get_mut also excludes a live Weak: the count-then-unwrap shape panicked the sender when a subscriber held one. Slots held across many consecutive acquires are reported as stuck, which is the signature of a subscriber that stores its Arc. No blocking acquire: delivery is synchronous, so the party that frees a slot is often the caller itself.
explicit_auto_deref: the &** in Deref was unwarranted caution; auto-deref reaches &T from &mut Arc<T> on its own. rustfmt orders the module list, and local_bus was already out of place on this branch, so adding payload_pool made the existing drift a failure. The revert-detector script has served its purpose and is dropped: R1-R4 each fail their named test with zero compile errors (job 4304).
…d fan-out The unit tests establish the pool's arithmetic. They say nothing about whether a pooled message survives a publisher, a transport, a durability setting or a second subscriber, which is what an adopter hits first. Every test turns on one property: a slot returns when the last holder drops its Arc. So available-back-at-capacity after a publish is the evidence that nothing downstream retained the buffer. The wire path is safe because serialization writes a fresh ZBuf - a copy, not a splice of the payload's ZSlice - and that is now asserted rather than read off the source, because nothing in the type system would stop a future serializer from splicing instead. Also fixes a docs defect the audit found: into_shared claimed the result suited publish_owned. It does not. publish_owned takes T by value and gives the message away, which is the opposite of pooling.
create_node returns a builder with no Result, QosProfile is a struct literal rather than a builder, with_locality takes zenoh's Locality, and RosString::data is a plain String. Verified against existing call sites rather than left for the compiler.
Retaining the published Arc turns the wire test, and only the wire test, red - patch confirmed applied, zero compile errors. Without that the five green results would be consistent with an instrument that cannot fail.
Local stable rustfmt ignores imports_granularity as an unstable option; the worker's cargo fmt applies it. Matching the gate rather than the local tool.
cargo doc exits 0 over an unresolved link. A path in a //! doc resolves at the pub mod line, where the module's own types are not in scope.
The scan stops at the first free slot, so a released slot keeps its streak until the cursor reaches it. Counting exactly would mean sweeping every slot on every acquire, which is the cost the pool exists to avoid.
The module doc pointed at a hiroz feature that no longer exists, and the manifest kept its comment after the feature was removed. Rewriting bytes inside an existing ZBuf still needs zenoh's opt_mut_slice, but a caller reaches that through ZBuf's inner type, not through hiroz.
publish_shared returns Published. This is a separate test target, so no run since that change had compiled it — the tell was ran=0, not the exit code, because cargo returns 101 for a build failure and a test failure alike.
The callback locked a different pool from the one the publish came out of, and held the guard across the publish - so the documented case would have deadlocked rather than passed. Both sides now use one pool, and the guard is released before publishing.
The rewrite removed the separate outer pool but left an assertion on it. The exhaustion check belongs on the shared pool anyway - that is where a failure of into_shared to end the borrow would show.
YuanYuYuan
force-pushed
the
feat/payload-pool
branch
from
September 2, 2026 09:55
c1ec1f1 to
9a43a5b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the payload-pool design issue. Part of the intra-process meta issue.
Note
Stacked on #340, which adds the intra-process bus this pool feeds. The base is
feat/publisher-locality, so the diff here is the pool alone. #340 merges first.PayloadPoolgives hiroz the largest measured win on the intra-process branch, as a feature rather than as a pattern a user must reimplement.What this adds
It was thirty lines of benchmark code over one feature-gated accessor. It is now
crates/hiroz/src/payload_pool.rs, and the benchmark's hand-rolled ring is deleted rather than bypassed.The module names no zenoh type, so it builds against released zenoh. Only rewriting a
ZBuf's bytes in place still needs the unreleased accessor — and that no longer involves hiroz at all: #340 removes the wrapper, and a caller reaches zenoh'sopt_mut_slicethroughZBuf's inner type with its own[patch].acquirenever allocatesNone; an invisible fallback is the defect it exists to preventWeakcannot panic the senderArc::get_mutis the test, notstrong_countthenexpectArcis visiblePoolStats::stuck, warned once per slot rather than per send7 unit tests plus 5 compatibility tests. Every defect it prevents has a detector proven to fail without it, each compiling — a revert that does not build is indistinguishable from a firing detector otherwise:
strong_count+expecta_weak_reference_is_skipped_rather_than_panickinga_retained_arc_removes_a_slot_and_is_eventually_reportedexhaustion_returns_none_and_never_allocates(+2)a_slot_is_reused_rather_than_reallocatedIt composes with the rest of hiroz
payload_pool.rscarries 7 unit tests for the ring itself — acquisition, reuse, exhaustion, the stuck-slot report.payload_pool_compat.rsadds 5 for how it composes with publishers, QoS and fan-out, all green. Each concludes "nothing downstream retained the buffer" fromavailablereturning to capacity — and that is not vacuous: retaining the publishedArcreds the wire test and only the wire test.ZBuf, so the transport queues a copyTRANSIENT_LOCALArc<T>Locality::Remotepublish_sharedreturnsArcshared — fan-out costs one slot, not Ninto_sharedconsumes the guard, ending the borrow before the publishNot
publish_owned, which takesTby value and gives the message away — the opposite of pooling. (#342 renames itpublish_moved.)How this differs from the pool rmw_zenoh_cpp already has
rmw_zenoh_cpp::BufferPool(detail/zenoh_utils.hpp) pools the buffer that serialization writes into — the CDR destination, recycled with a deleter, mutex-guarded, capped at 8 MiB to stay cache-resident, used when SHM is unavailable.rmw_zenoh_cpp::BufferPoolThey are complementary rather than competing: one makes serializing cheaper, the other avoids serializing.
rmw_zenoh has no intra-process path of its own. The only mentions in its source set
message_info->from_intra_process = false. rclcpp's intra-process comms sit above the rmw and bypass it — which is what the benchmark shows:rclcpp_ipc13.0 µs against 26.1 µs for the same binary with the middleware back in the path.Does this need a patched zenoh?
The pool does not. The module names no zenoh type;
Arc::get_mutis std. Its seven unit tests pass with no features enabled.Only rewriting the bytes inside an existing
ZBufdoes, and that is azenoh-bufferspatch the caller supplies, not a hiroz feature. So a pooled message whose fields are plain — aString, a fixed array, aVecreplaced wholesale — works against released zenoh today.Measured both ways, on a pinned host, median of three interleaved runs:
Vec<u8>, no patchZBuf, with patchThe same win either way, so the patch is not what produces it. Control (an unchanged binary in the same sweep) moved 0.7–3.1%.
Warning
At 64 B pooling is a small loss, not a win: +17.3% p50 and +8.5% min, above the control. Pooling is for large payloads. The two pooled arms above must not be compared against each other — they differ in message type, container and write path at once, so the difference between them is unattributable.
Breaking changes
pooled-payloadfeature removed (in #340)zenoh-buffersand callopt_mut_sliceonZBuf's inner typeNothing else.
PayloadPoolis additive, and no hiroz code path calls the gated accessor.What fails without this
Nothing. No failing baseline: this adds a capability. The justification is the caller it serves — a publisher that reuses one buffer instead of allocating per send — and the −99% that reuse is worth at 1 MiB.
Evidence
Sweep F — the first sweep whose pooled arm publishes through
PayloadPoolrather than the benchmark's own ring. 70 cells, 3 interleaved runs, 0 failures, 0 fallbacks; controls drifted 0.9% median over 21 unchanged cells.hiroz(zc)hiroz(zc,pool)The public API is measurably slower than the private ring it replaces:
minrose 17–25% across all three payloads (0.234 → 0.288 µs), consistent in sign and size and well outside control drift. That is a real regression, stated rather than omitted. It buys theWeaksafety, the visible exhaustion and the stuck reporting above. The likely mechanism —Arc::get_mutreads the weak count too, and the pool keeps three parallel vectors where the ring kept one — is a hypothesis, not a measurement.Seven unit tests plus five compatibility tests. Every defect the pool prevents has a detector proven to fail without it, and every reverted build reported zero compile errors —
cargoexits 101 for a build failure and a test failure alike, so a revert that never compiled looks exactly like a detector that fired.