-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 #45962
Reduce CacheEntry size #45962
Conversation
…ationTokens to separate type to reduce the typicall cache entry size
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsBased on a customer feedback received in #45592 (comment) I decided to try to shrink Main changes:
The CPU time has not changed, but allocations did:
|
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs
Outdated
Show resolved
Hide resolved
Question: why is the atomic interlocked-exchange needed? If you update the whole 4-byte field, it needs to be atomic, yes. But if you just update the individual field, shouldn't the JIT guarantee that only the partial bytes are changed in an atomic way?
results in ...
which is just a single byte update... and does not change the surrounding bytes. It's an atomic operation on the CPU. Fields can be changed independently and don't interfere with another. I would get rid of _state and update the fields normally. The JIT should account for the correct atomic field update. Unless I'm overlooking something here. |
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@hopperpl I've analysed the code again (and the way state can be updated) and you were right, it was not needed. I've added a simple test to ensure that. |
If you need to avoid a race condition when different flags are set/unset at the same time, use Interlocked.Or, Interlocked.And (on the 32-bit _state value). This atomic operation won't interfere with the other fields.
|
@hopperpl thanks! I've analysed the code and there is no need for it (and this particular lib targets .NET Standard 2.0 and these methods were introduced in .NET 5) |
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Outdated
Show resolved
Hide resolved
|
||
internal bool CheckForExpiredTokens(CacheEntry cacheEntry) | ||
{ | ||
if (_expirationTokens != null) |
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.
Why doesn't this need a lock like the other two methods?
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.
{ | ||
lock (this) | ||
{ | ||
lock (parentEntry.GetOrCreateTokens()) |
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.
Where else is this locked on?
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.
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.
I'm not sure how that answers my question. My primary concern would be if it'd ever be possible for something to take the second lock and then try to take the first, creating a priority inversion and deadlock potential. I'm simply wondering if that's possible. Does the child/parent nature of this guarantee locks are always acquired in the same order?
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.
Does the child/parent nature of this guarantee locks are always acquired in the same order?
I think it does in the case when the cache item is being added to the cache (by calling the Dispose method):
runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
Lines 155 to 157 in 6e111ea
if (_previous != null && CanPropagateOptions()) | |
{ | |
PropagateOptions(_previous); |
but I am not 100% sure if it would not be possible to create a code that does this and abuses the second possible call to this method from the Cache.TryGetValue
method (which is very unintuitive for me):
runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Lines 237 to 239 in 6e111ea
// When this entry is retrieved in the scope of creating another entry, | |
// that entry needs a copy of these expiration tokens. | |
entry.PropagateOptions(CacheEntryHelper.Current); |
FWIW I have not changed the locking order, it's the same as it used to be so far:
runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
Lines 331 to 340 in 6e111ea
lock (_lock) | |
{ | |
lock (parent._lock) | |
{ | |
foreach (IChangeToken expirationToken in _expirationTokens) | |
{ | |
parent.AddExpirationToken(expirationToken); | |
} | |
} | |
} |
{ | ||
lock (parentEntry.GetOrCreateTokens()) | ||
{ | ||
foreach (IChangeToken expirationToken in _expirationTokens) |
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.
Why in some cases are we using a for loop with direct indexing to enumerate the contents and in others we're using foreach?
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.
I don't know as I am not the author of the original code. To minimize the diff and make the review easier I've decided to change as few things as possible
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> Co-authored-by: Stephen Toub <stoub@microsoft.com>
…rom List to IList)
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.
LGTM. Nice improvement here!
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Based on a customer feedback received in #45592 (comment) I decided to try to shrink
CacheEntry
a little bit more. I was able to reduce the managed memory needed for allocating aCacheEntry
from 280 to 224 bytes (on x64).Main changes:
2x sizeof(int)
), store the information in an union struct calledCacheEntryState
. It now consists of two bytes for the mentioned enums and one byte for all the boolean flags. There is one more that is still not used, so we can add more stuff in the future ;). This looks like a magic and @roji is responsible for teaching me this kind of crazy stuff ;)CacheEntryTokens
. This allows us for saving space if they are not used (the most common scenario)_lock
field, use the field that storesCacheEntryTokens
for locking instead (it guards the access to the tokens and the lock is needed only to work with the tokens)The CPU time has not changed, but allocations did: