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

Developers observe more efficient expiration checks within MemoryCache #47239

Open
jeffhandley opened this issue Jan 20, 2021 · 9 comments
Open
Assignees
Labels
area-Extensions-Caching Bottom Up Work Not part of a theme, epic, or user story Priority:3 Work that is nice to have tenet-performance Performance related issue User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@jeffhandley
Copy link
Member

Previously, we avoided using a Timer for checking cache expiration because such usage would prevent recycling of idle services; this constraint no longer exists. With a Timer-based expiration check, MemoryCache APIs will no longer incur the cost of checking expiration time to spawn a background task to remove expired items. This will yield a ~10% RPS performance gain. See #45842 for more performance benchmark details and conversation.

@jeffhandley jeffhandley added tenet-performance Performance related issue area-Extensions-Caching User Story A single user-facing feature. Can be grouped under an epic. Bottom Up Work Not part of a theme, epic, or user story labels Jan 20, 2021
@jeffhandley jeffhandley added this to the 6.0.0 milestone Jan 20, 2021
@ghost
Copy link

ghost commented Jan 20, 2021

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

Issue Details

Previously, we avoided using a Timer for checking cache expiration because such usage would prevent recycling of idle services; this constraint no longer exists. With a Timer-based expiration check, MemoryCache APIs will no longer incur the cost of checking expiration time to spawn a background task to remove expired items. This will yield a ~10% RPS performance gain. See #45842 for more performance benchmark details and conversation.

Author: jeffhandley
Assignees: adamsitnik
Labels:

Bottom Up Work, User Story, area-Extensions-Caching, tenet-performance

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 20, 2021
@jeffhandley jeffhandley self-assigned this Jan 20, 2021
@jeffhandley jeffhandley added Priority:3 Work that is nice to have and removed untriaged New issue has not been triaged by the area owner labels Jan 20, 2021
@Tratcher
Copy link
Member

Tratcher commented Jun 1, 2021

Previously, we avoided using a Timer for checking cache expiration because such usage would prevent recycling of idle services; this constraint no longer exists.

Why does that constraint no longer exist? ASP.NET Core does not want timers firing on idle web services. @davidfowl

@davidfowl
Copy link
Member

We do have timers firing on idle web services. All the time.

@jeffhandley
Copy link
Member Author

We are moving this out to .NET 7 as we haven't been able to get to it yet during .NET 6.

@jeffhandley jeffhandley modified the milestones: 6.0.0, 7.0.0 Jul 7, 2021
@noahfalk
Copy link
Member

I was chatting with @adamsitnik about some possible optimizations for MemoryCache:

  • DateTimeOffset.UtcNow called in the Add and Lookup paths can be expensive and it gives precision the cache expiration logic probably doesn't need. A different implementation of ISystemClock based on Environment.TickCount would probably be accurate enough and much faster.

  • There are a number of fields in the MemoryCacheEntry object that could be removed, split, and/or made lazy to save space per entry. On x64 the current layout looks like this:

Size:        152(0x98) bytes
              MT    Field   Offset                 Type VT     Attr            Value Name
00007ff7f8ea7a00  4000008        8 ...emory.MemoryCache  0 instance 0000025f1ca75960 _cache
0000000000000000  4000009       10 ...+CacheEntryTokens  0 instance 0000000000000000 _tokens
00007ff7f8eaa608  400000d       18 ...Memory.CacheEntry  0 instance 0000000000000000 _previous
00007ff7f8bd5678  400000e       20        System.Object  0 instance 0000000000000000 _value
00007ff7f8bd5678  4000011       28        System.Object  0 instance 0000025f1ca75888 <Key>k__BackingField
00007ff7f8eaa138  400000a       30 ...Private.CoreLib]]  1 instance 0000025f1ca8aaf0 _absoluteExpirationRelativeToNow
00007ff7f8eaa138  400000b       40 ...Private.CoreLib]]  1 instance 0000025f1ca8ab00 _slidingExpiration
00007ff7f8ea8728  400000c       50 ...Private.CoreLib]]  1 instance 0000025f1ca8ab10 _size
00007ff7f8eaa388  400000f       60 ...y+CacheEntryState  1 instance 0000025f1ca8ab20 _state
00007ff7f8eaa510  4000010       68 ...Private.CoreLib]]  1 instance 0000025f1ca8ab28 <AbsoluteExpiration>k__BackingField
00007ff7f8d4d308  4000012       80 ...em.DateTimeOffset  1 instance 0000025f1ca8ab40 <LastAccessed>k__BackingField
00007ff7f8d95230  4000007       10 ...Private.CoreLib]]  0   static 0000000000000000 ExpirationCallback

