perf: drop moka, use PortableCache as the sole in-process cache backend - #860
Conversation
moka was the single largest contributor to the release binary: 1.8 MiB of .text (15.8%), almost entirely per-cache-type monomorphization — its `do_run_pending_tasks` alone is emitted 84 times (710 KiB) across the ~15 distinct `Cache<K,V>` types the client instantiates. The client already abstracted its cache as `Cache<K,V>` over moka and the target-gated `PortableCache` (which has shipped on wasm32 all along and mirrors moka's API: capacity + TTL/TTI eviction, single-flight `get_with`). This makes PortableCache the sole backend on every target, removing the moka dependency entirely. Measured on the real release profile (fat LTO, codegen-units=1): stripped: 13.35 MiB -> 10.78 MiB (-2.56 MiB, -19.2%) .text: 11.31 MiB -> 8.87 MiB (-2.44 MiB) No feature or API change: every cache method is preserved and tested. The behavioural difference is eviction policy (PortableCache uses FIFO; moka used TinyLFU) and concurrency (one RwLock vs moka's sharded lock-free reads) — a throughput/hit-rate trade-off under high load, which the CI benchmarks will quantify. - remove the `moka` dependency and the `moka-cache` feature - `Cache<K,V>` is now `PortableCache<K,V>` unconditionally - rename `TypedCache::from_moka`/`Inner::Moka` -> `from_local`/`Inner::Local` - drop `moka-cache` from the e2e and bench-integration manifests so CI exercises and benchmarks the PortableCache path https://claude.ai/code/session_01GdWEj1tkYCJBtPtv97Aena
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoves the optional ChangesMoka cache removal and PortableCache standardization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
You want this to be robust — good. 🚥 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 |
The second wasm job built with --features moka-cache to prove the moka-on-wasm fallback to PortableCache; with moka and the moka-cache feature removed that feature no longer exists (cargo errors on it). The first step (no default features) already guards the wasm lib build, which now uses PortableCache unconditionally. https://claude.ai/code/session_01GdWEj1tkYCJBtPtv97Aena
![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: 4426bae17d
ℹ️ 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".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
174-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename the diagnostics headings to match PortableCache.
The doc text is generic now, but
MemoryDiagnostics::fmtstill printsMoka cachesheadings. That leaves user-facing output advertising a backend this PR removed.Proposed fix
- writeln!(f, "--- Moka caches (TTL-bounded) ---")?; + writeln!(f, "--- PortableCache (TTL-bounded) ---")?; ... - writeln!(f, "--- Moka caches (capacity-only) ---")?; + writeln!(f, "--- PortableCache (capacity-only) ---")?;🤖 Prompt for 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. In `@src/client.rs` around lines 174 - 239, MemoryDiagnostics::fmt prints user-facing headings that still say "Moka caches"; update those heading strings to reference the current backend "PortableCache" instead. In the MemoryDiagnostics::fmt implementation replace the two occurrences of "--- Moka caches (TTL-bounded) ---" and "--- Moka caches (capacity-only) ---" with "--- PortableCache caches (TTL-bounded) ---" and "--- PortableCache caches (capacity-only) ---" respectively so the diagnostics output matches the PortableCache backend.
🤖 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/cache_store.rs`:
- Around line 47-50: Add a deprecated public compatibility constructor named
from_moka that forwards to the new from_local: define pub fn from_moka<K,
V>(cache: Cache<K, V>) -> Self { Self::from_local(cache) } (or call Self {
inner: Inner::Local(cache) } as appropriate) and annotate it with
#[deprecated(note = "from_moka is deprecated; use from_local instead")] (and a
doc comment /// Deprecated: use TypedCache::from_local) so downstream users keep
working for one release while being guided to the new TypedCache::from_local
constructor.
---
Outside diff comments:
In `@src/client.rs`:
- Around line 174-239: MemoryDiagnostics::fmt prints user-facing headings that
still say "Moka caches"; update those heading strings to reference the current
backend "PortableCache" instead. In the MemoryDiagnostics::fmt implementation
replace the two occurrences of "--- Moka caches (TTL-bounded) ---" and "--- Moka
caches (capacity-only) ---" with "--- PortableCache caches (TTL-bounded) ---"
and "--- PortableCache caches (capacity-only) ---" respectively so the
diagnostics output matches the PortableCache backend.
🪄 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: f8a09222-4129-4bf7-8933-1bc6349610d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlsrc/cache.rssrc/cache_config.rssrc/cache_store.rssrc/client.rssrc/client/device_registry.rssrc/client/device_topology.rssrc/lib.rssrc/lid_pn_cache.rssrc/message/tests.rssrc/portable_cache.rssrc/retry.rstests/bench-integration/Cargo.tomltests/e2e/Cargo.tomltests/e2e/tests/device_cache.rstests/e2e/tests/memory_soak.rswacore/src/store/cache.rs
💤 Files with no reviewable changes (4)
- tests/bench-integration/Cargo.toml
- src/lib.rs
- tests/e2e/Cargo.toml
- Cargo.toml
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adc22219cd
ℹ️ 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".
Making PortableCache the sole native backend surfaced behaviours where it diverged from moka. Bring it to parity: - Monotonic TTL/TTI: expiry now uses wacore::time::Instant instead of the wall clock (now_millis), so a system-clock jump can no longer expire entries early — restoring moka's timer semantics. Notably keeps session_recreate_history's throttle backstop from being bypassed by a forward clock step. (Codex) - Reclaim single-flight init locks: get_with/get_with_by_ref now drop a key's init lock once no other caller holds it, instead of relying on run_pending_tasks (which the hot session-lock / chat-lane / dedup caches never call). Fixes an unbounded init_locks growth keyed by sender/chat/message-id. (Codex) - Reliable async clear(): added PortableCache::clear() that awaits the write lock; cleanup_connection_state and TypedCache::clear use it instead of the best-effort sync invalidate_all (which can skip the clear under contention and leave a stale ChatLane after reconnect). (Codex) - Diagnostics headings no longer say "Moka caches". (CodeRabbit) https://claude.ai/code/session_01GdWEj1tkYCJBtPtv97Aena
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/client.rs`:
- Line 216: Update the diagnostics heading string passed to writeln! at the call
that currently writes "--- In-process caches (TTL-bounded) ---" so it is
backend-neutral (e.g. "--- TTL-bounded caches ---"); locate the writeln!(f,
"...") call in src/client.rs (the Display/diagnostics formatting block that
emits cache buckets) and replace the literal with the new label so it no longer
implies "in-process" only.
🪄 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: 0f1e1129-8f05-4345-8897-caf0de4e8996
📒 Files selected for processing (4)
src/cache_store.rssrc/client.rssrc/client/lifecycle.rssrc/portable_cache.rs
Follow-ups: trimming the remaining
|
| Bucket | ~Size | What it is |
|---|---|---|
core::ptr::drop_in_place::<…> (drop glue) |
~471 KiB | generic drop glue instantiated for our types; ~117 KiB is waproto message types alone |
core::fmt (Debug/Display + format machinery) |
~382 KiB | derived Debug/Display, panic/format plumbing |
backtrace symbolization (gimli / addr2line / backtrace_rs / miniz_oxide) |
~168 KiB | DWARF parsing to symbolize panic backtraces |
core::slice::sort (quicksort copies) |
~106 KiB | one monomorphization per element type |
f64 → string |
~12 KiB | 2 instantiations of float_to_decimal_* |
Key takeaway: most of "std" is generic glue instantiated for our types, not std runtime itself — so the levers are mostly about our type/usage shape, not dropping std. Candidate follow-ups, ranked by value/risk:
A. Drop backtrace symbolization (~168 KiB) — production binary only
gimli + addr2line + miniz_oxide are linked purely to symbolize panic backtraces — dead weight in a stripped, panic = "abort" production binary that can't symbolize anyway. The Docker build already uses build-std, so adding -Z build-std-features=panic_immediate_abort (or a backtrace-less std) removes this whole bucket plus most of the panic-path core::fmt.
- Tradeoff: panics abort with no message/backtrace.
- Caveat: this rides on
build-std, which the binary-size CI proxy does not use, so it won't move the size gate — it only shrinks the real shipped artifact (likely the single biggest remaining win there).
B. Trim #[derive(Debug)] on large wire/proto types (part of the 382 KiB fmt) — shows in CI gate
A large share of core::fmt is Debug glue for big waproto / wacore-binary node types. Where a type is only Debug-formatted incidentally (or never in a compiled path), #[cfg]-gating or dropping the derive removes its fmt monomorphization. Needs an audit of which Debug impls are actually reachable; tradeoff is log/debug ergonomics.
C. Box more large, rarely-present proto submessages (~117 KiB waproto drop glue + matching Clone) — shows in CI gate
waproto/build.rs already boxes a few hot ones (HistorySyncMsg.message, WebMessageInfo.message, …). Extending config.boxed(...) to other large/optional submessages shrinks the inline drop/clone glue carried by Message. Wire-compatible, low risk; pick fields by size/optionality. Probably the best size/risk ratio for the gated metric.
D. Collapse slice::sort monomorphizations (~106 KiB)
Several quicksort copies exist for distinct element types ((u32,u32), (usize,usize), Cow<str>, …). Sorting by an extracted key / sorting indices once reduces distinct instantiations. Medium effort, modest payoff.
E. Kill the two f64 formatting sites (~12 KiB) — cheap
A {} / {:.N} on an f64 pulls in float_to_decimal_*. Cheap to find and reformat as integer/fixed-point.
Suggested order: A for the shipped artifact (biggest, lowest-risk, but invisible to the CI gate), then C and E for quick gated wins, then B/D as larger audits. Happy to spike any of these as separate PRs — none belong in this one, which stays a focused moka removal.
(Numbers from cargo bloat --release --bin whatsapp-rust on the PR head; "std" here is cargo-bloat's attribution bucket, which folds in core/generic glue.)
Generated by Claude Code
The TTL-bounded bucket holds TypedCache instances that may be store-backed (e.g. Redis) via CacheConfig::from_store, so 'In-process caches' was inaccurate. Label by lifetime policy instead. (CodeRabbit) https://claude.ai/code/session_01GdWEj1tkYCJBtPtv97Aena
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7abc9549be
ℹ️ 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".
invalidate_entries_for_device enumerated cached groups via the best-effort iter() (try_read spin that returns an empty snapshot under write contention), so under group-send traffic a device removal could skip every invalidation and leave a stale has_key=true — dropping a later SKDM fanout for a re-added device. Added PortableCache::snapshot_entries() (awaited read lock) and use it here. (Codex) https://claude.ai/code/session_01GdWEj1tkYCJBtPtv97Aena
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a60ef9759b
ℹ️ 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".
| if let Some(v) = self.get(key).await { | ||
| v | ||
| } else { | ||
| self.insert_and_return(owned_key.clone(), init.await).await |
There was a problem hiding this comment.
If a caller is cancelled while this init.await is still loading a value, for example an application times out a group send while SenderKeyDeviceCache::get_or_init is reading the DB, execution never reaches the new reclaim_init_lock call below. The cloned Arc is dropped but the init_locks map still keeps the key, and these hot get_with_by_ref caches do not run run_pending_tasks, so high-cardinality cancelled sends leak one mutex per group until that same key is retried. Fresh evidence is that the normal-path reclaim was added only after this await.
Useful? React with 👍 / 👎.
2 participants
What
Removes the
mokadependency and makes the already-presentPortableCachethe sole in-process cache backend on every target.Why
mokawas the single largest contributor to the release binary — 1.8 MiB of.text(15.8%) — and it's almost entirely per-cache-type monomorphization, not intrinsic library code. Itsdo_run_pending_tasksalone is emitted 84 times (710 KiB) across the ~15 distinctCachetypes the client instantiates; each new typed cache drags in moka's full generic machinery (~100 KiB+).The client already abstracts its cache as
Cacheinsrc/cache.rs, switching between moka andPortableCacheby target/feature.PortableCachehas shipped on wasm32 all along, mirrors moka's API (capacity + TTL/TTI eviction, single-flightget_with/get_with_by_ref), and has its own test suite. This PR simply makes it the only backend.Measured impact
Real release profile (fat LTO,
codegen-units=1,panic=abort, strip); confirmed by the binary-size CI gate againstmain:.text(The net delta exceeds moka's own 1.8 MiB line because dropping moka also removes its transitive deps — crossbeam-channel/epoch, quanta, part of uuid — and unlocks further LTO savings;
PortableCacheadds back only a few KiB.) CodSpeed reports no performance change (172 benchmarks untouched).Trade-off (for reviewers / CI to weigh)
No feature, API, or correctness change — every cache method is preserved and the full test suite passes (
cargo test --workspace --exclude e2e-tests, fmt +clippy --all --testsclean). After the review hardening below, the only remaining behavioural differences vs moka are:PortableCacheuses FIFO; moka used TinyLFU (better hit rate under skewed access).RwLockper cache vs moka's sharded, lock-free reads (more contention under heavy concurrency).For typical bot/single-account workloads this is unlikely to matter; for very high-throughput multi-tenant use it could. The integration benchmarks (CodSpeed) and the binary-size gate on this PR are the right place to quantify both ends.
Changes
mokadependency and themoka-cachefeature (Cargo.toml,Cargo.lock), including the wasm build step that exercised the old fallback.Cacheis nowPortableCacheunconditionally (src/cache.rs);portable_cachemodule is no longer cfg-gated.TypedCache::from_moka→from_localandInner::Moka→Inner::Local.moka-cachefrom the e2e and bench-integration manifests so CI exercises and benchmarks thePortableCachepath.mokareferences in doc comments and diagnostics.PortableCache hardening (from review)
Making PortableCache the sole native backend surfaced a few places where it diverged from moka; brought to parity in follow-up commits:
wacore::time::Instantinstead of the wall clock, so a system-clock jump can't expire entries early. This restores moka's timer semantics (the native path used moka's monotonic timers before this PR), and keepssession_recreate_history's throttle backstop from being bypassed.get_with/get_with_by_refnow drop a key's init lock once no other caller holds it, instead of relying onrun_pending_tasks(which the hot session-lock / chat-lane / dedup caches never call). Fixes unboundedinit_locksgrowth keyed by sender/chat/message-id.clear()— addedPortableCache::clear()that awaits the write lock;cleanup_connection_stateandTypedCache::clearuse it instead of the best-effort syncinvalidate_all, which could skip the clear under contention and leave a staleChatLaneafter reconnect.TypedCacheinstances).Follow-ups (not in this PR)
stdis now the largest remaining attribution (1.13 MiB). A separate comment on this PR breaks it down and lists candidate follow-ups (drop backtrace symbolization viabuild-std/panic_immediate_abort, trimDebugderives, box more large proto submessages, etc.). None belong here — this stays a focused moka removal.https://claude.ai/code/session_01GdWEj1tkYCJBtPtv97Aena
Generated by Claude Code
<img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">