perf!: replace Cow<str> with NodeStr for inline decoded strings - #514
Merged
Conversation
Introduce NodeStr<'a> = Borrowed(&'a str) | Owned(CompactString) to replace Cow<'a, str> in the borrowed decode model. Short owned strings (≤24 bytes) are stored inline via CompactString, eliminating heap allocations for packed phone numbers and protocol values. Rewrite read_packed to decode into a [u8; 254] stack buffer instead of a heap Vec, then materialize as CompactString. SIMD helpers now write directly into the stack buffer. Also removes dead code: JidRef::new, Jid::actual_agent.
📝 WalkthroughWalkthroughReplaces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/binary/src/node.rs (1)
551-556: 🧹 Nitpick | 🔵 TrivialMake
NodeRef::newacceptimpl Into<NodeStr<'a>>.You already added
From<&str>andFrom<CompactString>forNodeStr, but the constructor still forces callers to spellNodeStr::Borrowed(...)/NodeStr::Owned(...)manually. Acceptingimpl Into<NodeStr<'a>>keeps the public API ergonomic during the migration.✂️ Suggested API tweak
- pub fn new(tag: NodeStr<'a>, attrs: AttrsRef<'a>, content: Option<NodeContentRef<'a>>) -> Self { + pub fn new( + tag: impl Into<NodeStr<'a>>, + attrs: AttrsRef<'a>, + content: Option<NodeContentRef<'a>>, + ) -> Self { Self { - tag, + tag: tag.into(), attrs, content: content.map(Box::new), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/binary/src/node.rs` around lines 551 - 556, Change the NodeRef constructor to accept a flexible tag type by replacing the tag parameter type from NodeStr<'a> to impl Into<NodeStr<'a>> (i.e., update pub fn new(tag: NodeStr<'a>, ...) -> Self to pub fn new(tag: impl Into<NodeStr<'a>>, ... ) -> Self) and then call tag.into() when storing it (e.g., tag: tag.into()). This leverages your existing From<&str>/From<CompactString> implementations for NodeStr and makes NodeRef::new ergonomic without callers needing NodeStr::Borrowed/Owned; leave attrs and content handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/binary/src/attrs.rs`:
- Around line 13-15: Public field AttrParserRef::attrs leaks NodeStr through its
pub declaration causing downstream breaks; make the storage private (remove pub
from attrs) and provide a public accessor that projects the internal
representation back to the original Cow-based API (e.g., pub fn attrs(&self) ->
Cow<'a, [(NodeStr<'a>, ValueRef<'a>)]> or an iterator producing
(Cow<NodeStr<'a>>, ValueRef<'a>) pairs) so callers that previously read
parser.attrs continue to get Cow-backed values; update code that used direct
field access to call the new accessor; keep the internal type as the changed &'a
[(NodeStr<'a>, ValueRef<'a>)] but only expose it via the Cow-returning accessor
to preserve ABI/behavior.
In `@wacore/binary/src/node.rs`:
- Around line 7-13: Add a clone-preserving helper on NodeStr, e.g. pub fn
to_compact_string(&self) -> CompactString, that returns the inner CompactString
when NodeStr::Owned (cloning the CompactString) and builds a CompactString from
the &str when NodeStr::Borrowed; then update NodeRef::to_owned() and
JidRef::to_owned() to call NodeStr::to_compact_string() instead of
round-tripping via as_ref() so owned NodeStr values keep the fast path and avoid
reallocating heap strings.
---
Outside diff comments:
In `@wacore/binary/src/node.rs`:
- Around line 551-556: Change the NodeRef constructor to accept a flexible tag
type by replacing the tag parameter type from NodeStr<'a> to impl
Into<NodeStr<'a>> (i.e., update pub fn new(tag: NodeStr<'a>, ...) -> Self to pub
fn new(tag: impl Into<NodeStr<'a>>, ... ) -> Self) and then call tag.into() when
storing it (e.g., tag: tag.into()). This leverages your existing
From<&str>/From<CompactString> implementations for NodeStr and makes
NodeRef::new ergonomic without callers needing NodeStr::Borrowed/Owned; leave
attrs and content handling unchanged.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 3381b605-d6dd-4320-96cd-422020c1e542
📒 Files selected for processing (5)
wacore/binary/src/attrs.rswacore/binary/src/decoder.rswacore/binary/src/jid.rswacore/binary/src/lib.rswacore/binary/src/node.rs
Comment on lines
13
to
15
| pub struct AttrParserRef<'a> { | ||
| pub attrs: &'a [(Cow<'a, str>, ValueRef<'a>)], | ||
| pub attrs: &'a [(NodeStr<'a>, ValueRef<'a>)], | ||
| pub errors: Vec<BinaryError>, |
There was a problem hiding this comment.
Public AttrParserRef now leaks NodeStr through a pub field.
Changing attrs to &[(NodeStr<'a>, ValueRef<'a>)] is a downstream compile break for any caller that reads parser.attrs directly. The accessor methods still return Cow, but the struct’s public surface no longer does. If preserving the old API is important, this storage needs to stay internal or be projected back through a Cow-based accessor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/binary/src/attrs.rs` around lines 13 - 15, Public field
AttrParserRef::attrs leaks NodeStr through its pub declaration causing
downstream breaks; make the storage private (remove pub from attrs) and provide
a public accessor that projects the internal representation back to the original
Cow-based API (e.g., pub fn attrs(&self) -> Cow<'a, [(NodeStr<'a>,
ValueRef<'a>)]> or an iterator producing (Cow<NodeStr<'a>>, ValueRef<'a>) pairs)
so callers that previously read parser.attrs continue to get Cow-backed values;
update code that used direct field access to call the new accessor; keep the
internal type as the changed &'a [(NodeStr<'a>, ValueRef<'a>)] but only expose
it via the Cow-returning accessor to preserve ABI/behavior.
Comment on lines
+7
to
+13
| /// Borrowed-or-inline string for decoded nodes. Short owned values (≤24 bytes) | ||
| /// are stored inline via `CompactString`, avoiding heap allocation. | ||
| #[derive(Clone, yoke::Yokeable)] | ||
| pub enum NodeStr<'a> { | ||
| Borrowed(&'a str), | ||
| Owned(CompactString), | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a clone-preserving escape hatch on NodeStr.
Right now the new type only has conversions into NodeStr, so downstream owned conversions have to round-trip through as_ref() and rebuild a CompactString. A small helper like to_compact_string() would let NodeRef::to_owned() and JidRef::to_owned() preserve the already-owned fast path instead of copying heap-backed strings again.
♻️ Suggested helper
+impl NodeStr<'_> {
+ #[inline]
+ pub fn to_compact_string(&self) -> CompactString {
+ match self {
+ NodeStr::Borrowed(s) => CompactString::from(*s),
+ NodeStr::Owned(s) => s.clone(),
+ }
+ }
+}Also applies to: 15-38, 82-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/binary/src/node.rs` around lines 7 - 13, Add a clone-preserving helper
on NodeStr, e.g. pub fn to_compact_string(&self) -> CompactString, that returns
the inner CompactString when NodeStr::Owned (cloning the CompactString) and
builds a CompactString from the &str when NodeStr::Borrowed; then update
NodeRef::to_owned() and JidRef::to_owned() to call NodeStr::to_compact_string()
instead of round-tripping via as_ref() so owned NodeStr values keep the fast
path and avoid reallocating heap strings.
Benchmark Results2 improvement(s):
57 unchanged benchmark(s)
|
- Format JidRef directly into CompactString via write!(), eliminating the intermediate String allocation in read_value_as_string JID paths - Add NodeStr::to_compact_string() for clone-preserving conversion; use it in NodeRef::to_owned() and JidRef::to_owned() to avoid re-parsing owned CompactString values through as_ref() - Make AttrParserRef.attrs pub(crate) to avoid leaking NodeStr in the public field API
Use #[inline(always)] to ensure the discriminant match is eliminated at every call site in the encode hot path.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/binary/src/node.rs (1)
563-568: 🧹 Nitpick | 🔵 TrivialAlign
NodeRef::newsignature withNode::newfor API consistency.The constructor currently requires explicit
NodeStrwrapping despiteNodeStrimplementingFrom<&'a str>andFrom<CompactString>. SinceNode::newalready acceptsimpl Into<Cow<'static, str>>,NodeRef::newshould acceptimpl Into<NodeStr<'a>>to provide consistent ergonomics across the public API. This change is backward compatible—code passingNodeStrdirectly will continue to work.♻️ Proposed fix
- pub fn new(tag: NodeStr<'a>, attrs: AttrsRef<'a>, content: Option<NodeContentRef<'a>>) -> Self { + pub fn new( + tag: impl Into<NodeStr<'a>>, + attrs: AttrsRef<'a>, + content: Option<NodeContentRef<'a>>, + ) -> Self { Self { - tag, + tag: tag.into(), attrs, content: content.map(Box::new), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/binary/src/node.rs` around lines 563 - 568, Change the NodeRef::new constructor to accept a generic input that can be converted into NodeStr by changing the first parameter from NodeStr<'a> to impl Into<NodeStr<'a>> (keeping the other params AttrsRef<'a> and Option<NodeContentRef<'a>> unchanged); inside NodeRef::new call .into() on that parameter to obtain a NodeStr when building Self so callers can pass &str, CompactString, or NodeStr just like Node::new.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wacore/binary/src/node.rs`:
- Around line 563-568: Change the NodeRef::new constructor to accept a generic
input that can be converted into NodeStr by changing the first parameter from
NodeStr<'a> to impl Into<NodeStr<'a>> (keeping the other params AttrsRef<'a> and
Option<NodeContentRef<'a>> unchanged); inside NodeRef::new call .into() on that
parameter to obtain a NodeStr when building Self so callers can pass &str,
CompactString, or NodeStr just like Node::new.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7929167-acc3-4612-87f4-dcde9acfecda
📒 Files selected for processing (1)
wacore/binary/src/node.rs
This was referenced Apr 14, 2026
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
1 participant
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.
Summary
NodeStr<'a>enum (Borrowed(&'a str)|Owned(CompactString)) replacingCow<'a, str>in the borrowed decode model[u8; 254]stack buffer instead of heapVec<u8>, with SIMD helpers writing directly into the stack bufferCompactString— eliminates heap allocations for phone numbers and protocol attribute valuesJidRef::new,Jid::actual_agentWhy NodeStr instead of just stack buffer
A stack buffer alone doesn't reduce allocations —
String::from(stack_slice)still heap-allocates. The win comes fromCompactString's inline storage: phone numbers (10-15 chars), message IDs, timestamps all fit in the 24-byte inline buffer.Cow<'a, str>::OwnedholdsStringwhich always heap-allocates, so the type must change.What changed
NodeRef.tagCow<'a, str>NodeStr<'a>AttrsRefkeyCow<'a, str>NodeStr<'a>ValueRef::StringCow<'a, str>NodeStr<'a>NodeContentRef::StringCow<'a, str>NodeStr<'a>JidRef.userCow<'a, str>NodeStr<'a>read_packedreturnString(heap Vec)CompactString(stack decode)NodeStrimplementsDeref<Target=str>so existing code using.as_ref(),.parse(),== "value"works unchanged.AttrParserRefpublic API still returnsCow<'a, str>.Breaking changes
ValueRef::Stringinner type changedNodeContentRef::Stringinner type changedNodeRef.tagtype changedJidRef.usertype changedAttrsRefkey type changedJidRef::new()(unused — all sites use struct literal)Jid::actual_agent()(unused)Test plan
cargo fmt --allcargo clippy --all --testscargo test --all --exclude e2e-tests(all 619 tests pass)cargo check -p wacore-binary --all-featuresSummary by CodeRabbit
Refactor
Breaking Change