perf(history-sync): size the secret-record accumulator by sampled density - #945
Conversation
…sity The msg_secret_records Vec grew by plain doubling, which copies about twice the final byte volume through realloc per chunk; on a measured 300x20 history sync that site alone moved 10 MB across 48 reallocs (37% of the client's total allocation churn) and cost 5.6% of the decode on the realloc plus memcpy path. A full pre-count pass was already tried and removed (#690): it re-scanned the whole blob and cost ~2.5% of the decode. Instead, once the accumulator reaches 128 records, extrapolate the record density observed so far to the blob's estimated decompressed size (zlib input ratio) and reserve once. O(1), no second pass; a low estimate just resumes doubling, and the clamp bounds over-allocation to ~2 MB. Sampling at 128 matters: extrapolating from the first conversation alone overshot to the clamp and doubled the transient peak on the same workload.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a ChangesDecompression Progress and Capacity Reservation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Alright, listen up — this is exactly the kind of ruthless efficiency I want to see. We measured. We extrapolated. We pre-reserved capacity so the vector isn't reallocating like it's 2004 dial-up. Small diff, high signal. Ship it, but I want the numbers to be right — no hand-waving on that ratio math. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: a3c0cea609
ℹ️ 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".
|
| Filename | Overview |
|---|---|
| wacore/binary/src/zlib_pool.rs | Adds compressed_progress() -> (usize, usize) exposing (in_pos, input.len()) so callers can compute the compression ratio mid-stream. Minimal, correct addition with no impact on existing inflate logic. |
| wacore/src/history_sync.rs | Adds parsed_bytes() and estimated_total_out() helpers to FieldWalker, then uses them for a one-shot density-based reserve once 128 secret records are accumulated. Logic is sound: overflow is bounded by saturating_mul, the clamp keeps over-allocation to ≤2 MB, and underestimates safely fall back to doubling. Two new targeted tests cover the key edge cases. Style-only nit: some new comments explain what rather than why (AGENTS.md). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[next_field CONVERSATIONS] --> B[extract_conversation_fields push secret records]
B --> C{density_reserved?}
C -- yes --> G[continue loop]
C -- no --> D{records >= 128?}
D -- no --> G
D -- yes --> E[density_reserved = true, estimated = records x estimated_total_out / parsed]
E --> F[estimated = estimated.min 16384, reserve estimated minus records]
F --> G
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[next_field CONVERSATIONS] --> B[extract_conversation_fields push secret records]
B --> C{density_reserved?}
C -- yes --> G[continue loop]
C -- no --> D{records >= 128?}
D -- no --> G
D -- yes --> E[density_reserved = true, estimated = records x estimated_total_out / parsed]
E --> F[estimated = estimated.min 16384, reserve estimated minus records]
F --> G
Reviews (2): Last reviewed commit: "docs(history-sync): update the accumulat..." | Re-trigger Greptile
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/history_sync.rs`:
- Around line 116-138: Add unit tests for the arithmetic in history_sync’s
`parsed_bytes` and `estimated_total_out` helpers so the ratio extrapolation and
guard behavior stay locked down. Use `FieldWalker` (or the same decompression
path exercised by `compressed_progress`, `total_out`, and `available`) to cover
a fully drained stream where `estimated_total_out()` equals `total_out()`, and a
partial-progress case where it extrapolates correctly from known compressed
input. Also include an edge-case assertion for zero compressed progress to
ensure the `in_pos == 0` branch remains safe.
- Around line 304-316: In the density estimate logic inside the history sync
reservation block, `parsed` is already clamped with `.max(1)`, so the
`checked_div(...).map_or(records, ...)` fallback is unreachable and misleading.
Simplify the estimate calculation in the `density_reserved` branch by removing
the unnecessary `checked_div` fallback path and keeping the `records`,
`walker.parsed_bytes()`, `walker.estimated_total_out()`, and
`RECORD_RESERVE_CAP` flow intact.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: de335061-2d4d-42df-819d-9e644e6bae57
📒 Files selected for processing (2)
wacore/binary/src/zlib_pool.rswacore/src/history_sync.rs
Merging this PR will improve performance by 54.77%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wacore/src/history_sync.rs">
<violation number="1" location="wacore/src/history_sync.rs:306">
P2: The density denominator (`parsed_bytes()`) may exclude the current conversation's payload because `pending_payload` documentation states the yielded field's bytes stay buffered until the next `next_field()` call — meaning they remain part of `available()` and are subtracted from `total_out()` in `parsed_bytes()`. Meanwhile, `records` already includes secrets extracted from that conversation. This inflates the apparent record density, causing the estimate to overshoot (potentially hitting `RECORD_RESERVE_CAP` of 16384) and reserving up to ~2 MB for blobs that may contain far fewer records.
Consider computing the sample after the field is fully consumed, or adding the current field's span to the parsed-byte count before using it as the denominator.</violation>
<violation number="2" location="wacore/src/history_sync.rs:311">
P3: Since `parsed` is guaranteed to be at least 1 (from `.max(1)` above), the `checked_div(parsed)` call can never return `None`, making the `map_or(records, ...)` fallback unreachable. This reads as if there's a live division-by-zero path when there isn't one. Consider using a plain division instead, or adding a comment noting the guard is purely defensive.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
parsed_bytes excluded the whole unparsed buffer, but the field most recently yielded by next_field stays buffered until the following call even though its records were already extracted. On a blob whose first conversation alone reaches the sample threshold the denominator was near zero, so the estimate clamped and over-reserved ~2 MB. Count the pending field, drop the unreachable checked_div fallback, and pin both behaviors with tests (single-big-conversation reserve, estimator exactness after a full drain).
…serve The initializer still described plain growth as the strategy; it now points at the density-based reserve below and keeps the pre-count history.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca8030d21
ℹ️ 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".
Resolve the history_sync.rs conflicts by taking main's side (the #945 density reserve comment and the #947 Option-returning read_varint), then adapt the cross-pollinated tests: the #945 single-big-conversation test builds MessageKey via buffa::MessageField::some, and the branch's varint overflow test asserts is_none() instead of a Result error.
1 participant
Sizes the history-sync secret-record accumulator from a sampled record density instead of growing it by plain doubling.
Problem
msg_secret_recordsgrew from empty by repeated doubling, which copies roughly 2x the final byte volume throughreallocper chunk. Profiling the history-sync scenario (300 convs x 20 msgs, 4 chunks) showed this single site was the client's largest allocation-churn source, and CodSpeed's flamegraph attributed 5.6% ofbench_process_history_syncto the resulting realloc+memcpy chain.A full pre-count pass was already tried and removed in #690: it re-scanned the whole blob and cost ~2.5% of the decode while over-allocating. This takes a third route: once the accumulator reaches 128 records, extrapolate the density observed so far to the blob's estimated decompressed size (from the zlib input ratio) and
reserveonce. O(1), no second pass; an under-estimate just resumes doubling, and the clamp bounds over-allocation to ~2 MB. The 128-record sample is load-bearing: extrapolating from the first conversation alone overshot to the clamp and doubled the transient peak on the same workload.Before / after
Production harness (whatsapp-benchs
history-sync, 300x20, MODE=dhat):Micro (
bench_process_history_sync, 500x40 fixture, valgrind dhat on the record site): 4.98 MB / 13 allocs -> 1.89 MB / 8 allocs.Wall time on the same bench is neutral-to-slightly-better under an interleaved A/B (baseline ~8.1 ms vs patched ~7.7 ms median-of-3, within machine noise); the deterministic instruction-count delta will show in this PR's CodSpeed run.
Regressions considered