There are 5 nullable valuetype fields (_absoluteExpirationRelativeToNow, _slidingExpiration, _size, AbsoluteExpiration, and LastAccessed). Each of those fields adds a single bit to store the Nullable.HasValue state and that bit gets padded out to 8 bytes to preserve the type alignment requirements. Moving those bits to the _state field would save 5*8 = 40 bytes per entry.

There are 2 DateTimeOffset fields (LastAccessed and AbsoluteExpiration). DateTimeOffset has an 8 byte ticks value and a 2 byte offset value. The 2 byte offset is padded up to 8 bytes to preserve alignment. If the DateTimeOffset will always be reported as a UTC time then the offset is zero and there is no need to store it. We could instead store an 8 byte DateTime and convert it to a DateTimeOffset when it is accessed if needed. If it is important for users to be able to round trip a non-zero offset in AbsoluteExpiration those two bytes could still be stored separately in a location that won't be padded.

The lazy allocation pattern being used for _tokens could be extended to store other uncommon data, for example rename the CacheEntryTokens type to be UncommonCacheEntryFields. We could move the _previous field into the uncommon set saving another 8 bytes. If it is rare for users to set both the _slidingExpiration and AbsoluteExpiration a single TimeSpan field could be used for both with a bit from _state used to store which one is present. If both are used simultaneously the 2nd field can be stored in the uncommon data. 8-16 more bytes savable here with the tradeoff that anyone who does use those fields will have more memory usage.

The _cache field could also be removable if we stored that object reference in the same field as the current _tokens reference and use a bit from state to discriminate which one is there. If the uncommon fields are ever needed then the _cache field has to be copied into the uncommon state at that point.

The size field could be changed to a 2 byte value within the state structure and if the full 8 bytes are needed then use the uncommon fields.

I haven't vetted carefully, but if everything here panned out it should save 88 bytes per entry.

@stephentoub
Copy link
Member

@noahfalk, have you looked at #59110?

@noahfalk
Copy link
Member

I wasn't aware of it, I only knew of this one because Adam pointed me here : ) Now I am taking a look...

@noahfalk
Copy link
Member

#59110 looks like it does the first 2 of the 5 size optimizations, nice : )

@eerhardt eerhardt modified the milestones: 7.0.0, Future Jul 20, 2022
@AraHaan
Copy link
Member

AraHaan commented Sep 3, 2022

I think a Timer that can accept an DateTimeOffset / DateTime would be great for this (aka is based on those types) and not like the ones in: System.Timers.Timer and System.Threading.Timer.

Likewise I do have need to patch https://github.com/dotnet/runtime/blob/main/src/libraries/System.ComponentModel.TypeConverter/src/System/Timers/Timer.cs to max at uint.MaxValue and not int.MaxValue because for example a TimeSpan or DateTimeOffset that differs by 1 month can be a breaking for me (as I do have need for such a thing where I want to run a task only once a month inside of a program of mine).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Extensions-Caching Bottom Up Work Not part of a theme, epic, or user story Priority:3 Work that is nice to have tenet-performance Performance related issue User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

No branches or pull requests

8 participants