Skip to content

perf!: replace Cow<str> with NodeStr for inline decoded strings #514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jlucaso1 merged 3 commits into from
Apr 12, 2026
Merged

perf!: replace Cow<str> with NodeStr for inline decoded strings #514

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
579fbb4
perf!: replace Cow<str> with NodeStr for inline decoded strings
jlucaso1 Apr 12, 2026
c17b1d8
perf: avoid intermediate String in JID-to-string decode + review fixes
jlucaso1 Apr 12, 2026
9390ba6
perf: force-inline NodeStr::Deref and AsRef
jlucaso1 Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions wacore/binary/src/attrs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ use std::str::FromStr;

use crate::error::{BinaryError, Result};
use crate::jid::Jid;
use crate::node::{Attrs, Node, NodeRef, NodeValue, ValueRef};
use crate::node::{Attrs, Node, NodeRef, NodeStr, NodeValue, ValueRef};

pub struct AttrParser<'a> {
pub attrs: &'a Attrs,
pub errors: Vec<BinaryError>,
}

pub struct AttrParserRef<'a> {
pub attrs: &'a [(Cow<'a, str>, ValueRef<'a>)],
pub attrs: &'a [(NodeStr<'a>, ValueRef<'a>)],
pub errors: Vec<BinaryError>,
Comment on lines 13 to 15

@coderabbitai coderabbitai Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}

Expand All @@ -36,11 +36,7 @@ impl<'a> AttrParserRef<'a> {
}

