Conversation
Reduce default capacities that were sized for extreme scenarios but waste memory in typical single-client usage: - Transport event channel: keep at 64 (user-tested at 5K msg/sec) - NoiseSocket send job channel: 32 -> 8 (serialized sender, <1ms/job) - Signal cache max entries: 10,000 -> 2,000 (typical ~200 contacts) - session_locks: 10,000 -> 2,000 - chat_lanes: 5,000 -> 1,000 - device_registry: 5,000 -> 1,000 - lid_pn_cache: 10,000 -> 2,000 - retried_group_messages: 2,000 -> 500 - message_retry_counts: 1,000 -> 500 - pdo_pending_requests: 500 -> 200 - enc_buf initial capacity: 4,096 -> 1,024 All capacities are still configurable via CacheConfig. Moka caches grow lazily so these are eviction ceilings, not pre-allocations. The bounded channels (transport, send_job) do pre-allocate ring buffers.
📝 WalkthroughWalkthroughReduced default cache capacities and channel/backpressure sizes, removed a PDO cache helper, switched some Tokio runtimes to current-thread, and made Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/cache_config.rs`:
- Around line 174-175: The config field pdo_pending_requests (type
CacheEntryConfig) has its default capacity changed to 200 but the PDO cache
instantiation still hardcodes .max_capacity(500) in src/pdo.rs; update the PDO
cache creation to read the capacity from the pdo_pending_requests config instead
of using .max_capacity(500) (e.g., use the CacheEntryConfig.capacity/
max_capacity accessor on the pdo_pending_requests instance when calling the
cache builder), or alternatively align the default back to 500 so the hardcoded
value and config match; locate the cache construction code in pdo.rs that calls
.max_capacity(500) and replace it to use the pdo_pending_requests configuration
value.
In `@src/socket/noise_socket.rs`:
- Around line 47-49: The comment above the channel creation for send jobs
currently says a buffer size of 32 but the actual call uses
async_channel::bounded::<SendJob>(8); update the comment to state a buffer size
of 8 (or adjust the bound to match the documented 32) so the comment and the
channel creation for send_job_tx/send_job_rx (SendJob) are consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1501357-5bda-450d-a732-05b32efa03f3
📒 Files selected for processing (4)
src/cache_config.rssrc/socket/noise_socket.rstransports/tokio-transport/src/lib.rswacore/src/store/signal_cache.rs
Comment on lines
+174
to
175
| /// PDO pending requests (time_to_live). Default: 30s TTL, 200 entries. | ||
| pub pdo_pending_requests: CacheEntryConfig, |
There was a problem hiding this comment.
pdo_pending_requests default appears out of sync with runtime capacity.
Line 247 changes default capacity to 200, but src/pdo.rs (Line 39 in the provided snippet) still uses a hardcoded .max_capacity(500). That makes this config value ineffective for the actual PDO cache path.
Suggested follow-up patch (outside this file)
-// src/pdo.rs
-.max_capacity(500)
+// src/pdo.rs
+.max_capacity(cache_config.pdo_pending_requests.capacity.max(1))Also applies to: 247-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cache_config.rs` around lines 174 - 175, The config field
pdo_pending_requests (type CacheEntryConfig) has its default capacity changed to
200 but the PDO cache instantiation still hardcodes .max_capacity(500) in
src/pdo.rs; update the PDO cache creation to read the capacity from the
pdo_pending_requests config instead of using .max_capacity(500) (e.g., use the
CacheEntryConfig.capacity/ max_capacity accessor on the pdo_pending_requests
instance when calling the cache builder), or alternatively align the default
back to 500 so the hardcoded value and config match; locate the cache
construction code in pdo.rs that calls .max_capacity(500) and replace it to use
the pdo_pending_requests configuration value.
Benchmark Results59 unchanged benchmark(s)
|
![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: fd1ba333fb
ℹ️ 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".
| session_locks_capacity: 10_000, | ||
| chat_lanes_capacity: 5_000, | ||
| session_locks_capacity: 2_000, | ||
| chat_lanes_capacity: 1_000, |
There was a problem hiding this comment.
chat_lanes is a capacity-evicted coordination cache, and this default (1_000) is now low enough to hit in long-lived/offline-sync sessions with many distinct chats. When a lane is evicted while its old worker is still draining queued messages (MessageHandler::handle + create_chat_lane in src/handlers/message.rs), the next message for that chat creates a new lane/worker, so two workers process the same chat concurrently and message order guarantees can be violated. The comment immediately above this setting already documents that eviction can break serialization, so lowering this default introduces correctness risk rather than only a memory tradeoff.
Useful? React with 👍 / 👎.
- Comment said buffer size 32 but code uses 8 after reduction - new_pdo_cache() was never called (client uses CacheConfig instead)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed06f9c97c
ℹ️ 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".
| // breaking serialization. Size generously to avoid eviction pressure. | ||
| session_locks_capacity: 10_000, | ||
| chat_lanes_capacity: 5_000, | ||
| session_locks_capacity: 2_000, |
There was a problem hiding this comment.
Lowering session_locks_capacity to 2,000 makes capacity eviction much more likely in long-lived/high-fanout sessions, and this cache stores live per-address mutexes used by Client::session_lock_for to serialize Signal session access. Once a key is evicted while an old Arc<Mutex<()>> is still held, the next lookup creates a second mutex for the same address, so concurrent encrypt/decrypt paths can run without mutual exclusion and race session state updates. The adjacent comment already documents that eviction here breaks serialization, so this default reduction introduces a real correctness risk rather than just a memory tradeoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/socket/noise_socket.rs`:
- Around line 47-49: The comment above the async_channel::bounded::<SendJob>(8)
creation contains a hardcoded latency claim ("<1ms each") which is incorrect
because transport.send(...).await introduces network-dependent latency; update
the comment for send_job_tx/send_job_rx to remove the timing assertion and
instead explain the rationale for capacity=8 in terms of memory usage and
backpressure (e.g., small fixed buffer to limit in-memory queued SendJob items
and to provide modest buffering for the serial sender), keeping the explanation
focused on "why" not timing guarantees.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cc76424-51b3-41cf-afaf-72b9822105b6
📒 Files selected for processing (2)
src/pdo.rssrc/socket/noise_socket.rs
💤 Files with no reviewable changes (1)
- src/pdo.rs
Comment on lines
+47
to
+49
| // Create channel for send jobs. Buffer of 8 is sufficient since the | ||
| // sender task processes jobs serially in <1ms each. | ||
| let (send_job_tx, send_job_rx) = async_channel::bounded::<SendJob>(8); |
There was a problem hiding this comment.
Avoid hardcoded latency claims in the queue-capacity comment.
Line 48 says jobs complete in “<1ms each,” but this path includes transport.send(...).await, so latency is network-dependent and not guaranteed. Keep the comment rationale-focused (memory/backpressure) instead of fixed timing.
Suggested patch
- // Create channel for send jobs. Buffer of 8 is sufficient since the
- // sender task processes jobs serially in <1ms each.
+ // Keep this queue small to cap pre-allocation and apply backpressure
+ // during bursts; default tuned for typical single-client usage.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Create channel for send jobs. Buffer of 8 is sufficient since the | |
| // sender task processes jobs serially in <1ms each. | |
| let (send_job_tx, send_job_rx) = async_channel::bounded::<SendJob>(8); | |
| // Keep this queue small to cap pre-allocation and apply backpressure | |
| // during bursts; default tuned for typical single-client usage. | |
| let (send_job_tx, send_job_rx) = async_channel::bounded::<SendJob>(8); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/socket/noise_socket.rs` around lines 47 - 49, The comment above the
async_channel::bounded::<SendJob>(8) creation contains a hardcoded latency claim
("<1ms each") which is incorrect because transport.send(...).await introduces
network-dependent latency; update the comment for send_job_tx/send_job_rx to
remove the timing assertion and instead explain the rationale for capacity=8 in
terms of memory usage and backpressure (e.g., small fixed buffer to limit
in-memory queued SendJob items and to provide modest buffering for the serial
sender), keeping the explanation focused on "why" not timing guarantees.
Remove tokio-native from default features. Single-thread runtime saves ~500KB peak memory and matches WA Web's single-thread model. Handles ~3K msg/sec sustained which covers typical bot usage. High-throughput deployments can opt in via features = ["tokio-native"] to get multi-thread runtime (5K+ msg/sec). Also enables cooperative yielding in TokioRuntime since single-thread depends on it for fair task scheduling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime_impl.rs`:
- Around line 33-35: The current-thread runtime impl provides yield_now() but
still inherits the default yield_frequency() (10), which is too high for a
single-threaded scheduler; add an override for the yield_frequency() method in
the same impl (implement yield_frequency(&self) -> usize) and return a much
smaller value (e.g., 1) so the current-thread runtime yields more frequently and
avoids starving the event loop; update the impl that contains yield_now() to
include this yield_frequency() override.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41d58659-2272-45ce-a2c1-a51092b9c54c
📒 Files selected for processing (4)
Cargo.tomlexamples/benchmark.rssrc/main.rssrc/runtime_impl.rs
💤 Files with no reviewable changes (1)
- Cargo.toml
TokioRuntime::yield_frequency now uses Handle::current().runtime_flavor() to return 1 for current-thread (yield every frame, avoid starvation) or 10 for multi-thread (yield infrequently, minimize overhead). Works correctly for both runtime modes without user configuration.
jlucaso1
changed the title
jlucaso1
marked this pull request as draft
July 1, 2026 12:23
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.
Summary
Reduce over-sized cache/channel pre-allocations and default to single-thread Tokio runtime for lower memory usage.
DHAT impact (multi-thread, capacity reductions only):
Single-thread runtime impact (on top of capacity reductions):
Full optimization journey (all PRs combined):
Changes
1. Reduce cache/channel pre-allocations
All values remain configurable via
CacheConfig.2. Default to single-thread runtime
tokio-nativefrom default featuresfeatures = ["tokio-native"]for multi-thread (5K+ msg/sec)3. Auto-detect yield frequency
TokioRuntime::yield_frequency()usesHandle::current().runtime_flavor()Cleanup
new_pdo_cache()function (cache built from CacheConfig, never called)Benchmark validation
Test plan