Repository navigation
Conversation
Pure move, no behavior change: ListSnapshot<T> and IListSnapshot were private nested types of InMemoryCacheClient and are now internal types in InMemoryListSnapshot.cs so they can be tested directly.
- A snapshot built from a dictionary keeps only the dictionary and its expiration bounds; blocks and the key index are built on the first small update, so bulk writes are one dictionary copy (fast copy constructor, no rehash). - Replace the ImmutableDictionary key index with small hash-partitioned dictionaries that are copied on write. The first small change on a 1,000-item list went from ~129 us / 90 KB (building the AVL tree) to a single pass. - Track min/max expiration per block instead of an ImmutableSortedDictionary; pruning skips blocks with nothing expired. - Add AsReadOnlyDictionary(), a read-only IDictionary/ICollection view used to size lists without copying them. - Add InMemoryListSnapshotTests, including a 3,000-step randomized comparison against a Dictionary model.
- Lists are shared (no full copy per write) whenever cloning is off, in every sizing mode; previously only no sizing and WithFixedSizing shared them. Snapshot updates size the whole list through ListSnapshot.AsReadOnlyDictionary(). - ListAddAsync no longer sizes and clones the incoming batch on its own when the key already holds a shared list. - Behavior change: a custom SizeCalculator receives list values as a read-only IDictionary<T, DateTime?> rather than a Dictionary<T, DateTime?>. Documented in the SizeCalculator contract, the caching guide and the skill. - Remove FixedSizeCalculator, which existed only for the sharing check; WithFixedSizing is back to main's lambda.
…ts, share immutable items under cloning - The first small change to an unindexed list copies its dictionary (~3 us at 1,000 items) and only a list that keeps changing builds the block and partition index, so a list written once or twice never pays for indexing. - ListAddAsync and ListRemoveAsync take a lock striped by key (64 stripes) after a lost compare-and-swap, so writers queue instead of each redoing an O(n) copy. Publishing still uses compare-and-swap; other writers stay lock-free. - With CloneValues on, lists whose items never need cloning (numbers, strings, value types) also keep a shared snapshot. Raw dictionary reads still deep-clone the materialized dictionary; reference-type items keep copying.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 736871fce9
ℹ️ 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".
…reference-holding structs, keep snapshot values owned Moves the conflict lock onto the single remaining update loop (eager stripes, mask index, System.Threading.Lock on net9+). Custom SizeCalculators receive a Dictionary copy again; only the built-in calculators read the read-only view. Struct items holding references are cloned under CloneValues. GetSnapshot always hands out a copy, so reads use the owned dictionary without layout checks. Skips publishing an unchanged list, avoids a discarded clone for new shareable lists, and adds Utc suffixes to new expiration names.
…ch target's lock type
…ory-list-snapshot
niemyjski
changed the title
…tries' into feature/pr582-refresh-20261023
…tries' into feature/pr582-refresh-20261023
…pr582-oct27-refresh # Conflicts: # .agents/skills/foundatio/SKILL.md
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch has not been deployed
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.
Stacked on #570. Reduce copying and retry work from atomic list updates while preserving comparers and collection snapshots. Include #587's expiration, calculator-recognition, cloning, and size-limit corrections before merging; it also adds an explicit fast path for custom sizing.
Compatibility: existing callbacks still receive concrete dictionaries and must not reenter the cache, because contended writes may invoke them under a stripe lock. Item expirations read back as UTC. Oversized fallback estimates are rejected or throw the configured size-limit exception. No existing CLR signature break.
Performance decision remains: existing concrete-dictionary custom calculators still copy whole lists. At 1,000 items with cloning off, the compatibility path costs 3.27× time / 37× allocation versus main. #587 adds an explicit read-only sizing opt-in: 0.95 µs / 7.80 KiB, versus 7.04 µs / 85.57 KiB without it and 2.16 µs / 2.31 KiB on main. This removes sizing copies, not all immutable-update allocation. Accept this migration/cost tradeoff before merging the cache stack.
Validation: combined Release 2,448 passed / 22 skipped / 0 failed; current provider source workspace build, Redis 777 passed / 18 skipped, repository tests 152 passed, net8/net10 API compatibility, and changed-file formatting passed. All five stack heads have green hosted Build workflows.
Verification and implementation details
Validated heads: #570
351d2cba, #5809760d3a8, #58211488067, #586e19ee7f3, #5873f24b90b; baseline mainbe10785c. Final combined core: 2,470 total, 2,448 passed, 22 skipped. Redis selection: 795 total, 777 passed, 18 skipped.The restore failures are resolved by the separate DataProtection update already on main (
be10785c): 8.0.32 resolves Xml 8.0.5 and Pkcs 8.0.2. Current provider revisions also resolve the old xUnit pins and Parsers cryptography advisory. No workflow edits or audit suppression.Implementation
Dictionary-backed snapshots defer indexing until repeated incremental changes. Indexed writes copy affected blocks and partitions; bulk changes still copy a dictionary. Writers serialize by key stripe after a failed compare-and-swap, so colliding keys can contend.
With cloning enabled, strings and value types without references can share immutable storage. Mutable payloads retain cloning. Cloned dictionary/Items reads preserve incremental writes; uncloned raw dictionary reads expose a shared dictionary and force subsequent writes to copy it.
Built-in fixed/dynamic sizing already accepts read-only views. #587 adds
SizeCalculator(calculateSize, supportsReadOnlyCollections: true)andSizeCalculatorSupportsReadOnlyCollections. A custom calculator must accept ordinary dictionaries and read-only views through interfaces and never mutate them. The old overload resets that opt-in, protecting replacement callbacks. This does not change cache reads or eliminate callback enumeration costs.Whole-result size limits apply to add/remove. An oversized result can reject a removal. Rejected writes preserve live value/TTL/size; an already-expired entry can still be reclaimed with its expiration event.
Paired measurements on the current production implementation
BenchmarkDotNet 0.15.8, MemoryDiagnoser, .NET 10.0.12, Apple M4 Max. Main
be10785cversus the production code in #5873f24b90b(production unchanged from9e111be1). Calculator readsIDictionary<int, DateTime?>.Count; each operation adds one item and removes it. KiB are BDN's “KB”.Three paired jobs: main, compatibility default, and read-only opt-in. Same harness/session with no overlapping builds/tests. 18 ShortRun cases; 32,768 invocations, unroll16, three warmups/measurements. Fast cases are directional because some iterations are below 100 ms.
A longer confirmation of the 1,000-item, cloning-off case used 131,072 invocations, five warmups and eight measured iterations: main 2.156 µs / 2.31 KiB; compatible 7.043 µs / 85.57 KiB; opt-in 0.947 µs / 7.80 KiB. The opt-in is 7.4× faster and allocates about 91% less than the compatibility path, but still allocates 3.38× main. At 10,000 items the compatibility default remains 7.80× slower / 318× allocation; the opt-in still allocates 10.58× main. Small clone-disabled lists also retain a slowdown. Do not describe this as eliminating all regressions.
The prior October 12 constant-return callback is a different workload. Its historical unsized/dynamic/cold-write results were not rerun. This pass does not establish scalar, compaction, bulk, retained-heap, or contended throughput neutrality. Non-cloning contended main throws and is not a valid timing baseline.
Reproduction and coverage
The checked-in
InMemoryListBenchmarksincludesCustomandCustomReadOnlywith the same Count callback. Build saved main/candidate DLLs and use paired BDN jobs with conditionalReference/HintPathreferences. Enable the capability only during candidate setup; setup/reflection is outside measurement.dotnet run --project benchmarks/Foundatio.Benchmarks.csproj -c Release -- --filter '*InMemoryListBenchmarks.AddRemove*'The new rejected-write/snapshot/allocation cases failed before the capability was wired and pass afterward. Contention coverage includes unsized and both custom-sizing paths with cloning off/on and verifies final tracked memory. Existing comparer, expiration, retry, cloning, and snapshot-model coverage remains.
Ten current sibling repositories build with
ReferenceFoundatioSource=true; two existingASPIRE010warnings remain. Redis 8.10.2 uses the exact combined core DLL, with the 14 lock cases from Redis #191 and six extra shared cache registrations in the temporary validation checkout. These registrations are not a claim that provider main/CI discovers them. Six Hybrid and three FastCloner cloning skips remain. Non-Redis service-backed suites and the documentation-site build were not run.Review #587 into this branch before merging. Retarget after #570, and release with the reviewed lock stack. Hosted CI is green. No new workflow changes, skips, cloner modifications, or file decomposition.