fn get_raw(&mut self, key: &str, require: bool) -> Option<&'a ValueRef<'a>> {
let val = self
.attrs
.iter()
.find(|(k, _)| k.as_ref() == key)
.map(|(_, v)| v);
let val = self.attrs.iter().find(|(k, _)| **k == *key).map(|(_, v)| v);

if require && val.is_none() {
self.errors.push(BinaryError::AttrParse(format!(
Expand Down
116 changes: 64 additions & 52 deletions wacore/binary/src/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use crate::error::{BinaryError, Result};
use crate::jid::JidRef;
use crate::node::{AttrsRef, NodeContentRef, NodeRef, NodeVec, ValueRef};
use crate::node::{AttrsRef, NodeContentRef, NodeRef, NodeStr, NodeVec, ValueRef};
use crate::token;
use compact_str::CompactString;
use std::borrow::Cow;
#[cfg(feature = "simd")]
use std::simd::{Simd, prelude::*, u8x16};
Expand Down Expand Up @@ -88,10 +89,10 @@ impl<'a> Decoder<'a> {
}

#[inline(always)]
fn read_string(&mut self, len: usize) -> Result<Cow<'a, str>> {
fn read_string(&mut self, len: usize) -> Result<NodeStr<'a>> {
let bytes = self.read_bytes(len)?;
match std::str::from_utf8(bytes) {
Ok(s) => Ok(Cow::Borrowed(s)),
Ok(s) => Ok(NodeStr::Borrowed(s)),
Err(e) => Err(BinaryError::InvalidUtf8(e)),
}
}
Expand All @@ -107,9 +108,8 @@ impl<'a> Decoder<'a> {
}

fn read_jid_pair(&mut self) -> Result<JidRef<'a>> {
let user_val = self.read_value_as_string()?;
let server_str = self.read_value_as_string()?.unwrap_or(Cow::Borrowed(""));
let user = user_val.unwrap_or(Cow::Borrowed(""));
let user = self.read_value_as_string()?.unwrap_or_default();
let server_str = self.read_value_as_string()?.unwrap_or_default();
let server = crate::jid::Server::try_from(server_str.as_ref()).map_err(|_| {
BinaryError::AttrParse(format!("JID_PAIR unknown server: {}", server_str))
})?;
Expand All @@ -125,7 +125,7 @@ impl<'a> Decoder<'a> {
fn read_ad_jid(&mut self) -> Result<JidRef<'a>> {
let agent = self.read_u8()?;
let device = self.read_u8()? as u16;
let user = self
let user: NodeStr<'a> = self
.read_value_as_string()?
.ok_or(BinaryError::InvalidNode)?;

Expand Down Expand Up @@ -157,7 +157,7 @@ impl<'a> Decoder<'a> {
.ok_or(BinaryError::InvalidNode)?;
let device = self.read_u16_be()?;
let integrator = self.read_u16_be()?;
let server_str = self.read_value_as_string()?.unwrap_or(Cow::Borrowed(""));
let server_str = self.read_value_as_string()?.unwrap_or_default();
if server_str.as_ref() != crate::jid::INTEROP_SERVER {
return Err(BinaryError::InvalidNode);
}
Expand All @@ -175,7 +175,7 @@ impl<'a> Decoder<'a> {
.read_value_as_string()?
.ok_or(BinaryError::InvalidNode)?;
let device = self.read_u16_be()?;
let server_str = self.read_value_as_string()?.unwrap_or(Cow::Borrowed(""));
let server_str = self.read_value_as_string()?.unwrap_or_default();
if server_str.as_ref() != crate::jid::MESSENGER_SERVER {
return Err(BinaryError::InvalidNode);
}
Expand All @@ -188,13 +188,13 @@ impl<'a> Decoder<'a> {
})
}

fn read_value_as_string(&mut self) -> Result<Option<Cow<'a, str>>> {
fn read_value_as_string(&mut self) -> Result<Option<NodeStr<'a>>> {
let tag = self.read_u8()?;
self.read_value_as_string_from_tag(tag)
}

#[inline(always)]
fn read_value_as_string_from_tag(&mut self, tag: u8) -> Result<Option<Cow<'a, str>>> {
fn read_value_as_string_from_tag(&mut self, tag: u8) -> Result<Option<NodeStr<'a>>> {
match tag {
token::LIST_EMPTY => Ok(None),
token::BINARY_8 => {
Expand All @@ -211,27 +211,31 @@ impl<'a> Decoder<'a> {
}
token::JID_PAIR => self
.read_jid_pair()
.map(|j| Some(Cow::Owned(j.to_string()))),
token::AD_JID => self.read_ad_jid().map(|j| Some(Cow::Owned(j.to_string()))),
.map(|j| Some(NodeStr::Owned(CompactString::from(j.to_string())))),
token::AD_JID => self
.read_ad_jid()
.map(|j| Some(NodeStr::Owned(CompactString::from(j.to_string())))),
token::INTEROP_JID => self
.read_interop_jid()
.map(|j| Some(Cow::Owned(j.to_string()))),
token::FB_JID => self.read_fb_jid().map(|j| Some(Cow::Owned(j.to_string()))),
token::NIBBLE_8 | token::HEX_8 => self.read_packed(tag).map(|s| Some(Cow::Owned(s))),
.map(|j| Some(NodeStr::Owned(CompactString::from(j.to_string())))),
token::FB_JID => self
.read_fb_jid()
.map(|j| Some(NodeStr::Owned(CompactString::from(j.to_string())))),
token::NIBBLE_8 | token::HEX_8 => {
self.read_packed(tag).map(|s| Some(NodeStr::Owned(s)))
}
tag @ token::DICTIONARY_0..=token::DICTIONARY_3 => {
let index = self.read_u8()?;
token::get_double_token(tag - token::DICTIONARY_0, index)
.map(|s| Some(Cow::Borrowed(s)))
.map(|s| Some(NodeStr::Borrowed(s)))
.ok_or(BinaryError::InvalidToken(tag))
}
_ => token::get_single_token(tag)
.map(|s| Some(Cow::Borrowed(s)))
.map(|s| Some(NodeStr::Borrowed(s)))
.ok_or(BinaryError::InvalidToken(tag)),
}
}

/// Read a value that can be either a string or a JID.
/// This avoids string allocation for JID tokens by returning the JidRef directly.
fn read_value(&mut self) -> Result<Option<ValueRef<'a>>> {
let tag = self.read_u8()?;
match tag {
Expand All @@ -248,87 +252,92 @@ impl<'a> Decoder<'a> {
let size = self.read_u32_be()? as usize;
self.read_string(size).map(|s| Some(ValueRef::String(s)))
}
// JID tokens - return JidRef directly without string allocation
token::JID_PAIR => self.read_jid_pair().map(|j| Some(ValueRef::Jid(j))),
token::AD_JID => self.read_ad_jid().map(|j| Some(ValueRef::Jid(j))),
token::INTEROP_JID => self.read_interop_jid().map(|j| Some(ValueRef::Jid(j))),
token::FB_JID => self.read_fb_jid().map(|j| Some(ValueRef::Jid(j))),
token::NIBBLE_8 | token::HEX_8 => self
.read_packed(tag)
.map(|s| Some(ValueRef::String(Cow::Owned(s)))),
.map(|s| Some(ValueRef::String(NodeStr::Owned(s)))),
tag @ token::DICTIONARY_0..=token::DICTIONARY_3 => {
let index = self.read_u8()?;
token::get_double_token(tag - token::DICTIONARY_0, index)
.map(|s| Some(ValueRef::String(Cow::Borrowed(s))))
.map(|s| Some(ValueRef::String(NodeStr::Borrowed(s))))
.ok_or(BinaryError::InvalidToken(tag))
}
_ => token::get_single_token(tag)
.map(|s| Some(ValueRef::String(Cow::Borrowed(s))))
.map(|s| Some(ValueRef::String(NodeStr::Borrowed(s))))
.ok_or(BinaryError::InvalidToken(tag)),
}
}

fn read_packed(&mut self, tag: u8) -> Result<String> {
/// Decode packed nibble/hex into a stack buffer, then create CompactString.
/// Max unpacked length is 254 bytes (127 packed × 2), so the stack buffer
/// is always sufficient. Short values (≤24 bytes) are stored inline.
fn read_packed(&mut self, tag: u8) -> Result<CompactString> {
let packed_len_byte = self.read_u8()?;
let is_half_byte = (packed_len_byte & 0x80) != 0;
let len = (packed_len_byte & 0x7F) as usize;

if len == 0 {
return Ok(String::new());
return Ok(CompactString::default());
}

let raw_len = if is_half_byte { (len * 2) - 1 } else { len * 2 };
let packed_data = self.read_bytes(len)?;
let mut unpacked_bytes = Vec::with_capacity(raw_len);
let mut buf = [0u8; 254];
let mut pos = 0;

match tag {
token::HEX_8 => Self::decode_packed_hex(packed_data, &mut unpacked_bytes),
token::NIBBLE_8 => Self::decode_packed_nibble(packed_data, &mut unpacked_bytes)?,
token::HEX_8 => Self::decode_packed_hex(packed_data, &mut buf, &mut pos),
token::NIBBLE_8 => Self::decode_packed_nibble(packed_data, &mut buf, &mut pos)?,
_ => return Err(BinaryError::InvalidToken(tag)),
}

if is_half_byte {
unpacked_bytes.pop();
if is_half_byte && pos > 0 {
pos -= 1;
}

// Lookup tables produce only ASCII bytes ('0'..'9', 'A'..'F', '-', '.', '\0'),
// so from_utf8 will never fail. Use the safe version to avoid unsafe.
Ok(String::from_utf8(unpacked_bytes).expect("packed decode produced non-ASCII"))
// All output bytes are ASCII, so from_utf8 cannot fail.
let s = std::str::from_utf8(&buf[..pos]).expect("packed decode produced non-ASCII");
Ok(CompactString::from(s))
}

#[inline]
fn decode_packed_hex(packed_data: &[u8], unpacked_bytes: &mut Vec<u8>) {
fn decode_packed_hex(packed_data: &[u8], out: &mut [u8], pos: &mut usize) {
#[cfg(feature = "simd")]
let packed_data = {
const HEX_LOOKUP: [u8; 16] = *b"0123456789ABCDEF";
let lookup_table = Simd::from_array(HEX_LOOKUP);
let low_mask = Simd::splat(0x0F);

let (chunks, remainder) = packed_data.as_chunks::<16>();
unpacked_bytes.reserve(chunks.len() * 32);
for chunk in chunks {
let data = u8x16::from_array(*chunk);
let high_nibbles = (data >> 4) & low_mask;
let low_nibbles = data & low_mask;
let high_chars = lookup_table.swizzle_dyn(high_nibbles);
let low_chars = lookup_table.swizzle_dyn(low_nibbles);
let (lo, hi) = Simd::interleave(high_chars, low_chars);
unpacked_bytes.extend_from_slice(lo.as_array());
unpacked_bytes.extend_from_slice(hi.as_array());
out[*pos..*pos + 16].copy_from_slice(lo.as_array());
*pos += 16;
out[*pos..*pos + 16].copy_from_slice(hi.as_array());
*pos += 16;
}
remainder
};

for &byte in packed_data {
let high = (byte & 0xF0) >> 4;
let low = byte & 0x0F;
unpacked_bytes.push(Self::unpack_hex(high));
unpacked_bytes.push(Self::unpack_hex(low));
out[*pos] = Self::unpack_hex(high);
*pos += 1;
out[*pos] = Self::unpack_hex(low);
*pos += 1;
}
}

#[inline]
fn decode_packed_nibble(packed_data: &[u8], unpacked_bytes: &mut Vec<u8>) -> Result<()> {
fn decode_packed_nibble(packed_data: &[u8], out: &mut [u8], pos: &mut usize) -> Result<()> {
#[cfg(feature = "simd")]
let packed_data = {
const NIBBLE_LOOKUP: [u8; 16] = *b"0123456789-.\x00\x00\x00\x00";
Expand All @@ -338,7 +347,6 @@ impl<'a> Decoder<'a> {
let f15 = Simd::splat(15);

let (chunks, remainder) = packed_data.as_chunks::<16>();
unpacked_bytes.reserve(chunks.len() * 32);
for chunk in chunks {
let data = u8x16::from_array(*chunk);

Expand All @@ -348,7 +356,6 @@ impl<'a> Decoder<'a> {
let hi_valid = high_nibbles.simd_le(le11) | high_nibbles.simd_eq(f15);
let lo_valid = low_nibbles.simd_le(le11) | low_nibbles.simd_eq(f15);
if !(hi_valid & lo_valid).all() {
// Validate first, then decode scalar as a conservative fallback.
for byte in *chunk {
let high = (byte & 0xF0) >> 4;
let low = byte & 0x0F;
Expand All @@ -358,26 +365,32 @@ impl<'a> Decoder<'a> {
for byte in *chunk {
let high = (byte & 0xF0) >> 4;
let low = byte & 0x0F;
unpacked_bytes.push(Self::unpack_nibble(high)?);
unpacked_bytes.push(Self::unpack_nibble(low)?);
out[*pos] = Self::unpack_nibble(high)?;
*pos += 1;
out[*pos] = Self::unpack_nibble(low)?;
*pos += 1;
}
continue;
}

let high_chars = lookup_table.swizzle_dyn(high_nibbles);
let low_chars = lookup_table.swizzle_dyn(low_nibbles);
let (lo, hi) = Simd::interleave(high_chars, low_chars);
unpacked_bytes.extend_from_slice(lo.as_array());
unpacked_bytes.extend_from_slice(hi.as_array());
out[*pos..*pos + 16].copy_from_slice(lo.as_array());
*pos += 16;
out[*pos..*pos + 16].copy_from_slice(hi.as_array());
*pos += 16;
}
remainder
};

for &byte in packed_data {
let high = (byte & 0xF0) >> 4;
let low = byte & 0x0F;
unpacked_bytes.push(Self::unpack_nibble(high)?);
unpacked_bytes.push(Self::unpack_nibble(low)?);
out[*pos] = Self::unpack_nibble(high)?;
*pos += 1;
out[*pos] = Self::unpack_nibble(low)?;
*pos += 1;
}

Ok(())
Expand Down Expand Up @@ -409,10 +422,9 @@ impl<'a> Decoder<'a> {
let key = self
.read_value_as_string()?
.ok_or(BinaryError::NonStringKey)?;
// Use read_value to get ValueRef - avoids string allocation for JIDs
let value = self
.read_value()?
.unwrap_or(ValueRef::String(Cow::Borrowed("")));
.unwrap_or(ValueRef::String(NodeStr::Borrowed("")));
attrs.push((key, value));
}
Ok(attrs)
Expand Down
21 changes: 2 additions & 19 deletions wacore/binary/src/jid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::node::NodeStr;
use compact_str::CompactString;
use std::borrow::Cow;
use std::fmt;
use std::str::FromStr;

Expand Down Expand Up @@ -374,7 +374,7 @@ pub struct Jid {

#[derive(Debug, Clone, PartialEq, Eq, Hash, yoke::Yokeable)]
pub struct JidRef<'a> {
pub user: Cow<'a, str>,
pub user: NodeStr<'a>,
pub server: Server,
pub agent: u8,
pub device: u16,
Expand Down Expand Up @@ -505,13 +505,6 @@ impl Jid {
}
}

pub fn actual_agent(&self) -> u8 {
match self.server {
Server::Pn | Server::Lid | Server::Hosted | Server::HostedLid => 0,
_ => self.agent,
}
}

pub fn to_non_ad(&self) -> Self {
Self {
user: self.user.clone(),
Expand Down Expand Up @@ -596,16 +589,6 @@ impl<'a> JidExt for JidRef<'a> {
}

impl<'a> JidRef<'a> {
pub fn new(user: Cow<'a, str>, server: Server) -> Self {
Self {
user,
server,
agent: 0,
device: 0,
integrator: 0,
}
}

pub fn to_owned(&self) -> Jid {
Jid {
user: CompactString::from(self.user.as_ref()),
Expand Down
4 changes: 3 additions & 1 deletion wacore/binary/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@ pub use marshal::{
marshal, marshal_auto, marshal_exact, marshal_ref, marshal_ref_auto, marshal_ref_exact,
marshal_ref_to, marshal_ref_to_vec, marshal_to, marshal_to_vec,
};
pub use node::{Attrs, Node, NodeContent, NodeContentRef, NodeRef, NodeValue, OwnedNodeRef};
pub use node::{
Attrs, Node, NodeContent, NodeContentRef, NodeRef, NodeStr, NodeValue, OwnedNodeRef,
};
Loading
Loading