perf(binary): drop the per-node box around NodeRef content

[jlucaso1]

Stacked on #1215, which is stacked on #1214. Review those first; the diff against #1215 is one type change plus its mechanical fallout.

Summary

NodeRef::content was Option<Box<NodeContentRef>>, so every node carrying any content allocated. That includes the commonest leaf in a fanout, an <enc> whose content is a Cow::Borrowed slice of the frame and allocates nothing itself: the box was the only allocation, and it existed to keep the field pointer-sized.

#1214 refused a visitor API on the grounds that the tree is nearly free, measuring one allocation and 3.9% on a 30-byte ack. That was right for the ack and wrong as a generalization: the same PR reported 45 allocations for a fanout and never used the number. A native profile of the fanout puts malloc, free and NodeRef's drop glue at 11.3%, and a fanout is not a corner case, it is what the send path builds for every group message, one child per device.

Measurement

Against #1215, perf stat -r 5 over 2M iterations, pinned to one core:

	base (#1215)	this PR	delta
fanout	2148.4 ms	1908.8 ms	-11.2%
large	366.6 ms	325.4 ms	-11.2%
ack	175.1 ms	164.9 ms	-5.8%
small	154.7 ms	144.8 ms	-6.4%

Allocations per decode, from AllocProfiler:

	before	after
fanout	45	27
large	50	26
ack	1	1 (no content)

Instructions on the fanout drop 9.5% under callgrind, which is the instrument CodSpeed uses, so CI should report the same order.

size_of::<NodeRef>() goes from 48 to 72 bytes. That is the cost this trades against, and it does not show up: every case improves, including small, which has no content and therefore gains nothing from the removed allocation. The gain tracks how many nodes carry content, which is the shape the change predicts.

What this does not cover. Native only. The 13.2% that motivated the proposal is wasm32, where dlmalloc is dearer, and #1214 already showed gains do not always cross targets.

Design

Three directions were on the table. This is the first and cheapest: the type change. Dropping the box does not create an infinite type, because the recursion runs through Box<[NodeRef]>, not through content. The other two (an arena per decode, and the visitor) both change what the caller receives, so they are not optimizations of the same thing; the arena in particular would need a lifetime the current unmarshal_ref signature cannot express.

This reverses a decision that was made once before, and that deserves saying plainly. A previous sweep considered inlining AttrsRef storage and rejected it, on the grounds that inlining would fatten every NodeRef and pay the tree back in memcpy. That reasoning is sound and it is the risk here too. Two things differ: that change was about 192 bytes per node, this one is 24; and the earlier rejection was reasoned, while this one is measured, in all four benchmark shapes, with none regressing. If the numbers had gone the other way for small, which is pure cost under this change, that would have settled it.

The box itself was never a considered decision. It arrived with NodeRef in the commit that created the zero-copy type, carries no comment, and nothing since has justified it.

Why this is not the visitor

#1212 refused a derivation cache in the core, and #1214 refused a visitor API. Both stay refused, and this PR needs neither: it adds no state, no bound, no policy, no API. unmarshal_ref returns the same tree with the same lifetimes; only a field's layout changed.

The visitor would come back only if this and an arena together left most of the 11.3% on the table. This takes the allocation count down by 40%, so that is not the situation.

Blast radius

content is a public field, so this reaches 22 files outside wacore/binary, in wacore/src/iq/*, wacore/src/stanza/* and src/. Every one of them is as_deref() becoming as_ref(); no logic moves and no signature changes. The proposal scoped itself to wacore/binary alone, which turned out not to be possible for this direction, and that is worth weighing.

NodeRef derives yoke::Yokeable, and wacore-binary owns the workspace's only load-bearing unsafe, so cargo miri test -p wacore-binary --lib was run against the new layout: 116 passed.

Validation

cargo fmt --all
cargo nextest run --profile ci -p wacore-binary                  # 132 passed
cargo nextest run --profile ci -p wacore-binary --features simd  # 132 passed
cargo nextest run --profile ci --workspace --exclude e2e-tests   # 4309 passed
cargo miri test -p wacore-binary --lib                           # 116 passed
cargo clippy --workspace --exclude e2e-tests --all-targets -- -D warnings
cargo bench -p wacore-binary --bench binary_benchmark

The roundtrip proptest passes unedited. Full matrix left to CI.

Follows #1211, #1212, #1213, #1214 and #1215.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 73abadd2-cacb-4261-963f-51dbe8d0628d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The PR removes the per-content allocation from borrowed binary nodes while preserving their decoded tree semantics.

• Changes NodeRef::content from Option<Box<NodeContentRef>> to Option<NodeContentRef>.
• Updates decoding, encoding, conversion, formatting, and protocol consumers to access the inline value directly.
• Keeps recursive child storage indirect through Box<[NodeRef]>.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
wacore/binary/src/node.rs	Inlines NodeContentRef within NodeRef and consistently updates constructors, conversions, accessors, serialization, and yoke-backed ownership.
wacore/binary/src/decoder.rs	Removes the content boxing step while retaining the same decoded content variants and indirect child-node recursion.
wacore/binary/src/encoder.rs	Replaces boxed-content dereferencing with direct borrowing without changing encoding behavior or traversal order.
wacore/binary/src/marshal.rs	Updates reservation and capacity estimation to inspect inline content while preserving existing thresholds and traversal.
wacore/src/xml.rs	Adapts borrowed content access without changing XML rendering behavior.

_{Reviews (2): Last reviewed commit: "perf(binary): drop the per-node box arou..." | Re-trigger Greptile}

[cubic-dev-ai]

No issues found across 24 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Requires human review: The change removes an allocation but alters the type of the public NodeRef::content field (Option<Box<...>> to Option<...>), a breaking API contract for downstream consumers. A human should decide whether that break is acceptable; the mechanical fallout doesn't remove that decision.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.