Conversation
…hashing group_delivery_receipts builds a per-group dedup HashMap by walking the full receipt batch. On reconnect that batch can carry a large offline backlog, so the map rehashed repeatedly as it grew. Pre-size it to the receipt count; it is dropped at function end, so the transient over-allocation under heavy grouping never outlives the call. `groups` stays growth-sized since it is returned and grouping can shrink it sharply.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIn ChangesHashMap Pre-allocation in Receipt Grouping
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
🚥 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 |
![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: 40c868ab8e
ℹ️ 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".
| // over-allocation when grouping is heavy never outlives the call. `groups` is | ||
| // returned and grouping can shrink it sharply, so it's left to grow. | ||
| let mut index: std::collections::HashMap<Key, usize> = | ||
| std::collections::HashMap::with_capacity(infos.len()); |
There was a problem hiding this comment.
In the offline flush path, flush_offline_receipts drains the entire replay buffer and then calls this grouping helper; when a reconnect replays many messages from the same chat/sender, groups.len() can be 1 while infos.len() is thousands. This line now forces the dedup map to allocate for every message before discovering they collapse into one group, whereas the previous lazy map stayed small in that common aggregate case, so a large catch-up can burn or OOM transient memory before any receipts are sent. Prefer leaving the map lazy or reserving only as groups are added.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
2 participants
What
group_delivery_receiptsaggregates buffered offline messages into one<receipt>per group (WA Web'ssendAggregateOfflineReceipts). It walks the full receipt batch building a per-group dedupHashMap<Key, usize>keyed by the receipt-level attrs (to/ participant / type / recipient).The map was
HashMap::new(), so on reconnect — when the batch can carry a large offline backlog — it rehashed repeatedly as it grew (each resize re-inserts every live entry). Pre-size it to the receipt count (infos.len()), an upper bound on the distinct keys, so it reaches its final capacity in one allocation.The companion
groupsVec is deliberately left growth-sized: it's the returned value and grouping can shrink it far belowinfos.len()(many messages collapse into a few chats), so pre-sizing it to the input count would leave an over-allocated Vec alive past the call. The map has no such issue — it's dropped when the function returns, so a heavy-grouping over-allocation is purely transient.Why it's correct
Pure pre-sizing:
HashMap::with_capacity(n)is behavior-identical toHashMap::new()— same entries, same lookup/insert semantics, only the initial capacity differs. No logic, ordering, or key handling changes, andinfos.len()is always ≥ the number of distinct keys, so the hint never under-sizes.Tests
No behavior to pin — covered by the existing receipt suite.
cargo test -p whatsapp-rust --lib receipt(81) green;cargo fmt --all+cargo clippyclean.Impact
Removes the incremental rehashing of the dedup map on the reconnect / offline-aggregation path; the win scales with backlog size and is nil for the common small-batch case (the map already fit). Modest and allocation-side — the rest of the path was already lean.