perf: single-buffer ProtocolAddress + reusable hot-loop address construction - #518
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced per-iteration address allocations with preallocated buffers and in-place Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReusableAddr as ReusableAddrBuf
participant SessionStore
participant PrekeyProcessor
participant IdentityStore
participant Encryptor
Client->>ReusableAddr: make_reusable_protocol_address() / reset_protocol_address(&reusable_addr)
Client->>SessionStore: load_session(&reusable_addr)
alt session exists
SessionStore-->>Client: session
else session missing
Client->>PrekeyProcessor: process_prekey_bundle(&reusable_addr, ...)
PrekeyProcessor->>IdentityStore: save_identity(&reusable_addr, ...)
PrekeyProcessor-->>Client: new_session
end
Client->>Encryptor: message_encrypt(&reusable_addr, ...)
Encryptor-->>Client: encrypted_message / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 2s —— View job Code Review:
|
![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
🤖 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 1518-1521: The code is allocating a full ProtocolAddress via
make_reusable_protocol_address() and then calling write_protocol_address_to(),
which builds the ProtocolAddress and its cached display unnecessarily for
session_lock_for()/session_mutexes_for(); change to a string-only helper that
returns or writes just the lock-key string (e.g.,
make_reusable_protocol_lock_key or write_protocol_address_key_to) and use that
buffer when calling session_lock_for(&buf). Replace calls to
make_reusable_protocol_address() + write_protocol_address_to() with the new
string-only helper so you avoid constructing ProtocolAddress and its cached
display on the hot path.
In `@wacore/src/types/jid.rs`:
- Around line 23-34: make_reusable_protocol_address() still causes a
reallocation because ProtocolAddress::new builds its internal display from an
empty name sized only for ".0"; add a capacity-aware constructor on
ProtocolAddress (e.g., ProtocolAddress::with_capacity or new_with_capacity) that
preallocates the internal display/string to the expected full size (derive
capacity from SIGNAL_ADDRESS_CAPACITY plus room for "." and SIGNAL_DEVICE_ID),
update ProtocolAddress::with_capacity(...) to initialize fields without growing
on first reset, and call that new constructor from
make_reusable_protocol_address() instead of ProtocolAddress::new so
reset_protocol_address() can avoid allocations on first use.
🪄 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: b88dd0df-a5d2-4545-b98a-581b26521e48
📒 Files selected for processing (4)
src/send.rswacore/libsignal/src/core/address.rswacore/src/send.rswacore/src/types/jid.rs
Benchmark Results3 improvement(s):
56 unchanged benchmark(s)
|
4667310 to
6a3650c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/types/jid.rs (1)
151-183: 🧹 Nitpick | 🔵 TrivialThe new agent-path coverage is still self-referential.
The added cases include
"100000000000001.1:75@lid", but the expected side callsto_protocol_address*(), which uses the same formatter under test. If agent handling regresses, these assertions still pass. Please add one literal expected-string assertion for an agent-bearing JID.🧪 Suggested test hardening
#[test] + fn test_agent_is_preserved_in_signal_and_protocol_addresses() { + let jid = Jid::from_str("100000000000001.1:75@lid").unwrap(); + assert_eq!(jid.to_signal_address_string(), "100000000000001.1:75@lid"); + assert_eq!(jid.to_protocol_address_string(), "100000000000001.1:75@lid.0"); + } + + #[test] fn test_reset_protocol_address_matches_fresh() { let jids = [ "123456789@lid", "123456789:33@lid", "100000000000001.1:75@lid",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/types/jid.rs` around lines 151 - 183, The tests use to_protocol_address() / to_protocol_address_string() (and reset_protocol_address()/make_reusable_protocol_address()) to compute both actual and expected values for agent-bearing JIDs, making the check self-referential; add a literal expected-string assertion for at least one agent-bearing JID (e.g. the "100000000000001.1:75@..." case) so the test compares the formatter output to a hard-coded expected protocol-address string rather than calling to_protocol_address*() for the expected value.
🤖 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/libsignal/src/core/address.rs`:
- Around line 310-318: The display buffer is under-reserved: change
ProtocolAddress::with_capacity to allocate for the full ".{device_id}" suffix
(dot plus the decimal digits of device_id) instead of the fixed +2; compute the
decimal width of device_id (e.g., number of digits via to_string().len() or a
small digit-count routine) and use capacity + 1 + decimal_digits when
constructing display: String::with_capacity(capacity + 1 + decimal_digits), so
the first reset_name() remains zero-allocation.
---
Outside diff comments:
In `@wacore/src/types/jid.rs`:
- Around line 151-183: The tests use to_protocol_address() /
to_protocol_address_string() (and
reset_protocol_address()/make_reusable_protocol_address()) to compute both
actual and expected values for agent-bearing JIDs, making the check
self-referential; add a literal expected-string assertion for at least one
agent-bearing JID (e.g. the "100000000000001.1:75@..." case) so the test
compares the formatter output to a hard-coded expected protocol-address string
rather than calling to_protocol_address*() for the expected value.
🪄 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: 1f5a9c9c-c24a-43a0-9d9a-98dd1e18ee47
📒 Files selected for processing (4)
src/send.rswacore/libsignal/src/core/address.rswacore/src/send.rswacore/src/types/jid.rs
6a3650c to
ff514e5
Compare
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/src/types/jid.rs (1)
133-199:⚠️ Potential issue | 🟠 MajorUse obviously synthetic JIDs in these tests.
Several fixtures here are formatted like live phone/JID identifiers (
5511999887766...). Please replace them with unmistakably fake values before merging so we do not check plausible PII into the repo. As per coding guidelines, "Applies to **/test.rs : Never use real PII (phone numbers and JIDs) in test code; use fictitious values instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/types/jid.rs` around lines 133 - 199, Tests use plausible real phone-style JIDs (e.g. "5511999887766..."); replace all such values with clearly synthetic identifiers in the tests (e.g. "5550000000000@s.whatsapp.net" and matching variants like "5550000000000:33@s.whatsapp.net" or other obviously fake long ids) and update the expected strings in assertions accordingly (affecting test_signal_address_string_phone, test_protocol_address_string_matches_to_string, test_reset_protocol_address_matches_fresh, and test_write_functions_dry as well as any helper like make_reusable_protocol_address uses); keep the same formatting/structure (e.g. .to_signal_address_string() -> "5550000000000@c.us", protocol address strings -> "id@lid.0") so the logic is unchanged, only the input/output literals are replaced with synthetic values.
♻️ Duplicate comments (1)
wacore/libsignal/src/core/address.rs (1)
317-345:⚠️ Potential issue | 🟠 Major
with_capacity()leaves the cached key invalid until the first reset.This constructor returns a
ProtocolAddresswithdisplay == "", butDisplay,Eq,Ord, andHashall read that field. Any accidental use beforereset_with()will therefore hit the empty session key, and two fresh values with differentdevice_ids compare equal.♻️ Minimal fix
pub fn with_capacity(capacity: usize, device_id: DeviceId) -> Self { // display = "{name}.{device_id}" — reserve for dot + decimal digits let suffix_len = 1 + digit_count(u32::from(device_id)); - ProtocolAddress { + let mut address = ProtocolAddress { name: String::with_capacity(capacity), device_id, display: String::with_capacity(capacity + suffix_len), - } + }; + address.rebuild_display(); + address }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/core/address.rs` around lines 317 - 345, with_capacity() currently leaves display empty which makes Display/Eq/Ord/Hash observe an incorrect cached key; fix by populating the cached display before returning: after constructing the ProtocolAddress (with name String::with_capacity and display String::with_capacity(...)) call rebuild_display() (or otherwise format/push '.' + device_id into display) so the returned value has a valid display string consistent with reset_with(), ensuring ProtocolAddress::with_capacity, rebuild_display, reset_with and consumers of Display/Eq/Ord/Hash behave correctly.
🤖 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/src/types/jid.rs`:
- Around line 133-199: Tests use plausible real phone-style JIDs (e.g.
"5511999887766..."); replace all such values with clearly synthetic identifiers
in the tests (e.g. "5550000000000@s.whatsapp.net" and matching variants like
"5550000000000:33@s.whatsapp.net" or other obviously fake long ids) and update
the expected strings in assertions accordingly (affecting
test_signal_address_string_phone,
test_protocol_address_string_matches_to_string,
test_reset_protocol_address_matches_fresh, and test_write_functions_dry as well
as any helper like make_reusable_protocol_address uses); keep the same
formatting/structure (e.g. .to_signal_address_string() -> "5550000000000@c.us",
protocol address strings -> "id@lid.0") so the logic is unchanged, only the
input/output literals are replaced with synthetic values.
---
Duplicate comments:
In `@wacore/libsignal/src/core/address.rs`:
- Around line 317-345: with_capacity() currently leaves display empty which
makes Display/Eq/Ord/Hash observe an incorrect cached key; fix by populating the
cached display before returning: after constructing the ProtocolAddress (with
name String::with_capacity and display String::with_capacity(...)) call
rebuild_display() (or otherwise format/push '.' + device_id into display) so the
returned value has a valid display string consistent with reset_with(), ensuring
ProtocolAddress::with_capacity, rebuild_display, reset_with and consumers of
Display/Eq/Ord/Hash behave correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db55d47b-a4e1-4406-bdb8-2a301ae948e2
📒 Files selected for processing (4)
src/send.rswacore/libsignal/src/core/address.rswacore/src/send.rswacore/src/types/jid.rs
ff514e5 to
cabed27
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 3m 51s —— View job Code Review:
|
| # | Issue | Status |
|---|---|---|
| 1 | display not pre-allocated in make_reusable_protocol_address() |
Fixed — with_capacity now sizes buffer with digit_count() |
| 2 | Wasted ProtocolAddress allocation in session_mutexes_for |
Fixed — make_address_buffer() added, used at src/send.rs:1518 |
| 3 | Dotted LID test coverage removed | Fixed — "100000000000001.1:75@lid" in both test_protocol_address_string_matches_to_string and test_reset_protocol_address_matches_fresh with hard-coded expected strings |
Nits
1. Dead initial write in with_capacity — wacore/libsignal/src/core/address.rs:328
pub fn with_capacity(capacity: usize, device_id: DeviceId) -> Self {
use std::fmt::Write;
let suffix_len = 1 + digit_count(u32::from(device_id));
let mut buf = String::with_capacity(capacity + suffix_len);
write!(buf, ".{device_id}").unwrap(); // ← cleared on first reset_with()
Self { buf, name_len: 0, device_id }
}reset_with() calls self.buf.clear() before writing, so the initial .0 is immediately discarded on first use. This leaves the address in an odd intermediate state where name() returns "" and as_str() returns ".0". Since the doc says "Call reset_with() to fill" and all callers follow that contract, this isn't a bug — but the write is dead work. You could just leave buf empty:
let buf = String::with_capacity(capacity + suffix_len);
Self { buf, name_len: 0, device_id }2. Double mapped_server call in to_signal_address_string / to_protocol_address_string — wacore/src/types/jid.rs:94-110
Both methods call mapped_server() once for the capacity hint, then write_signal_address_to() calls it again internally. It's just a string comparison so the cost is negligible, but if you wanted to tighten it, you could pass the pre-computed server into the write function or just use SIGNAL_ADDRESS_CAPACITY for the hint (like the reusable path does).
3. test_write_functions_dry is self-referential — wacore/src/types/jid.rs:194-203
write_signal_address_to(&jid, &mut buf);
assert_eq!(buf, jid.to_signal_address_string()); // delegates to same functionThis only validates that the public wrapper calls the helper correctly (round-trip consistency), not that the output is correct. Consider adding one hard-coded expected value:
write_signal_address_to(&jid, &mut buf);
assert_eq!(buf, "15550000001@c.us");Correctness notes
- The single-buffer design is sound.
name()→&buf[..name_len],as_str()→&buf— no ambiguity since device_id is always a decimal integer (no dots), so"{name}.{device_id}"is always unambiguously parseable. Eq/Hash/Ordall operate on the fullbufstring, which is correct — same(name, device_id)always produces the samebuf.- The 40+ cold-path
to_protocol_address()call sites acrosssrc/message.rs,src/features/signal.rs,src/client/sessions.rs, etc. all benefit from the reduced struct size (2 Strings → 1) automatically. - The
encrypt_for_deviceshot loop correctly reuses oneProtocolAddressacross all four per-device phases (LID check, direct check, prekey processing, encryption).
LGTM — nits are all optional.
DRY the address formatting: extract write_signal_address_to() as the core helper, make write_protocol_address_to() and all JidExt methods delegate to it. Replace magic strings with existing constants (DEFAULT_USER_SERVER, LEGACY_USER_SERVER). Add ProtocolAddress::reset_name() for in-place string rewrite. Add DeviceId::new() const constructor. Add make_reusable_protocol_address() factory for hot-loop use. Add SIGNAL_ADDRESS_CAPACITY (64) and SIGNAL_DEVICE_ID constants. In encrypt_for_devices(), reuse one buffer + ProtocolAddress across all 4 hot loops (LID session check, direct session check, prekey processing, message encryption). For a 100-device group, this eliminates ~200 String allocations per message send. Buffer capacity of 64 bytes based on real WhatsApp logs showing max signal address length of 53 chars.
cabed27 to
ed0b21a
Compare
jlucaso1
changed the title
1 participant
Summary
Single-buffer ProtocolAddress
Collapsed
ProtocolAddressfrom two Strings (name+display) into one (buf+name_len). The buffer stores"{name}.{device_id}"and accessors are zero-cost slices:name()→&buf[..name_len]as_str()→&bufnew(name, device_id)reset_with()clone()All 40+ cold-path
to_protocol_address()call sites benefit automatically.DRY address formatting
write_signal_address_to()is the single core helper — all address functions delegate to itappend_device_suffix()shared bynew,reset_with— fast path for device_id=0 (push_str(".0")instead ofwrite!)make_address_buffer()/make_reusable_protocol_address()factories — no magic numbersSIGNAL_ADDRESS_CAPACITY(64) andSIGNAL_DEVICE_IDconstants replace magic valuesDEFAULT_USER_SERVER/LEGACY_USER_SERVERreplace magic strings inmapped_server()Hot loop optimization
In
encrypt_for_devices(), one reusableProtocolAddressacross all 4 per-device loops:reset_with(|name| write_signal_address_to(jid, name))writes directly into the buffer — single write pass, no intermediate copy. For a 100-device group, eliminates hundreds of String allocations per send.Buffer capacity
64 bytes based on real WhatsApp logs (
docs/real-whatsapp-log.json) showing max signal address length of 53 chars.Lab results (experiments/heaptrack-impact-lab)
Test plan
cargo fmt --allcargo clippy --all --testscargo test --all --exclude e2e-teststest_reset_protocol_address_matches_fresh— hard-coded expected strings including dotted LIDtest_write_functions_dry— hard-coded expected output, not self-referentialtest_protocol_address_string_matches_to_string— dotted LID, phone, device variantsSummary by CodeRabbit