-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Clear() to MemoryCache #57631
Add Clear() to MemoryCache #57631
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @eerhardt, @maryamariyan, @michaelgsharp Issue Detailsfixes #45593
|
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
{ | ||
CheckDisposed(); | ||
|
||
// the following two operations are not atomic change as a whole, but an alternative would be to introduce a global lock for every access to _entries and _cacheSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get away with this by introducing a new class that contained both the dictionary and the size? Then we can atomically replace the pointer with a new instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, yes. In other, where we add/remove item from the cache and update the size afterwards, not.
runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Lines 265 to 270 in 40cbe82
if (_entries.TryRemove(key, out CacheEntry entry)) | |
{ | |
if (_options.SizeLimit.HasValue) | |
{ | |
Interlocked.Add(ref _cacheSize, -entry.Size.Value); | |
} |
runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Lines 151 to 158 in 40cbe82
entryAdded = _entries.TryUpdate(entry.Key, entry, priorEntry); | |
if (entryAdded) | |
{ | |
if (_options.SizeLimit.HasValue) | |
{ | |
// The prior entry was removed, decrease the by the prior entry's size | |
Interlocked.Add(ref _cacheSize, -priorEntry.Size.Value); |
I am not sure if it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In those other cases you can atomically read the pointer, store it in a local variable and modify that. If Clear gets called concurrently, it will either update the pointer before or after the other mutation. Thus, we should always have a coherent set of entries and cache size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt well, it took me a while to get back to this PR. PTAL at my recent commit.
src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs
Outdated
Show resolved
Hide resolved
…xpirationTests.cs
- Pulling MemoryCache fixes from dotnet/runtime#57631 and dotnet/runtime#61187
- Pulling MemoryCache fixes from dotnet/runtime#57631 and dotnet/runtime#61187
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for the contribution here @adamsitnik.
I just had some minor comments and questions.
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs
Outdated
Show resolved
Hide resolved
|
||
private long _cacheSize; | ||
private CoherentState _coherentState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a case where we need volatile
? My intuition tells me "no" because we only ever read the field once at the beginning of a method, and then use the local variable. But I wanted to ask the question, since I'm not an expert on when to use volatile
.
Co-authored-by: Eric Erhardt <[email protected]>
fixes #45593