perf!: use CompactString for NodeValue, NodeContent, and Jid.user - #512
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates many core string fields to compact_str::CompactString, adds compact_str as a workspace dependency and re-exports it, updates many call sites/tests to use .into()/.to_string() for compatibility, and adds sent-message TTL-based eviction to the in-memory store. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@claude review check for further refinements, possibly remove unnecessary clones, allocations, consistency, bugs, regressions, etc |
|
Claude finished @jlucaso1's task in 5m 4s —— View job Review: CompactString Migration PR
Overall this is a clean, well-scoped migration. The profiling data is compelling and the changes are mechanical + correct. A few findings below, roughly ordered by impact. Unnecessary allocations —
|
| File | Line | Current | Suggestion |
|---|---|---|---|
wacore/src/stanza/groups.rs |
310 | s.to_string() → String for member_add_mode |
Fine (target is String), but could change field to CompactString to avoid alloc |
wacore/src/stanza/business.rs |
62 | s.to_string() for verified name |
Same — target is Option<String> |
wacore/src/iq/usync.rs |
103 | user.jid.user.to_string() for phone number formatting |
Needed here because of format!("+{}", ...) in the else branch |
src/handlers/notification.rs |
625, 689, 704, 715 | Multiple from_jid.user.to_string() |
These feed into DeviceListRecord.user: String and String variables — needed unless downstream types are also migrated |
src/send.rs |
1220, 1413, 1417, 1473, 1477 | to.user.to_string() / sender.user.to_string() / jid.user.to_string() |
These feed String variables for token_jid lookups into LidPnCache (which uses TypedCache<String, _>) — allocation is necessary here |
Verdict: Most of the .to_string() calls are actually necessary because the downstream types (DeviceListRecord, UsyncLidMapping, LidPnEntry, LidPnCache, etc.) still use String. This is the correct approach for a scoped PR — you migrated the hot path (binary protocol layer) without refactoring every downstream consumer. The remaining .to_string() calls are on cold paths (notification handling, IQ responses, token lookups) where the allocation cost is negligible.
However, there is one spot worth reconsidering:
wacore/src/send.rs:1243 — user_set.insert(d.user.to_string()) inside the SKDM stale-user loop. This collects into a HashSet<String>. Since d.user is CompactString, you could use HashSet<CompactString> and .clone() instead to avoid heap allocations for phone-number-length strings. The stale_device_users field is Vec<String> downstream though, so you'd need to .to_string() when collecting the final Vec. Net benefit is marginal — only matters if the distribution list is large.
Redundant .clone() on CompactString that could be avoided
wacore/src/client/context.rs:13 — build_pn_to_lid_map:
let lid_jid = Jid::lid(lid_user.clone());lid_user is &CompactString (from iterating HashMap), and Jid::lid takes impl Into<CompactString>. The .clone() creates a new CompactString, which is correct. However, since you're borrowing from a map you're iterating, the clone is necessary here. No issue.
wacore/src/client/context.rs:132 — phone_device_jid_to_lid:
Jid::lid_device(lid_base.user.clone(), phone_device_jid.device)lid_base is &Jid from a map lookup. Clone is necessary. Fine.
InMemoryBackend TTL eviction — potential issue
wacore/src/store/in_memory.rs:499-501:
if s.sent_messages.len() >= SENT_MESSAGE_EVICT_THRESHOLD {
let cutoff = now - self.sent_message_ttl_secs;
s.sent_messages.retain(|_, e| e.timestamp >= cutoff);
}This is a good addition that caps unbounded growth. A couple of observations:
-
Threshold of 64 is reasonable for an in-memory test/benchmark backend. The eviction only runs on
store_sent_message, not ontake_sent_message, which is fine sincetake_sent_messagealready removes entries. -
Edge case: If a clock is misbehaving (e.g.,
now_secs()returns 0 in tests with mocked time),cutoff = 0 - 300 = -300would retain everything (all timestamps ≥ -300). This is actually the safe behavior — no accidental data loss. Good. -
No eviction on read path:
take_sent_messagedoesn't evict expired entries, but since it usesremove()it won't return stale data. The only risk is if messages are stored but never taken — they accumulate until the nextstore_sent_messagecall crosses the threshold. For a test/benchmark backend, this is fine.
Consistency nits
-
wacore/binary/src/marshal.rs:270-272uses the verbose form:Some(NodeContent::String(compact_str::CompactString::from("hello".repeat(40))))
Since
compact_str::CompactStringis already imported asCompactStringinnode.rs(and re-exported fromwacore_binary), this could just be:Some(NodeContent::String("hello".repeat(40).into()))
or import
CompactStringat the top. Minor — it's test code. -
wacore/src/iq/profile.rs:41-43uses:NodeContent::String(wacore_binary::CompactString::from(self.text.as_str()))
Could be simplified to:
NodeContent::String(self.text.as_str().into())
since
&str → CompactStringviaFromis available. -
src/message.rs:2470— test still usesHashMap<String, Jid>with.to_string()inserts, while the sibling tests at lines 2345-2351 and 2410-2416 useHashMap<CompactString, Jid>withCompactString::from(...). Inconsistent but functionally fine (it's simulating inline logic, not usingGroupInfo).
No bugs or regressions found
- All
CompactString↔&strconversions are correct.CompactStringimplementsDeref<Target=str>,Borrow<str>,PartialEq<str>,PartialEq<&str>, andPartialEq<String>, so all comparison, HashMap lookup, and method-call sites work correctly. - The encoder/decoder round-trip is safe —
write_stringtakes&strandCompactStringderefs to&strtransparently. serdefeature gate correctly addscompact_str/serde— serialization/deserialization ofJid,NodeValue, andNodeContentwill work correctly.- The benchmark example switch from
SqliteStoretoInMemoryBackendis appropriate for the profiling use case and removes an unnecessary dependency on SQLite for benchmarks.
Summary
The migration is well-executed and correctly scoped. The -34% peak heap reduction is significant for a change of this size. The main refinement opportunity is the verbose CompactString::from(...) patterns in a few places that could use .into(), but these are cosmetic. No correctness issues found.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed96886491
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cutoff = now - self.sent_message_ttl_secs; | ||
| s.sent_messages.retain(|_, e| e.timestamp >= cutoff); |
There was a problem hiding this comment.
Inline eviction uses self.sent_message_ttl_secs here, but this backend field is initialized to a fixed 300s default and (repo-wide rg) is never wired from Client.cache_config.sent_message_ttl_secs. That means any non-default TTL (including 0 to disable expiration) is ignored once sent_messages reaches the threshold, so older entries can be dropped far earlier than the configured policy and retry handling may fail to find payloads that should still be retained.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client.rs`:
- Line 3606: The code currently allocates an intermediate String via
node.tag.to_string() when inserting the class attribute; replace that allocation
by passing a &str directly to NodeValue::from (use node.tag.as_str() or
&node.tag as appropriate) in the attrs.insert call so NodeValue::from receives a
string slice instead of building a new String; update the line using
attrs.insert, NodeValue::from, and node.tag accordingly to avoid the extra
allocation.
In `@wacore/src/store/in_memory.rs`:
- Around line 498-502: The current eviction does a full retain on
s.sent_messages on nearly every store_sent_message call once len >=
SENT_MESSAGE_EVICT_THRESHOLD, causing O(n) work and lock contention; change the
logic to amortize scans by adding a throttling condition (e.g., a
last_sent_eviction timestamp or counter on the same struct) and only run the
retain when either enough wall-clock time has passed since last eviction (e.g.,
>= SENT_MESSAGE_EVICT_INTERVAL_SECS) or the map grows well beyond the threshold
(e.g., >= SENT_MESSAGE_EVICT_THRESHOLD * 2); update the store_sent_message
function to consult and update that last_sent_eviction marker (and constants
like SENT_MESSAGE_EVICT_INTERVAL_SECS) under the mutex so retain runs rarely, or
alternatively remove a bounded number of expired entries per call instead of
full retain.
- Around line 95-98: The with_sent_message_ttl setter currently accepts
non-positive values which can trigger massive eviction; update the function
with_sent_message_ttl to validate ttl_secs > 0 before assigning to
self.sent_message_ttl_secs and if ttl_secs is <= 0 do not change the field
(return self unchanged) or optionally set a safe minimum (e.g., 1) so
non-positive inputs cannot overwrite the existing TTL and cause unintended
retry-cache invalidation.
🪄 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: 7519d98f-8c58-4d1d-9f4d-ca79fabb367b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
Cargo.tomlexamples/benchmark.rssrc/client.rssrc/client/lid_pn.rssrc/features/groups.rssrc/handlers/notification.rssrc/handlers/router.rssrc/history_sync.rssrc/lib.rssrc/message.rssrc/retry.rssrc/send.rssrc/session.rssrc/usync.rstests/e2e/src/lib.rswacore/binary/Cargo.tomlwacore/binary/src/builder.rswacore/binary/src/decoder.rswacore/binary/src/encoder.rswacore/binary/src/jid.rswacore/binary/src/lib.rswacore/binary/src/marshal.rswacore/binary/src/node.rswacore/src/client/context.rswacore/src/iq/business.rswacore/src/iq/profile.rswacore/src/iq/spam_report.rswacore/src/iq/usync.rswacore/src/message_processing.rswacore/src/prekeys.rswacore/src/protocol/retry.rswacore/src/send.rswacore/src/stanza/business.rswacore/src/stanza/groups.rswacore/src/store/in_memory.rswacore/src/usync.rs
Benchmark Results3 improvement(s):
56 unchanged benchmark(s)
|
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/benches/send_receive_benchmark.rs (1)
190-196: 🧹 Nitpick | 🔵 TrivialConsider using the
Jid::new()factory method.The struct literal construction could be simplified using the existing factory method, which performs the same operations more concisely.
♻️ Proposed refactor using factory method
- let jid = Jid { - user: user.into(), - server: wacore_binary::jid::cow_server_from_str(server), - device: 0, - agent: 0, - integrator: 0, - }; + let jid = Jid::new(user, server);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/benches/send_receive_benchmark.rs` around lines 190 - 196, The code constructs a Jid via a struct literal (Jid { user: ..., server: wacore_binary::jid::cow_server_from_str(server), device: 0, agent: 0, integrator: 0 }) — replace this with the existing factory method Jid::new(user.into(), server) (or the appropriate Jid::new signature) to simplify and centralize server conversion logic; update the call sites in send_receive_benchmark.rs to use Jid::new instead of manual field initialization.
🤖 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/benches/send_receive_benchmark.rs`:
- Around line 190-196: The code constructs a Jid via a struct literal (Jid {
user: ..., server: wacore_binary::jid::cow_server_from_str(server), device: 0,
agent: 0, integrator: 0 }) — replace this with the existing factory method
Jid::new(user.into(), server) (or the appropriate Jid::new signature) to
simplify and centralize server conversion logic; update the call sites in
send_receive_benchmark.rs to use Jid::new instead of manual field
initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad8d652a-e9ca-4d36-89f3-621f894313f0
📒 Files selected for processing (1)
wacore/benches/send_receive_benchmark.rs
|
@claude review things carefully; also, check possible additional refinements we can do before merging (since this PR is a breaking change, so we can do superb things) |
|
Claude finished @jlucaso1's task in 3m 33s —— View job Deep Review: CompactString Migration
Overall: clean, well-scoped migration with compelling profiling data. The fixup commits (65a2ff9, 4ec5401) already addressed most of the low-hanging fruit from the initial reviews. Below are findings ordered by impact, split into "should fix before merge" and "could do while you're here" given this is already a breaking change. Issues to fix before merge1.
|
| Location | Code | Why it allocates |
|---|---|---|
wacore/src/send.rs:1243 |
d.user.to_string() into HashSet<String> |
Could use HashSet<CompactString> + .clone() |
wacore/src/stanza/groups.rs:312 |
s.to_string() for member_add_mode |
Target is String in GroupNotificationAction — could change variant field to CompactString |
wacore/src/stanza/business.rs:62 |
s.to_string() for verified name |
Target is Option<String> |
wacore/src/stanza/business.rs:265 |
id.to_string() for product IDs |
Target is Vec<String> |
The GroupNotificationAction enum fields and related String-typed fields in stanza types could be migrated to CompactString for consistency, but that's a wider change and these are cold paths.
5. ensure_status_participants — nice use of CompactString
In wacore/src/send.rs:1316, existing_users was changed to HashSet<CompactString> — good pattern. But user_set at line 1243 is still HashSet<String>, so d.user.to_string() still allocates. Consider the same treatment.
6. build_user_nodes phone formatting (wacore/src/iq/usync.rs:102-106)
let phone = if user.jid.user.starts_with('+') {
user.jid.user.to_string() // allocates String
} else {
format!("+{}", user.jid.user) // also allocates
};The format! branch is unavoidable. The first branch could use .clone() and pass CompactString since string_content takes impl Into<CompactString>. Fix this →
7. iq/profile.rs — verbose CompactString construction
At wacore/src/iq/profile.rs:41:
content: Some(NodeContent::String(self.text.as_str().into())),This is fine, but could be even simpler since self.text is String:
content: Some(NodeContent::String(self.text.clone().into())),...or best, since From<String> for CompactString exists:
content: Some(NodeContent::String(CompactString::from(self.text.as_str()))),Current code is already correct and equivalent. No change needed — just noting for style consistency.
InMemoryBackend TTL eviction — looks solid
The eviction logic at wacore/src/store/in_memory.rs:505-517 is well-designed after the fixup:
- Amortized: Only scans when both len ≥ 64 AND ≥ 30s since last eviction
- Atomic last_eviction: Uses
Relaxedordering correctly (monotonic clock, single writer under mutex) - TTL validation:
with_sent_message_ttlrejects ≤ 0 values - No read-path eviction:
take_sent_messageremoves viaremove(), no stale data risk
One minor observation: last_eviction is AtomicI64 outside the mutex while sent_messages is inside. This means a concurrent reader of last_eviction could see a stale value, but since the atomic is only used as a "has enough time passed?" heuristic under the mutex guard, this is safe and correct.
No bugs or correctness issues found
- All
CompactString↔&strconversions are correct viaDeref<Target=str>,Borrow<str>,PartialEq<str>,PartialEq<&str>, andPartialEq<String>. - Encoder/decoder round-trip is safe —
write_stringtakes&str. serdefeature gate correctly addscompact_str/serde.- The privacy token notification refactoring (
src/handlers/notification.rs:683-721) is a clean borrow-based approach that avoids all intermediate allocations. - The
history_sync.rsandsend.rstoken_jid patterns correctly use&strborrows.
Summary
The migration is well-executed. The main opportunity before merge is widening the CompactString adoption to LidPnEntry, LidPnMappingEntry, and DeviceListRecord — these are already breaking types in the PR's dependency chain, and their fields (phone numbers, LIDs) are always ≤16 chars. This would close the allocation gap between the binary protocol layer and the storage/cache layer. Everything else is minor and could be follow-up.
4ec5401 to
11edf13
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11edf130f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let resolved_lid_user = if to.is_lid() { | ||
| Some(to.user.clone()) | ||
| None |
There was a problem hiding this comment.
Setting resolved_lid_user to None for to.is_lid() drops direct-LID recipients out of the NCT fallback path: the later guard && let Some(lid_user) = &resolved_lid_user will always fail, so no cstoken is attached when there is no stored tctoken. This regresses first-contact sends to LID targets (with NCT salt + flag enabled) and can surface as avoidable 463 failures, while the previous behavior still had the recipient LID available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/send.rs`:
- Around line 1219-1225: The current logic forces resolved_lid_user to None when
to.is_lid(), which prevents using cached LID tokens for direct-LID 1:1 sends and
breaks the cstoken fallback; change the assignment so you always call
self.lid_pn_cache.get_current_lid(&to.user).await (i.e., remove the if
to.is_lid() branch) so resolved_lid_user is Some(...) when a cache hit exists
and token_jid still falls back to &to.user via as_deref().unwrap_or(&to.user),
restoring the nct_send_enabled / cstoken fallback path (affecting
resolved_lid_user, to.is_lid(), lid_pn_cache.get_current_lid, and token_jid
usage).
🪄 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: 95bfaff6-0bd8-4826-8c04-d262e8e253f8
📒 Files selected for processing (7)
src/features/groups.rssrc/handlers/notification.rssrc/history_sync.rssrc/send.rswacore/benches/send_receive_benchmark.rswacore/src/iq/usync.rswacore/src/usync.rs
11edf13 to
c4b6600
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/history_sync.rs (1)
362-373:⚠️ Potential issue | 🟠 MajorDon’t overwrite tc-token state after a read failure.
Errfromget_tc_token(token_key)currently falls into the same branch asOk(None), and the code still writes a new entry. That skips the monotonicity check and can clobber a newer cachedsender_timestampwith stale history-sync data after a transient backend error.Suggested fix
- let merged_sender_ts = if let Ok(Some(existing)) = backend.get_tc_token(token_key).await { - if (existing.token_timestamp as u64) > timestamp { - return; - } - match (existing.sender_timestamp, incoming_sender_ts) { - (Some(e), Some(i)) => Some(e.max(i)), - (Some(e), None) => Some(e), - (None, i) => i, - } - } else { - incoming_sender_ts - }; + let merged_sender_ts = match backend.get_tc_token(token_key).await { + Ok(Some(existing)) => { + if (existing.token_timestamp as u64) > timestamp { + return; + } + match (existing.sender_timestamp, incoming_sender_ts) { + (Some(e), Some(i)) => Some(e.max(i)), + (Some(e), None) => Some(e), + (None, i) => i, + } + } + Ok(None) => incoming_sender_ts, + Err(e) => { + log::warn!( + target: "Client/TcToken", + "Failed to read existing history sync tctoken for {}: {e}", + token_key + ); + return; + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/history_sync.rs` around lines 362 - 373, The current logic treats Err from backend.get_tc_token(token_key) the same as Ok(None) and proceeds to write merged_sender_ts, which can clobber newer state; change the branch so that if backend.get_tc_token(token_key) returns Err you do not proceed with writing (e.g., return or propagate the error) instead of falling back to incoming_sender_ts—ensure the match around get_tc_token distinguishes Ok(Some(existing)), Ok(None) and Err(_) and only computes merged_sender_ts/write when you have a successful read (Ok), keeping references to get_tc_token, merged_sender_ts, token_key, incoming_sender_ts, sender_timestamp, and token_timestamp to locate the code to modify.wacore/src/send.rs (1)
1237-1247: 🧹 Nitpick | 🔵 TrivialDeduplicate stale users before allocating
Strings.
d.user.to_string()allocates once per missing device, so a user with several stale companions still pays multiple heap allocations before theHashSetdrops duplicates. In this perf-focused PR, collect borrowed users first and stringify only once when buildingstale_device_users.Suggested change
- let mut user_set = HashSet::new(); + let mut user_set: HashSet<&str> = HashSet::new(); if let Some(ref dist) = distribution_list { for d in dist { if !encrypted_set.contains(d) { - user_set.insert(d.user.to_string()); + user_set.insert(d.user.as_str()); } } } - user_set.into_iter().collect() + user_set.into_iter().map(str::to_owned).collect()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/send.rs` around lines 1237 - 1247, The current loop converts d.user.to_string() for every missing device which repeats allocations; instead collect borrowed user IDs first and stringify once when building stale_device_users: in the stale_users block (around had_unregistered_devices, skdm_encrypted_devices, distribution_list) change user_set to a HashSet of borrowed IDs (e.g., HashSet<&Jid> or HashSet<&str>) and insert &d.user when a device is missing, then after the loop convert the unique borrows into Strings (user_set.into_iter().map(|u| u.to_string()).collect()) so each user is allocated only once.
♻️ Duplicate comments (1)
src/send.rs (1)
1219-1224:⚠️ Potential issue | 🟠 MajorDirect-LID chats still bypass the
cstokenfallback.On Line 1219, forcing
resolved_lid_usertoNoneforto.is_lid()makes the fallback on Lines 1267-1270 unreachable for direct-LID recipients. If there is no cachedtctoken, first-contact LID sends regress back to token-less stanzas.Also applies to: 1267-1270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/send.rs` around lines 1219 - 1224, The code currently forces resolved_lid_user to None when to.is_lid(), which prevents the later cstoken fallback from ever being reached for direct-LID recipients; instead, remove the short-circuit and always call self.lid_pn_cache.get_current_lid(&to.user).await to attempt resolution, then let token_jid = resolved_lid_user.as_deref().unwrap_or(&to.user) and let the existing cstoken fallback logic (the later token lookup code) handle missing entries; update any null-handling around lid_pn_cache.get_current_lid and ensure behavior is correct for both LID and JID inputs (refer to resolved_lid_user, to.is_lid(), lid_pn_cache.get_current_lid, and token_jid).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/benchmark.rs`:
- Line 34: The benchmark currently uses InMemoryBackend::new() which brings the
default 300s sent-message retry cache and skews long-run measurements; replace
the call to InMemoryBackend::new() with the backend constructor that accepts an
explicit TTL (e.g., InMemoryBackend::with_ttl or similar) and pass a short,
benchmark-specific Duration (for example 5–30s) so the benchmark measures
protocol performance reproducibly and not retention/eviction behavior—update the
variable backend (Arc::new(...)) to use that TTL-taking constructor.
In `@wacore/binary/src/builder.rs`:
- Around line 54-55: The public method string_content currently exposes
compact_str::CompactString in its signature; change the bound to use the crate
re-export instead (e.g., impl Into<crate::CompactString>) so the public API
references the local CompactString alias; update the signature of string_content
(and any other public-facing signatures in builder.rs that mention
compact_str::CompactString) to use crate::CompactString while leaving the body
(self.content = Some(NodeContent::String(s.into()))) unchanged.
In `@wacore/src/store/in_memory.rs`:
- Around line 507-516: The eviction cadence currently uses the fixed
SENT_MESSAGE_EVICT_INTERVAL_SECS which can exceed a caller-set
sent_message_ttl_secs; change the check that compares now - last against
SENT_MESSAGE_EVICT_INTERVAL_SECS to instead use the smaller of
SENT_MESSAGE_EVICT_INTERVAL_SECS and self.sent_message_ttl_secs so short TTLs
are honored. Update the condition around last_eviction (and any related
bookkeeping such as calling sent_messages.retain and last_eviction.store) to
compute interval = min(SENT_MESSAGE_EVICT_INTERVAL_SECS,
self.sent_message_ttl_secs) and use that interval for the throttle decision so
eviction runs at least as often as the configured TTL.
---
Outside diff comments:
In `@src/history_sync.rs`:
- Around line 362-373: The current logic treats Err from
backend.get_tc_token(token_key) the same as Ok(None) and proceeds to write
merged_sender_ts, which can clobber newer state; change the branch so that if
backend.get_tc_token(token_key) returns Err you do not proceed with writing
(e.g., return or propagate the error) instead of falling back to
incoming_sender_ts—ensure the match around get_tc_token distinguishes
Ok(Some(existing)), Ok(None) and Err(_) and only computes merged_sender_ts/write
when you have a successful read (Ok), keeping references to get_tc_token,
merged_sender_ts, token_key, incoming_sender_ts, sender_timestamp, and
token_timestamp to locate the code to modify.
In `@wacore/src/send.rs`:
- Around line 1237-1247: The current loop converts d.user.to_string() for every
missing device which repeats allocations; instead collect borrowed user IDs
first and stringify once when building stale_device_users: in the stale_users
block (around had_unregistered_devices, skdm_encrypted_devices,
distribution_list) change user_set to a HashSet of borrowed IDs (e.g.,
HashSet<&Jid> or HashSet<&str>) and insert &d.user when a device is missing,
then after the loop convert the unique borrows into Strings
(user_set.into_iter().map(|u| u.to_string()).collect()) so each user is
allocated only once.
---
Duplicate comments:
In `@src/send.rs`:
- Around line 1219-1224: The code currently forces resolved_lid_user to None
when to.is_lid(), which prevents the later cstoken fallback from ever being
reached for direct-LID recipients; instead, remove the short-circuit and always
call self.lid_pn_cache.get_current_lid(&to.user).await to attempt resolution,
then let token_jid = resolved_lid_user.as_deref().unwrap_or(&to.user) and let
the existing cstoken fallback logic (the later token lookup code) handle missing
entries; update any null-handling around lid_pn_cache.get_current_lid and ensure
behavior is correct for both LID and JID inputs (refer to resolved_lid_user,
to.is_lid(), lid_pn_cache.get_current_lid, and token_jid).
🪄 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: 57293095-61ab-4e3d-8d23-6fe0da677029
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
Cargo.tomlexamples/benchmark.rssrc/client.rssrc/client/lid_pn.rssrc/features/groups.rssrc/handlers/notification.rssrc/handlers/router.rssrc/history_sync.rssrc/lib.rssrc/message.rssrc/retry.rssrc/send.rssrc/session.rssrc/usync.rstests/e2e/src/lib.rswacore/benches/send_receive_benchmark.rswacore/binary/Cargo.tomlwacore/binary/src/builder.rswacore/binary/src/decoder.rswacore/binary/src/encoder.rswacore/binary/src/jid.rswacore/binary/src/lib.rswacore/binary/src/marshal.rswacore/binary/src/node.rswacore/src/client/context.rswacore/src/iq/business.rswacore/src/iq/groups.rswacore/src/iq/profile.rswacore/src/iq/spam_report.rswacore/src/iq/usync.rswacore/src/message_processing.rswacore/src/prekeys.rswacore/src/protocol/retry.rswacore/src/send.rswacore/src/stanza/business.rswacore/src/stanza/groups.rswacore/src/store/in_memory.rswacore/src/usync.rs
c4b6600 to
a94c632
Compare
Replace heap-allocated `String` with `CompactString` in the three
hottest allocation sites identified by heaptrack profiling:
- `NodeValue::String(String)` → `NodeValue::String(CompactString)`
- `NodeContent::String(String)` → `NodeContent::String(CompactString)`
- `Jid { user: String }` → `Jid { user: CompactString }`
CompactString stores strings ≤ 24 bytes inline (no heap allocation).
Since 90%+ of protocol attribute values and 100% of phone numbers
fit within this threshold, this eliminates the majority of per-node
heap allocations during `NodeRef::to_owned()`.
Measured results (heaptrack, real WhatsApp session):
- Peak heap: 7.90 MB → 5.15-5.24 MB (~34% reduction)
- Peak RSS: 32.57 MB → 28.28-28.57 MB (~13% reduction)
- The two largest allocation sites (Attrs FromIterator at 1.88 MB
and 750 KB) no longer appear in the top 15 consumers.
Additional changes:
- InMemoryBackend: amortized TTL-based eviction in store_sent_message
to prevent unbounded growth between periodic cleanup cycles.
- Benchmark example: use InMemoryBackend instead of SqliteStore.
- Re-export CompactString from wacore-binary and whatsapp-rust.
- Jid factory methods now take `impl Into<CompactString>` directly,
avoiding an intermediate String allocation.
- UsyncLidMapping fields changed to CompactString.
- Token lookup paths (send.rs, history_sync.rs, notification.rs)
borrow &str instead of allocating String where possible.
- content_as_string() returns CompactString to avoid heap alloc.
BREAKING CHANGE: `NodeValue::String` now wraps `CompactString`
instead of `String`. `NodeContent::String` likewise wraps
`CompactString`. `Jid.user` is now `CompactString` instead of
`String`. `content_as_string()` returns `Option<CompactString>`.
`UsyncLidMapping` fields are now `CompactString`. Jid factory
methods take `impl Into<CompactString>`. All existing `From<&str>`
and `From<String>` conversions still work. Code that pattern-matches
on these types and binds the inner value will see `CompactString`
(which derefs to `&str`). `HashMap<String, _>::get(&jid.user)` must
change to `.get(jid.user.as_str())`.
a94c632 to
b56d105
Compare
1 participant
Summary
StringwithCompactStringinNodeValue::String,NodeContent::String, andJid.userto eliminate heap allocations for short strings (≤ 24 bytes stored inline)InMemoryBackend::store_sent_messageto cap unbounded growthInMemoryBackendinstead ofSqliteStoreProfiling results
Measured with heaptrack against a real WhatsApp session (benchmark example via bartender mock server):
The two largest allocation sites from the baseline —
Attrs::FromIteratorat 1.88 MB and 750 KB (cloning(Cow<str>, NodeValue)tuples duringNodeRef::to_owned()) — no longer appear in the top 15 peak consumers.to_owned()still allocates (Vec containers, byte content, long strings > 24 chars), but it is no longer a top-tier memory problem.Why CompactString
CompactString(compact_strcrate) has the samesize_ofasString(24 bytes on x86_64) but stores strings ≤ 24 bytes inline — zero heap allocation. PoC benchmarks showed:NodeValue::Stringvalues are ≤ 24 chars (protocol values like"msg","pkmsg","text", timestamps)Jid.uservalues (phone numbers, 10-15 chars) fit inlineTags and attribute keys keep
Cow<'static, str>+intern_cow()— already zero-alloc for tokenized strings via static references.Breaking changes
NodeValue::StringwrapsCompactStringinstead ofStringNodeContent::StringwrapsCompactStringinstead ofStringJid.userisCompactStringinstead ofStringimpl Into<CompactString>instead ofimpl Into<String>Migration guide
Construction — all existing patterns still work:
Pattern matching —
CompactStringderefs to&str:When you need
String:CompactStringis re-exported from bothwacore_binary::CompactStringandwhatsapp_rust::CompactString.Test plan
cargo clippy --all --tests— zero warningscargo test -p wacore-binary -p wacore -p whatsapp-rust— all passingcargo check -p wacore-binary --all-features— serde feature verifiedSummary by CodeRabbit
New Features
Chores