feat: sharded in-memory stores + #[concurrent_cached] default#264
Open
jaemk wants to merge 1 commit into
Open
Conversation
This comment has been minimized.
This comment has been minimized.
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
2f427df to
fbc8261
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
fbc8261 to
3dc1867
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
3dc1867 to
f2bdc69
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
f2bdc69 to
2bace40
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
2bace40 to
aeddf09
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
aeddf09 to
52e6969
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
52e6969 to
1c7c966
Compare
jaemk
force-pushed
the
20250525.sharded-cache
branch
from
1c7c966 to
2473a37
Compare
Comment on lines
+147
to
+151
| **Impact on `cache_delete`:** `cache_delete` is a provided method that returns `true` when | ||
| `cache_remove` returns `Some`. Because expired entries return `None`, `cache_delete` returns | ||
| `false` even though the entry was physically removed. If you call `cache_delete` to detect | ||
| whether a key existed, use `cache_remove` instead and check whether the returned option is | ||
| `Some` (live entry) or `None` (expired or absent). |
|
|
||
| **Detection:** search for `.cache_remove(` / `.remove(` / `.cache_delete(` / `.delete(` calls on an expiring store. | ||
|
|
||
| **Action:** if the code relied on `Some(value)` for expired-but-present entries, add an explicit `is_expired()` check before removal, or ignore the return value. Also note that `cache_delete` / `delete` return `false` for expired entries that are physically removed — if you use `cache_delete` to check whether a key existed, switch to `cache_remove` and check whether the result is `Some` (live) or `None` (expired or absent). |
Comment on lines
+8
to
+14
| Commands (executed in the order given): | ||
| comments Fetch and display all inline PR review comments (new vs pre-existing) | ||
| threads List all review threads with resolution status | ||
| resolve Resolve all open review threads | ||
| rerequest Re-request Copilot review | ||
| minimize Minimize (hide) all Copilot review comments as "resolved" | ||
| codspeed Check CodSpeed benchmark results |
Comment on lines
+58
to
+60
| **Detection:** search for `.cache_remove(` / `.remove(` / `.cache_delete(` / `.delete(` calls on an expiring store. | ||
|
|
||
| **Action:** if the code relied on `Some(value)` for expired-but-present entries, add an explicit `is_expired()` check before removal, or ignore the return value. Also note that `cache_delete` / `delete` return `false` for expired entries that are physically removed — if you use `cache_delete` to check whether a key existed, switch to `cache_remove` and check whether the result is `Some` (live) or `None` (expired or absent). |
Comment on lines
+147
to
+151
| **Impact on `cache_delete`:** `cache_delete` is a provided method that returns `true` when | ||
| `cache_remove` returns `Some`. Because expired entries return `None`, `cache_delete` returns | ||
| `false` even though the entry was physically removed. If you call `cache_delete` to detect | ||
| whether a key existed, use `cache_remove` instead and check whether the returned option is | ||
| `Some` (live entry) or `None` (expired or absent). |
Comment on lines
+3
to
+12
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - `Cached::cache_remove_entry<Q>(&mut self, k: &Q) -> Option<(K, V)>`: new required method on the `Cached` trait that removes an entry and returns the stored key and value. Unlike `cache_remove`, this returns `Some` even when the deleted entry was already expired, making it possible to distinguish "key absent" from "key present but expired". Always fires the store's `on_evict` callback (if set). | ||
| - `ConcurrentCached::cache_remove_entry(&self, k: &K) -> Result<Option<(K, V)>, Self::Error>`: same semantics on the concurrent trait; implemented for all thirteen stores (seven non-sharded, six sharded). | ||
| - `Cached::cache_delete<Q>(&mut self, k: &Q) -> bool`: new default method on `Cached` that deletes an entry without returning it; returns `true` if an entry was physically removed (including expired entries), `false` if the key was absent. Implemented via `cache_remove_entry`. | ||
| - `DiskCache` and `RedisCache` / `AsyncRedisCache` now require `K: Clone` (in addition to existing bounds) for their `ConcurrentCached` / `ConcurrentCachedAsync` impls, which is needed to return the stored key from `cache_remove_entry`. | ||
|
|
||
| ### Fixed | ||
| - `Cached::cache_delete` (now on `Cached` via `cache_remove_entry`) correctly returns `true` for entries that were present but already expired; previously `cache_delete` on `ConcurrentCached` returned `false` for expired entries. |
Comment on lines
+34
to
+36
| /// so spatial locality is a win. Counters use `Relaxed` atomics; on stores | ||
| /// that allow concurrent readers (read-lock paths), increments can race — | ||
| /// this is intentional, trading exactness for lower overhead. |
Comment on lines
+227
to
+250
| if args.cache_err && !is_result_return { | ||
| return syn::Error::new( | ||
| fn_ident.span(), | ||
| "`cache_err = true` requires the function to return `Result<T, E>`", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } | ||
| if args.cache_none && !is_option_return { | ||
| return syn::Error::new( | ||
| fn_ident.span(), | ||
| "`cache_none = true` requires the function to return `Option<T>`", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } | ||
| if args.cache_err && args.result_fallback { | ||
| return syn::Error::new( | ||
| fn_ident.span(), | ||
| "`cache_err` and `result_fallback` are mutually exclusive", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } |
Comment on lines
+124
to
+139
| if args.cache_err && !is_result_return { | ||
| return syn::Error::new( | ||
| fn_ident.span(), | ||
| "`cache_err = true` requires the function to return `Result<T, E>`", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } | ||
| if args.cache_none && !is_option_return { | ||
| return syn::Error::new( | ||
| fn_ident.span(), | ||
| "`cache_none = true` requires the function to return `Option<T>`", | ||
| ) | ||
| .to_compile_error() | ||
| .into(); | ||
| } |
Comment on lines
+302
to
+308
| print(f"Branch: {branch}") | ||
| print(f"\n--- CodSpeed workflow runs (last {limit}) ---") | ||
| r = gh("run", "list", "--workflow=codspeed.yml", f"--branch={branch}", | ||
| f"--limit={limit}", "--json", "databaseId,conclusion,createdAt,status", check=False) | ||
| if r.returncode != 0: | ||
| print("Could not fetch workflow runs.") | ||
| return |
Comment on lines
+303
to
+308
| print(f"\n--- CodSpeed workflow runs (last {limit}) ---") | ||
| r = gh("run", "list", "--workflow=codspeed.yml", f"--branch={branch}", | ||
| f"--limit={limit}", "--json", "databaseId,conclusion,createdAt,status", check=False) | ||
| if r.returncode != 0: | ||
| print("Could not fetch workflow runs.") | ||
| return |
Comment on lines
+193
to
+203
| pub fn cache_clear_with_on_evict(&mut self) { | ||
| if self.on_evict.is_none() { | ||
| return self.cache_clear(); | ||
| } | ||
| let entries: Vec<(K, V)> = self.store.drain().collect(); | ||
| if let Some(on_evict) = &self.on_evict { | ||
| for (k, v) in &entries { | ||
| on_evict(k, v); | ||
| } | ||
| } | ||
| } |
Comment on lines
+233
to
+247
| pub fn cache_clear_with_on_evict(&mut self) { | ||
| if self.on_evict.is_none() { | ||
| return self.cache_clear(); | ||
| } | ||
| let entries: Vec<(K, TimedEntry<V>)> = self.store.drain().collect(); | ||
| let count = entries.len() as u64; | ||
| if count > 0 { | ||
| self.evictions.fetch_add(count, Ordering::Relaxed); | ||
| } | ||
| if let Some(on_evict) = &self.on_evict { | ||
| for (k, entry) in &entries { | ||
| on_evict(k, &entry.value); | ||
| } | ||
| } | ||
| } |
Comment on lines
+383
to
+401
| let mutex_map_w: Arc<Mutex<HashMap<usize, usize>>> = Arc::new(Mutex::new(HashMap::new())); | ||
| group.bench_function("Mutex<HashMap>", |b| { | ||
| b.iter_custom(|iters| { | ||
| let map = mutex_map_w.clone(); | ||
| run_concurrent!(map, iters, t, i, { | ||
| map.lock().insert(write_key(i, t), i * 2); | ||
| }) | ||
| }) | ||
| }); | ||
|
|
||
| let sharded_w = ShardedCache::<usize, usize>::new(); | ||
| group.bench_function("ShardedCache", |b| { | ||
| b.iter_custom(|iters| { | ||
| let cache = sharded_w.clone(); | ||
| run_concurrent!(cache, iters, t, i, { | ||
| cache.cache_set(write_key(i, t), i * 2).unwrap(); | ||
| }) | ||
| }) | ||
| }); |
Comment on lines
+302
to
+308
| print(f"Branch: {branch}") | ||
| print(f"\n--- CodSpeed workflow runs (last {limit}) ---") | ||
| r = gh("run", "list", "--workflow=codspeed.yml", f"--branch={branch}", | ||
| f"--limit={limit}", "--json", "databaseId,conclusion,createdAt,status", check=False) | ||
| if r.returncode != 0: | ||
| print("Could not fetch workflow runs.") | ||
| return |
Comment on lines
+125
to
+126
| ShardedExpiringCache<K, V> — unbounded, per-value TTL | ||
| ShardedExpiringLruCache<K, V> — LRU, per-value TTL |
Comment on lines
+291
to
+306
| def cmd_codspeed(pr, owner, repo, branch=None, limit=5): | ||
| section(f"PR #{pr} CodSpeed") | ||
|
|
||
| if branch is None: | ||
| r = gh("pr", "view", str(pr), "--json", "headRefName", "--jq", ".headRefName", check=False) | ||
| if r.returncode == 0 and r.stdout.strip(): | ||
| branch = r.stdout.strip() | ||
| else: | ||
| print("Could not auto-detect branch. Pass --branch explicitly.") | ||
| return | ||
|
|
||
| print(f"Branch: {branch}") | ||
| print(f"\n--- CodSpeed workflow runs (last {limit}) ---") | ||
| r = gh("run", "list", "--workflow=codspeed.yml", f"--branch={branch}", | ||
| f"--limit={limit}", "--json", "databaseId,conclusion,createdAt,status", check=False) | ||
| if r.returncode != 0: |
Comment on lines
+31
to
+34
| /// (in practice always `CachePadded<Shard<S>>`). The lock word and the | ||
| /// hit/miss counters intentionally share a cache line: they are touched by | ||
| /// the same op (a `cache_get` acquires the lock and then bumps a counter), | ||
| /// so spatial locality is a win. Counters use `Relaxed` atomics; on stores |
| /// In the in-memory default path, `map_error` is optional and defaults to an `Infallible` shim. | ||
| /// Because the default sharded stores cannot fail, a `map_error` supplied on this path is accepted | ||
| /// but ignored (it is never invoked); reserve it for `redis`/`disk`/custom `ty`/`create` stores. | ||
| /// Functions may return a plain `T`, `Option<T>`, or `Result<T, E>`. Plain values are |
Comment on lines
+303
to
+305
| print(f"\n--- CodSpeed workflow runs (last {limit}) ---") | ||
| r = gh("run", "list", "--workflow=codspeed.yml", f"--branch={branch}", | ||
| f"--limit={limit}", "--json", "databaseId,conclusion,createdAt,status", check=False) |
Breaking macro changes: - `result = true` / `option = true` removed from `#[cached]` and `#[once]`; interceptors added with clear removal errors pointing to `cache_err`/`cache_none` - `option = true` never valid on `#[concurrent_cached]`; error updated accordingly - `result_fallback` no longer requires `result = true`; now requires `ttl` (compile guard) - `map_error` on default sharded path is a compile error (was silently ignored) - `cache_err + expires`, `cache_none + expires`, `result_fallback + expires`, `result_fallback + with_cached_flag`, `cache_none + with_cached_flag` all rejected at compile time with actionable diagnostics New macro attributes: - `cache_err = true` — opt-in to cache Err values (`#[cached]`, `#[once]`, `#[concurrent_cached]`) - `cache_none = true` — opt-in to cache None values (same three macros) - `result_fallback = true` on `#[concurrent_cached]` — returns last cached Ok on Err; requires `ttl`; stale TTL refreshed on every Err call; first-ever-failure returns raw Err - `shards = N` on `#[concurrent_cached]` quick-reference table New sharded concurrent stores: - ShardedCache, ShardedLruCache, ShardedTtlCache, ShardedLruTtlCache, ShardedExpiringCache, ShardedExpiringLruCache — Arc-backed, per-shard parking_lot RwLock, builder APIs, on_evict callbacks, metrics, 128-byte cache-line padding - `#[concurrent_cached]` defaults to in-memory sharded stores; `ty`/`create`/`map_error` optional on the in-memory path New traits and methods: - `ConcurrentCloneCached<K, V>` — returns stale cache entries with expiry status without evicting; implemented on four expiry-capable sharded stores - `cache_remove_entry` added as required method to `Cached` and `ConcurrentCached`; returns Some for expired-but-present entries (unlike `cache_remove`) - `cache_clear_with_on_evict()` on all 13 stores; fires on_evict per entry, increments eviction counter (plain `cache_clear()` remains side-effect-free) - `cache_delete` added as default method on `Cached` (was `ConcurrentCached`-only) - `cache_delete` on `ConcurrentCached` now returns true for expired entries (BREAKING) Store behavior changes: - `cache_remove` on expiring stores returns None for expired-but-present entries - `is_result_return_type` / `is_option_return_type` use exact ident match; aliases like `MyResult<T>` are treated as plain values and Err is cached - `LruCache::retain` fires on_evict and increments evictions counter - All non-unbound stores fire on_evict on explicit `cache_remove` - Rename `clear_with_on_evict` → `cache_clear_with_on_evict` (cache_ prefix convention) - Rename `LruTtlInner::ttl_evictions` → `non_capacity_evictions` - `DiskCache`/`RedisCache`/`AsyncRedisCache` `ConcurrentCached` impls now require `K: Clone` Fixes and hardening: - Fix use-after-move in `concurrent_cached` result_fallback for non-Copy key types - Async result_fallback uses `ConcurrentCachedAsync` (.await) consistently - Fix dead `is_smart_option` branch; Option<T> + non-default backend errors correctly - result_fallback static visibility matches annotated function visibility - Safe two-lookup pattern in TtlSortedCache (revert unsafe raw-pointer optimization) - CACHE_LINE padding: `assert!()` replaces unused dead const - `#[concurrent_cached]` store-conflict diagnostics (create/redis/disk) echo the user-written attribute name (`max_size` vs reconciled `size`) - Document that explicit `shards = N` is rounded up to a power of two but not clamped to the 8–1024 default range (`size`/`max_size` attribute unaffected) - Document that `CachedIter::iter()` (non-sharded `ExpiringCache`/`ExpiringLruCache` only) filters expired entries without removing them from the map - Document that sharded reads clone values from under the shard lock, so `V: Clone` - Remove stale 1.1-to-1.2 migration doc (version never released, content contradicted code) - Stabilize flaky async CI test with timing-independent assertion Tests: - Integration tests for `#[concurrent_cached]` with `cache_err`, `cache_none`, `result_fallback` (sync + async, TTL expiry, first-ever-failure, non-Copy keys) - Eviction counter tests for all 11 stores with eviction tracking - cache_remove_entry coverage: counter increment, absent-key, stored-key identity - Compile-fail tests for all new mutual-exclusion guards, including the `max_size` store-conflict diagnostic wording - All sharded TTL tests moved into `time_store_tests` module (project convention) Macro attribute deprecation: - `size = N` on `#[cached]` / `#[concurrent_cached]` is now a deprecated alias for `max_size = N`; it still compiles but emits a deprecation warning (a hard error under `-D warnings` / `#![deny(deprecated)]`) steering users to `max_size`. Setting both on one annotation remains a compile error. - Implemented via a `#[deprecated]` marker const (`__DEPRECATED_SIZE_ATTR`) referenced through `quote_spanned!` anchored at the user's `size` token, so the warning points at the attribute. All internal uses (tests, examples, doctests, README) migrated to `max_size`; dedicated alias-regression tests keep `size` covered under `#[allow(deprecated)]`, and trybuild UI tests pin the deprecation diagnostic. Builder rename: - Sharded builder `per_shard_size` -> `per_shard_max_size`, matching the `max_size` vocabulary used across the rest of the size-bound API. Dev tooling (skills/agents, not shipped in the crate): - New `pr-review` skill: standalone read-only review of a PR or checked-out branch (acquire diff, spawn code-review + consumer sub-agents in parallel, report findings with severity and a valid/already-fixed/invalid verdict). Review-agent model defaults to Sonnet, overridable per run (e.g. opus). - `pr-cycle` refactored into the orchestrator: its review step delegates to `pr-review` instead of duplicating it; retains `full`/`local`/`remote` modes and the review-agent model override. - `release` skill gains a pre-release `review` mode: runs a whole-crate consistency review plus an API-examination pass (via the `consumer-experience-review` skill) and reports lingering inconsistencies and a breaking/non-breaking improvement list; advisory only. - Add `pr-code-reviewer`, `pr-consumer-reviewer`, `pr-fix-implementer` agent definitions the skills reference. Version bump: 1.1 → 2.0.0 Migration guide: docs/migrations/1.1-to-2.0-human.md
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
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
Test plan
🤖 Generated with Claude Code