Skip to content
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

Merged
merged 8 commits into from
Nov 23, 2021
Merged

Add Clear() to MemoryCache #57631

merged 8 commits into from
Nov 23, 2021

Conversation

adamsitnik
Copy link
Member

fixes #45593

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

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.

@ghost
Copy link

ghost commented Aug 18, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan, @michaelgsharp
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #45593

Author: adamsitnik
Assignees: -
Labels:

area-Extensions-Caching

Milestone: 7.0.0

{
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
Copy link
Member

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?

Copy link
Member Author

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.

if (_entries.TryRemove(key, out CacheEntry entry))
{
if (_options.SizeLimit.HasValue)
{
Interlocked.Add(ref _cacheSize, -entry.Size.Value);
}

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.

Copy link
Member

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.

Copy link
Member Author

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.

ayende added a commit to ayende/ravendb that referenced this pull request Nov 11, 2021
- Pulling MemoryCache fixes from  dotnet/runtime#57631 and dotnet/runtime#61187
Copy link
Member

@eerhardt eerhardt left a 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.


private long _cacheSize;
private CoherentState _coherentState;
Copy link
Member

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.

@stephentoub ?

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@adamsitnik adamsitnik merged commit 092c2ab into dotnet:main Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .Clear() method to MemoryCache
4 participants