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

Reduce CacheEntry size #45410

Merged
merged 13 commits into from
Dec 2, 2020
Merged

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Dec 1, 2020

I've one more big improvement for the MemoryCache, but it's a little bit controversial so I decided to separate it from reducing the CacheEntry size (this PR).

Changes:

  • Remove _notifyCacheOfExpiration, _notifyCacheEntryCommit and _logger from CacheEntry, replace it with a reference to MemoryCache and instead of invoking delegates, invoke internal methods.
  • Replace 3 boolean fields (_isDisposed, _isExpired and _valueHasBeenSet) with a single [Flag] field. I did this mostly because I plan to introduce another boolean flag soon.

@ghost
Copy link

ghost commented Dec 1, 2020

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

Issue Details

I've one more big improvement for the MemoryCache, but it's a little bit controversial so I decided to separate it from reducing the CacheEntry size (this PR).

Main changes:

  • Remove _notifyCacheOfExpiration, _notifyCacheEntryCommit and _logger from CacheEntry, replace it with a reference to MemoryCache and instead of invoking delegates, invoke internal methods.
  • Replace 3 boolean fields (_isDisposed, _isExpired and _valueHasBeenSet) with a single [Flag] field. I did this mostly because I plan to introduce another boolean flag soon.
  • Remove _absoluteExpirationRelativeToNow and change the AbsoluteExpirationRelativeToNow property implementation to use AbsoluteExpiration property immediately. So far we were setting a private field and computing the actual value later in the SetEntry method.
  • Don't acquire lock in DetachTokens if the field is already set to null
  • avoid interface method call when the clock has not been provided (most common case)
Author: adamsitnik
Assignees: -
Labels:

area-Extensions-Caching

Milestone: 6.0.0

…uteExpiration and current time" because it was causing clock to get called twice

This reverts commit f84b6cb.

# Conflicts:
#	src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
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.

I just had one minor question on the unit test. Other than that, I think this looks good.

@adamsitnik adamsitnik merged commit 62b4a1f into dotnet:master Dec 2, 2020
@adamsitnik adamsitnik deleted the reduceCacheEntrySize branch December 2, 2020 17:04
@adamsitnik
Copy link
Member Author

@eerhardt thanks a lot for the review!

@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 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.

3 participants