perf(prekeys): stream prekey generation to cut the connect-time peak - #901
Conversation
upload_pre_keys_pass built the whole batch of PreKeyRecordStructures in a Vec before encoding them, so all 812 records — each owning two heap Vecs for its public/private key bytes — were resident at once alongside the public keys carried to the upload. That batch was the dominant controllable peak on the connect/registration path. Encode each record straight into the shared buffer and drop it immediately, keeping only its public key. The records are no longer held collectively, and the buffer is pre-sized by the 73-byte max record length to stay a single allocation. Wire output is unchanged.
|
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
WalkthroughInside ChangesPre-key batch encoding refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
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 `@src/prekeys.rs`:
- Around line 476-482: The constant MAX_RECORD_LEN is set to 73 bytes but the
actual maximum protobuf-encoded record size is 74 bytes. The calculation is: id
field requires 6 bytes maximum (1-byte tag plus 5-byte varint), publicKey field
requires 34 bytes (1-byte tag, 1-byte length, 32 bytes), and privateKey field
requires 34 bytes (same breakdown), totaling 74 bytes. Change the MAX_RECORD_LEN
constant from 73 to 74 to match the correct maximum record length and update the
associated comment to reflect the accurate byte breakdown for each field,
particularly correcting the id field from 5 bytes to 6 bytes to include the tag
byte.
🪄 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: 994f1ecb-fef5-40f7-984f-4c281d5fa2a1
📒 Files selected for processing (1)
src/prekeys.rs
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | send_message[20] |
5.6 ms | 5 ms | +10.84% |
| 👁 | Memory | send_and_receive[20] |
65.7 KB | 79 KB | -16.89% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/prekey-gen-stream-peak (c6017e9) with main (931a5d6)
MAX_RECORD_LEN pre-sizes the encode buffer to a single allocation; being only a capacity hint, derive it from the u32 id type (1-byte tag + 5-byte varint + two 34 B key fields = 74) rather than the tighter 24-bit id cap, so it is self-evidently correct without depending on the id-range invariant. No behavior change.
What
Follow-up to #900.
upload_pre_keys_passgenerates the one-time pre-key batch (812 keys by default, WA Web fidelity) in a blocking offload, then carries their public keys to the upload IQ. That offload built all the records up front — aVec<(u32, PreKeyRecordStructure)>where every record owns two heapVecs (public + private key bytes) — so the whole batch sat resident at once, alongside the public keys and the encoded buffer, just to be encoded and dropped immediately after.Encode each record straight into the shared batch buffer and drop it in the same iteration, keeping only its public key:
recordsVec — at most onePreKeyRecordStructureis live at a time.encoded_len()sum over the full materialized batch.Why
#900 cut allocation count/volume on the connect path by carrying generated public keys to the upload instead of reloading + decoding them, but it left the records batch fully materialized during generation — the dominant controllable contributor to
connect_to_ready's peak memory (the irreducible part being the 812 X25519 keygens themselves). Removing that batch's residency targets peak directly while preserving #900's allocation win.Verification
cargo fmt --all/cargo clippy -p whatsapp-rust --lib --tests— cleancargo test -p whatsapp-rust --lib prekeys— 16 tests pass, including thewindow_teststhat driveupload_pre_keys_passagainst a mock backend (upload-window, retry-single-key, collapse/regenerate)connect_to_readypeak-memory delta on this PR.Scope
Generation-phase only. The upload itself stays a single batched IQ (WA Web
WAWebUploadPreKeysJob), and the public keys + final encoded frame are still resident at send time — that floor is inherent to the protocol and small.Generated by Claude Code