docs(LruCache): note that a hit on the already-MRU entry keeps enumerators valid - #330
Merged
marius-bughiu merged 2 commits intoJul 30, 2026
Merged
Conversation
…ators valid The class remarks, the TryGet remarks, the GetEnumerator summary, and the API reference all stated unconditionally that a mutating read invalidates active enumerators. MoveToHeadIfNeeded deliberately skips the relink and the version bump when the entry is already the head, which LruCacheEnumerationTests pins as GetOfAlreadyMostRecentEntry_DoesNotInvalidateEnumerator. Documentation only; no behavioural change.
marius-bughiu
added
the
automated:code-review
Coverage
|
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The updated enumerator documentation still uses “modified during enumeration” wording that is broader than the actual _version-based invalidation behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the public documentation for LruCache to reflect an intentional implementation detail: a mutating read (this[key] getter / TryGet) only invalidates active enumerators when it actually changes the recency list; a hit on the already-most-recently-used entry is a no-op and keeps enumerators valid.
Changes:
- Updated
LruCacheclass remarks andTryGetremarks to document the MRU hit no-op enumerator-validity exception. - Updated
GetEnumeratorXML documentation to describe the exception in the enumerator invalidation semantics. - Updated the
LruCachesection indocs/api/collections.mdwith the same clarification.
File summaries
| File | Description |
|---|---|
| src/Celerity/Collections/LruCache.cs | Refines XML docs to note the MRU-hit no-op case for enumerator validity. |
| docs/api/collections.md | Mirrors the MRU-hit no-op enumerator-validity exception in the public markdown docs. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
"Modified during enumeration" was broader than the code: AddOrUpdate on an entry that is already the head overwrites _nodeValues without bumping _version, so an in-place value overwrite leaves enumerators valid. Name the real condition — a change to the entry set or the recency order — instead.
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The documentation updates align with the implementation’s _version semantics and existing library conventions, with no code changes introduced.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Benchmarks8 regressions Highlights
Collections (508)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
11 tasks
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.
What
LruCachedocuments in four places that a mutating read (the indexer getter orTryGet) invalidates any in-progress enumerator. It does so unconditionally, but the implementation is deliberately narrower:MoveToHeadIfNeededreturnsfalsewithout relinking or bumping_versionwhen the entry is already the head, so a hit on the most-recently-used entry leaves active enumerators valid. This PR adds that exception to the class remarks, theTryGetremarks, theGetEnumeratorsummary, and theLruCachesection ofdocs/api/collections.md. No code changes.Why
The behaviour is intentional — the internal comment on
MoveToHeadIfNeededspells it out ("so a getter of the freshest entry does not spuriously invalidate enumerators") andLruCacheEnumerationTests.GetOfAlreadyMostRecentEntry_DoesNotInvalidateEnumeratorpins it — but a consumer reading the public docs would expect the opposite. The repo already documents this class of exemption elsewhere (FenwickTree's "except when they are no-ops",BTreeDictionary's "a rejected duplicateTryAddis a true no-op"), so this bringsLruCachein line rather than introducing a new convention.Test plan
dotnet build— succeeds, 0 warnings forCelerity.csprojdotnet testrun: the change is XML doc comments and Markdown only, with no effect on emitted ILLruCacheEnumerationTests.GetOfAlreadyMostRecentEntry_DoesNotInvalidateEnumeratoralready covers the behaviour now being documented