Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
69 changes: 69 additions & 0 deletions src/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,78 @@ impl Client {
where
S: wacore::iq::spec::IqSpec,
{
// Try the direct-encode fast path to avoid intermediate Node allocations.
// Only allocate the buffer if the spec actually uses it.
let req_id = self.generate_request_id();
{
let mut buf = Vec::new();
if let Ok(true) = spec.encode_iq_direct(&req_id, &mut buf) {
let response = self.send_iq_raw(req_id, buf).await?;
return spec
.parse_response(response.get())
.map_err(IqError::ParseError);
}
}
Comment on lines +169 to +185

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider pre-allocating the buffer for the fast path.

Vec::new() starts at zero capacity and grows during encoding. Since prekey IQs are typically 1-4KB, a pre-sized buffer could avoid reallocations.

♻️ Optional optimization
-            let mut buf = Vec::new();
+            let mut buf = Vec::with_capacity(2048);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/request.rs` around lines 169 - 185, Pre-allocate the encode buffer to
avoid reallocations in the fast path: replace the zero-capacity Vec::new() used
before calling spec.encode_iq_direct(&req_id, &mut buf) with a
Vec::with_capacity(...) sized for typical prekey IQs (e.g. 4 * 1024) or, if
available, use a size hint from the spec (e.g. spec.encoded_len_hint() or
similar) to set capacity; keep the rest of the logic around
send_and_wait_iq(req_id, ... async { self.send_raw_bytes(buf).await }) and
parse_response(...) unchanged and still map encode errors to
IqError::EncodeError.


let iq = spec.build_iq();
let response = self.send_iq(iq).await?;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
spec.parse_response(response.get())
.map_err(IqError::ParseError)
}

/// Send pre-encoded IQ bytes and wait for the response.
async fn send_iq_raw(
&self,
req_id: String,
buf: Vec<u8>,
) -> Result<Arc<wacore_binary::OwnedNodeRef>, IqError> {
if !self.is_running.load(Ordering::Relaxed) {
return Err(IqError::NotConnected);
}

let default_timeout = Duration::from_secs(75);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Hardcoded timeout limits flexibility for future specs.

The 75-second default matches send_iq, but if future IqSpec implementations using encode_iq_direct need different timeouts, they'll be stuck with this value. Consider exposing an optional timeout in encode_iq_direct's return type or as a separate trait method if this becomes a practical concern.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/request.rs` at line 244, The hardcoded default_timeout
(Duration::from_secs(75)) reduces flexibility for different IqSpec
implementations; update the API so callers can control timeout instead of using
a fixed 75s. Specifically, modify encode_iq_direct (or its return type) or add
an optional trait method on IqSpec to provide a per-spec timeout, then replace
the direct use of default_timeout with the provided timeout (falling back to a
sensible default) and update send_iq to accept or honor that timeout; reference
symbols: default_timeout, Duration::from_secs(75), encode_iq_direct, send_iq,
IqSpec.


let (tx, rx) = futures::channel::oneshot::channel();
self.response_waiters
.lock()
.await
.insert(req_id.clone(), tx);

let shutdown = self.shutdown_notifier.listen();

if !self.is_running.load(Ordering::Acquire) {
self.response_waiters.lock().await.remove(&req_id);
return Err(IqError::NotConnected);
}

if let Err(e) = self.send_raw_bytes(buf).await {
self.response_waiters.lock().await.remove(&req_id);
return match e {
crate::client::ClientError::Socket(s_err) => Err(IqError::Socket(s_err)),
crate::client::ClientError::NotConnected => Err(IqError::NotConnected),
_ => Err(IqError::Socket(SocketError::Crypto(e.to_string()))),
};
}

let request_utils = self.get_request_utils();
futures::select! {
result = rt_timeout(&*self.runtime, default_timeout, rx).fuse() => {
match result {
Ok(Ok(response_node)) => match request_utils.parse_iq_response(response_node.get()) {
Ok(()) => Ok(response_node),
Err(e) => Err(e.into()),
},
Ok(Err(_)) => Err(IqError::InternalChannelClosed),
Err(_) => {
self.response_waiters.lock().await.remove(&req_id);
Err(IqError::Timeout)
}
}
}
_ = shutdown.fuse() => {
self.response_waiters.lock().await.remove(&req_id);
Err(IqError::NotConnected)
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
18 changes: 8 additions & 10 deletions src/socket/noise_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,10 @@ impl NoiseSocket {
send_job_rx: async_channel::Receiver<SendJob>,
) {
let mut write_counter: u32 = 0;
// Reusable buffers -- capacity stays allocated between sends
let mut enc_buf = Vec::with_capacity(4096);
let mut out_buf = Vec::with_capacity(4096);
// BytesMut: split().freeze() yields a zero-copy Bytes while retaining
// the underlying allocation for the next frame.
let mut out_buf = BytesMut::with_capacity(4096);

while let Ok(job) = send_job_rx.recv().await {
let result = Self::process_send_job(
Expand All @@ -106,24 +107,21 @@ impl NoiseSocket {
write_counter: &mut u32,
plaintext: &[u8],
enc_buf: &mut Vec<u8>,
out_buf: &mut Vec<u8>,
out_buf: &mut BytesMut,
) -> SendResult {
let counter = *write_counter;

if plaintext.len() <= INLINE_ENCRYPT_THRESHOLD {
// Copy into reusable enc_buf, encrypt in place
enc_buf.clear();
enc_buf.extend_from_slice(plaintext);
if let Err(e) = write_key.encrypt_in_place_with_counter(counter, enc_buf) {
return Err(EncryptSendError::crypto(anyhow::anyhow!(e.to_string())));
}

out_buf.clear();
if let Err(e) = wacore::framing::encode_frame_into(enc_buf, None, out_buf) {
return Err(EncryptSendError::framing(e));
}
} else {
// Large messages: encrypt on blocking thread (reads plaintext, returns new ciphertext)
let write_key = write_key.clone();
let plaintext_owned = plaintext.to_vec();

Expand All @@ -139,15 +137,15 @@ impl NoiseSocket {
}
};

out_buf.clear();
if let Err(e) = wacore::framing::encode_frame_into(&ciphertext, None, out_buf) {
return Err(EncryptSendError::framing(e));
}
}

// copy_from_slice so out_buf retains its capacity for the next send
let frame = bytes::Bytes::copy_from_slice(out_buf);
out_buf.clear();
// Zero-copy: split() moves the written data into a new BytesMut,
// freeze() converts it to Bytes. The original out_buf retains its
// allocated capacity for the next frame.
let frame = out_buf.split().freeze();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve send buffer capacity across frame handoff

Using out_buf.split().freeze() here does not keep the reusable buffer capacity as intended: split() is split_to(len), which advances out_buf to the tail region. Under sustained traffic, each send reduces the remaining capacity by that frame size until it reaches zero, so encode_frame_into starts reallocating frequently. This turns the new hot path into allocation churn on long-lived connections and undermines the performance goal of this change.

Useful? React with 👍 / 👎.

if let Err(e) = transport.send(frame).await {
return Err(EncryptSendError::transport(e));
}
Expand Down
29 changes: 16 additions & 13 deletions wacore/binary/src/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::jid::{self, Jid, JidRef};
use crate::node::{Node, NodeContent, NodeContentRef, NodeRef, NodeValue, ValueRef};
use crate::token;

pub(crate) trait ByteWriter {
pub trait ByteWriter {
fn write_u8(&mut self, value: u8) -> Result<()>;
fn write_bytes(&mut self, bytes: &[u8]) -> Result<()>;
}
Expand Down Expand Up @@ -41,7 +41,7 @@ impl<W: Write> ByteWriter for IoByteWriter<W> {
}
}

pub(crate) struct VecByteWriter<'a> {
pub struct VecByteWriter<'a> {
buffer: &'a mut Vec<u8>,
}

Expand Down Expand Up @@ -110,7 +110,7 @@ impl ByteWriter for SliceByteWriter<'_> {
/// Trait for encoding node structures (both owned Node and borrowed NodeRef).
/// All encoding logic lives in the trait implementation, keeping
/// the Encoder simple and focused on low-level byte writing.
pub(crate) trait EncodeNode {
pub trait EncodeNode {
fn tag(&self) -> &str;
fn attrs_len(&self) -> usize;
fn has_content(&self) -> bool;
Expand Down Expand Up @@ -177,7 +177,7 @@ impl EncodeNode for NodeRef<'_> {
}

fn encode_attrs<'a, W: ByteWriter>(&self, encoder: &mut Encoder<'a, W>) -> Result<()> {
for (k, v) in &self.attrs {
for (k, v) in self.attrs.iter() {
encoder.write_string(k)?;
match v {
ValueRef::String(s) => encoder.write_string(s)?,
Expand Down Expand Up @@ -602,13 +602,13 @@ fn validate_hex(value: &str) -> bool {
.all(|&b| b.is_ascii_digit() || (b'A'..=b'F').contains(&b))
}

pub(crate) struct Encoder<'a, W: ByteWriter> {
pub struct Encoder<'a, W: ByteWriter> {
writer: W,
string_hints: Option<&'a StringHintCache>,
}

impl<W: Write> Encoder<'static, IoByteWriter<W>> {
pub(crate) fn new(writer: W) -> Result<Self> {
pub fn new(writer: W) -> Result<Self> {
let mut enc = Self {
writer: IoByteWriter::new(writer),
string_hints: None,
Expand All @@ -619,7 +619,8 @@ impl<W: Write> Encoder<'static, IoByteWriter<W>> {
}

impl<'v> Encoder<'static, VecByteWriter<'v>> {
pub(crate) fn new_vec(buffer: &'v mut Vec<u8>) -> Result<Self> {
pub fn new_vec(buffer: &'v mut Vec<u8>) -> Result<Self> {
buffer.clear();
let mut enc = Self {
writer: VecByteWriter::new(buffer),
string_hints: None,
Expand Down Expand Up @@ -680,7 +681,7 @@ impl<'a, W: ByteWriter> Encoder<'a, W> {
}

#[inline(always)]
fn write_bytes_with_len(&mut self, bytes: &[u8]) -> Result<()> {
pub fn write_bytes_with_len(&mut self, bytes: &[u8]) -> Result<()> {
let len = bytes.len();
if len < 256 {
self.write_u8(token::BINARY_8)?;
Expand All @@ -696,7 +697,7 @@ impl<'a, W: ByteWriter> Encoder<'a, W> {
}

#[inline(always)]
fn write_string(&mut self, s: &str) -> Result<()> {
pub fn write_string(&mut self, s: &str) -> Result<()> {
if let Some(string_hints) = self.string_hints
&& let Some(hint) = string_hints.hint_for(s)
{
Expand Down Expand Up @@ -782,7 +783,7 @@ impl<'a, W: ByteWriter> Encoder<'a, W> {

/// Write an owned Jid directly without converting to string first.
/// This avoids the allocation that would occur with `jid.to_string()`.
fn write_jid_owned(&mut self, jid: &Jid) -> Result<()> {
pub fn write_jid_owned(&mut self, jid: &Jid) -> Result<()> {
if jid.device > 0 {
// AD_JID format: domain_type, device, user
let device = u8::try_from(jid.device).map_err(|_| {
Expand Down Expand Up @@ -909,21 +910,23 @@ impl<'a, W: ByteWriter> Encoder<'a, W> {
Ok(())
}

fn write_list_start(&mut self, len: usize) -> Result<()> {
pub fn write_list_start(&mut self, len: usize) -> Result<()> {
if len == 0 {
self.write_u8(token::LIST_EMPTY)?;
} else if len < 256 {
self.write_u8(248)?;
self.write_u8(len as u8)?;
} else {
} else if len <= u16::MAX as usize {
self.write_u8(249)?;
self.write_u16_be(len as u16)?;
} else {
return Err(BinaryError::InvalidNode);
}
Ok(())
}

/// Write any node type (owned or borrowed) using the EncodeNode trait.
pub(crate) fn write_node<N: EncodeNode>(&mut self, node: &N) -> Result<()> {
pub fn write_node<N: EncodeNode>(&mut self, node: &N) -> Result<()> {
let content_len = if node.has_content() { 1 } else { 0 };
let list_len = 1 + (node.attrs_len() * 2) + content_len;

Expand Down
2 changes: 1 addition & 1 deletion wacore/binary/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pub mod attrs;
pub mod builder;
pub mod consts;
mod decoder;
mod encoder;
pub mod encoder;
pub mod error;
pub mod jid;
pub mod marshal;
Expand Down
103 changes: 101 additions & 2 deletions wacore/binary/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,106 @@ impl FromIterator<(Cow<'static, str>, NodeValue)> for Attrs {
Self(iter.into_iter().collect())
}
}
pub type AttrsRef<'a> = Vec<(NodeStr<'a>, ValueRef<'a>)>;
/// Covariant inline container for decoded node attributes.
/// Most WA protocol nodes have 0-1 attributes; storing them inline avoids
/// a heap allocation for the common case. Covariant in `'a` (unlike SmallVec)
/// so it works with yoke::Yokeable.
#[derive(Debug, Clone, PartialEq)]
pub enum AttrsRef<'a> {
Empty,
One((NodeStr<'a>, ValueRef<'a>)),
Many(Vec<(NodeStr<'a>, ValueRef<'a>)>),
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

impl<'a> AttrsRef<'a> {
pub fn with_capacity(n: usize) -> Self {
match n {
0 => Self::Empty,
// 1 attr: start Empty, push() will create One (no heap alloc)
1 => Self::Empty,
_ => Self::Many(Vec::with_capacity(n)),
}
}

pub fn push(&mut self, item: (NodeStr<'a>, ValueRef<'a>)) {
match self {
Self::Empty => *self = Self::One(item),
Self::One(_) => {
let Self::One(prev) = std::mem::replace(self, Self::Empty) else {
unreachable!()
};
*self = Self::Many(vec![prev, item]);
}
Self::Many(v) => v.push(item),
}
}

#[inline]
pub fn len(&self) -> usize {
match self {
Self::Empty => 0,
Self::One(_) => 1,
Self::Many(v) => v.len(),
}
}

#[inline]
pub fn is_empty(&self) -> bool {
matches!(self, Self::Empty)
}

#[inline]
pub fn as_slice(&self) -> &[(NodeStr<'a>, ValueRef<'a>)] {
match self {
Self::Empty => &[],
Self::One(item) => std::slice::from_ref(item),
Self::Many(v) => v.as_slice(),
}
}

#[inline]
pub fn iter(&self) -> impl Iterator<Item = &(NodeStr<'a>, ValueRef<'a>)> {
self.as_slice().iter()
}
}

impl<'a> FromIterator<(NodeStr<'a>, ValueRef<'a>)> for AttrsRef<'a> {
fn from_iter<I: IntoIterator<Item = (NodeStr<'a>, ValueRef<'a>)>>(iter: I) -> Self {
let mut result = Self::Empty;
for item in iter {
result.push(item);
}
result
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// Safety: AttrsRef is covariant in 'a because:
// - Empty has no lifetime
// - One contains (NodeStr<'a>, ValueRef<'a>) which are covariant
// - Many(Vec<T>) is covariant in T
// yoke::Yokeable requires covariance, which Vec and tuples provide.
unsafe impl<'a> yoke::Yokeable<'a> for AttrsRef<'static> {
type Output = AttrsRef<'a>;

fn transform(&'a self) -> &'a Self::Output {
self
}

fn transform_owned(self) -> Self::Output {
self
}

unsafe fn make(from: Self::Output) -> Self {
unsafe { std::mem::transmute(from) }
}

fn transform_mut<F>(&'a mut self, f: F)
where
F: 'static + for<'b> FnOnce(&'b mut Self::Output),
{
unsafe { f(std::mem::transmute::<&mut Self, &mut Self::Output>(self)) }
}
}
Comment on lines +460 to +481

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Unsafe Yokeable impl looks correct but warrants careful maintenance.

The safety argument relies on AttrsRef being covariant in 'a. This holds because:

  • Empty has no lifetime
  • One contains (NodeStr<'a>, ValueRef<'a>) which are both #[derive(Yokeable)]
  • Many(Vec<T>) is covariant in T

The transmute in make() and transform_mut() is safe given identical memory layout across lifetimes. Consider adding a compile-time covariance assertion (e.g., a phantom function) to catch accidental future changes that break covariance.

🤖 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 464 - 485, The unsafe Yokeable impl
for AttrsRef (impl Yokeable for AttrsRef<'static>) relies on AttrsRef remaining
covariant in 'a; add a compile-time covariance assertion to detect future
regressions by introducing a private phantom-check function that references
AttrsRef with contravariant and covariant contexts (e.g., a helper that accepts
fn(AttrsRef<'static>) -> AttrsRef<'_> or uses core::mem::transmute checks) and
call it in the module so the compiler will error if covariance is violated;
update or document this check alongside the unsafe methods make() and
transform_mut() so future changes to AttrsRef variants (Empty, One, Many) are
caught early.


/// A decoded attribute value that can be either a string or a structured JID.
/// This avoids string allocation when decoding JID tokens - the JidRef is returned
Expand Down Expand Up @@ -615,7 +714,7 @@ struct AttrsRefWrapper<'a, 'b>(&'b AttrsRef<'a>);
#[cfg(feature = "serde")]
impl serde::Serialize for AttrsRefWrapper<'_, '_> {
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
serializer.serialize_newtype_struct("Attrs", self.0)
serializer.serialize_newtype_struct("Attrs", self.0.as_slice())
}
}

Expand Down
Loading
Loading