perf(send): hash the participant list from one arena instead of a String per device - #822
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR extracts Jid AD-string rendering into a reusable ChangesAD String Optimization for Participant Hashing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Look, this is solid work. You're eliminating wasteful per-device allocations in the phash computation—that matters when you're dealing with real group scale. The arena pattern is clean: single buffer, byte ranges, sort semantically on the slices themselves, feed the hasher in sorted order. That's how you avoid garbage and keep things moving. The But make sure the arena string stays in scope for the entire sort-and-hash sequence. Any borrow issues hiding in there will bite you in production. And that test set with duplicates and prefix cases—keep that sharp. Edge cases in sort order are where correctness bugs hide. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Benchmark Results2 improvement(s):
65 unchanged benchmark(s)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/messages.rs`:
- Around line 977-990: The test currently only verifies the 6-byte
truncated/base64 "public phash" string; change it to also compute and assert the
full 32-byte SHA-256 digest before formatting: generate the full digest via
Sha256 (the variable digest produced from hasher.finalize()), compare that full
digest bytes (or hex) against the expected full-digest test vector, then
separately preserve the existing base64 "2:" formatting (expected string) and
assert MessageUtils::participant_list_hash(&devices) still matches the public
string form; update the test to split digest generation (hasher/digest) from the
formatting step (expected string) and add the full-digest equality assertion
against the canonical 32-byte 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1314e1e6-f864-427a-b10a-5b53a4b0b244
📒 Files selected for processing (2)
wacore/binary/src/jid.rswacore/src/messages.rs
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.
Problem
participant_list_hashruns over the full device set on every group send (and the phash now travels on every send since #678). It materialized aVec<String>with oneto_ad_string()heap allocation per device and sorted the Strings: for an 800-device group that is 801 discarded allocations per message, pure allocator churn on the send hot path.Change
Format every device into one shared arena
Stringvia a newJid::push_ad_to(the append-style sibling ofto_ad_string, which now delegates to it), and sort lightweight(start, end)range views over the arena. Sorting the slices is the same lexicographic order as sorting the individual Strings, so the hashed concatenation is byte-identical: the pinned cross-impl vectors (phash_crosscheck_vectors, locked against whatsmeow and WA Web server behavior) pass unchanged.Benchmark
Counting-allocator measurement (release, 800 devices, 1000 iterations, bench not committed):
Wall time of the full function is dominated by the SHA-256 over ~26KB and stays flat; the win is the 798 fewer allocations per group send.
Tests
New
phash_arena_matches_per_string_reference: locks the arena output against an inline reference implementation (String per device, sorted, concatenated, hashed) over a mixed set with unsorted input, duplicates, agents, multiple servers, and the "111" vs "1110" prefix-ordering edge where a slice-comparator bug would diverge. The existing pinned vectors (phash_crosscheck_vectors) andvalidate_bcl_hashtests pass unchanged.cargo fmt --allcargo clippy --all-targets -- -D warningscargo test -p wacore(995 passing)cargo test -p whatsapp-rust --lib(753 passing)cargo test -p wacore-binary(98 passing)Breaking
None.
to_ad_stringkeeps its exact output;push_ad_tois additive.