From: Adam Sitnik Date: Thu, 7 Jan 2021 19:57:33 +0000 (+0100) Subject: Reduce CacheEntry size (#45962) X-Git-Tag: submit/tizen/20210909.063632~3895 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=533a807b74c3b9258bb5355eb94c7fb48918f4b9;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Reduce CacheEntry size (#45962) * reduce the size of CacheEntry instance by 2xsizeof(int) * remove _lock field and use "this" for locking instead * move _expirationTokenRegistrations, _postEvictionCallbacks and _expirationTokens to separate type to reduce the typical cache entry size * apply code review suggestion: don't use Enum.HasFlag Co-authored-by: Jan Kotas * Apply suggestions from code review Co-authored-by: Eric Erhardt Co-authored-by: Stephen Toub Co-authored-by: Jan Kotas Co-authored-by: Eric Erhardt Co-authored-by: Stephen Toub --- diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs new file mode 100644 index 0000000..77aac7b --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -0,0 +1,61 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +namespace Microsoft.Extensions.Caching.Memory +{ + internal partial class CacheEntry + { + // this type exists just to reduce CacheEntry size by replacing many enum & boolean fields with one of a size of Int32 + private struct CacheEntryState + { + private byte _flags; + private byte _evictionReason; + private byte _priority; + + internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority; + + internal bool IsDisposed + { + get => ((Flags)_flags & Flags.IsDisposed) != 0; + set => SetFlag(Flags.IsDisposed, value); + } + + internal bool IsExpired + { + get => ((Flags)_flags & Flags.IsExpired) != 0; + set => SetFlag(Flags.IsExpired, value); + } + + internal bool IsValueSet + { + get => ((Flags)_flags & Flags.IsValueSet) != 0; + set => SetFlag(Flags.IsValueSet, value); + } + + internal EvictionReason EvictionReason + { + get => (EvictionReason)_evictionReason; + set => _evictionReason = (byte)value; + } + + internal CacheItemPriority Priority + { + get => (CacheItemPriority)_priority; + set => _priority = (byte)value; + } + + private void SetFlag(Flags option, bool value) => _flags = (byte)(value ? (_flags | (byte)option) : (_flags & ~(byte)option)); + + [Flags] + private enum Flags : byte + { + Default = 0, + IsValueSet = 1 << 0, + IsExpired = 1 << 1, + IsDisposed = 1 << 2, + } + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs new file mode 100644 index 0000000..5800a40 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -0,0 +1,139 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.Extensions.Caching.Memory +{ + internal partial class CacheEntry + { + // this type exists just to reduce average CacheEntry size + // which typically is not using expiration tokens or callbacks + private sealed class CacheEntryTokens + { + private List _expirationTokens; + private List _expirationTokenRegistrations; + private List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typical CacheEntry size + + internal List ExpirationTokens => _expirationTokens ??= new List(); + internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); + + internal void AttachTokens() + { + if (_expirationTokens != null) + { + lock (this) + { + for (int i = 0; i < _expirationTokens.Count; i++) + { + IChangeToken expirationToken = _expirationTokens[i]; + if (expirationToken.ActiveChangeCallbacks) + { + _expirationTokenRegistrations ??= new List(1); + IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this); + _expirationTokenRegistrations.Add(registration); + } + } + } + } + } + + internal bool CheckForExpiredTokens(CacheEntry cacheEntry) + { + if (_expirationTokens != null) + { + for (int i = 0; i < _expirationTokens.Count; i++) + { + IChangeToken expiredToken = _expirationTokens[i]; + if (expiredToken.HasChanged) + { + cacheEntry.SetExpired(EvictionReason.TokenExpired); + return true; + } + } + } + return false; + } + + internal bool CanPropagateTokens() => _expirationTokens != null; + + internal void PropagateTokens(CacheEntry parentEntry) + { + if (_expirationTokens != null) + { + lock (this) + { + lock (parentEntry.GetOrCreateTokens()) + { + foreach (IChangeToken expirationToken in _expirationTokens) + { + parentEntry.AddExpirationToken(expirationToken); + } + } + } + } + } + + internal void DetachTokens() + { + // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock + // instead we are checking for _expirationTokens, because if they are not null, then _expirationTokenRegistrations might also be not null + if (_expirationTokens != null) + { + lock (this) + { + List registrations = _expirationTokenRegistrations; + if (registrations != null) + { + _expirationTokenRegistrations = null; + for (int i = 0; i < registrations.Count; i++) + { + IDisposable registration = registrations[i]; + registration.Dispose(); + } + } + } + } + } + + internal void InvokeEvictionCallbacks(CacheEntry cacheEntry) + { + if (_postEvictionCallbacks != null) + { + Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), cacheEntry, + CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); + } + } + + private static void InvokeCallbacks(CacheEntry entry) + { + List callbackRegistrations = Interlocked.Exchange(ref entry._tokens._postEvictionCallbacks, null); + + if (callbackRegistrations == null) + { + return; + } + + for (int i = 0; i < callbackRegistrations.Count; i++) + { + PostEvictionCallbackRegistration registration = callbackRegistrations[i]; + + try + { + registration.EvictionCallback?.Invoke(entry.Key, entry.Value, entry.EvictionReason, registration.State); + } + catch (Exception e) + { + // This will be invoked on a background thread, don't let it throw. + entry._cache._logger.LogError(e, "EvictionCallback invoked failed"); + } + } + } + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 6f91cb2..606f0ad 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -6,33 +6,30 @@ using System.Collections.Generic; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Primitives; namespace Microsoft.Extensions.Caching.Memory { - internal class CacheEntry : ICacheEntry + internal partial class CacheEntry : ICacheEntry { private static readonly Action ExpirationCallback = ExpirationTokensExpired; - private readonly object _lock = new object(); private readonly MemoryCache _cache; - private IList _expirationTokenRegistrations; - private IList _postEvictionCallbacks; - private IList _expirationTokens; + private CacheEntryTokens _tokens; // might be null if user is not using the tokens or callbacks private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; private CacheEntry _previous; // this field is not null only before the entry is added to the cache private object _value; - private int _state; // actually a [Flag] enum called "State" + private CacheEntryState _state; internal CacheEntry(object key, MemoryCache memoryCache) { Key = key ?? throw new ArgumentNullException(nameof(key)); _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); _previous = CacheEntryHelper.EnterScope(this); + _state = new CacheEntryState(CacheItemPriority.Normal); } /// @@ -87,18 +84,18 @@ namespace Microsoft.Extensions.Caching.Memory /// /// Gets the instances which cause the cache entry to expire. /// - public IList ExpirationTokens => _expirationTokens ??= new List(); + public IList ExpirationTokens => GetOrCreateTokens().ExpirationTokens; /// /// Gets or sets the callbacks will be fired after the cache entry is evicted from the cache. /// - public IList PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); + public IList PostEvictionCallbacks => GetOrCreateTokens().PostEvictionCallbacks; /// /// Gets or sets the priority for keeping the cache entry in the cache during a /// memory pressure triggered cleanup. The default is . /// - public CacheItemPriority Priority { get; set; } = CacheItemPriority.Normal; + public CacheItemPriority Priority { get => _state.Priority; set => _state.Priority = value; } /// /// Gets or sets the size of the cache entry value. @@ -125,30 +122,24 @@ namespace Microsoft.Extensions.Caching.Memory set { _value = value; - IsValueSet = true; + _state.IsValueSet = true; } } internal DateTimeOffset LastAccessed { get; set; } - internal EvictionReason EvictionReason { get; private set; } - - private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } - - private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } - - private bool IsValueSet { get => ((State)_state).HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); } + internal EvictionReason EvictionReason { get => _state.EvictionReason; private set => _state.EvictionReason = value; } public void Dispose() { - if (!IsDisposed) + if (!_state.IsDisposed) { - IsDisposed = true; + _state.IsDisposed = true; // Don't commit or propagate options if the CacheEntry Value was never set. // We assume an exception occurred causing the caller to not set the Value successfully, // so don't use this entry. - if (IsValueSet) + if (_state.IsValueSet) { _cache.SetEntry(this); @@ -163,8 +154,11 @@ namespace Microsoft.Extensions.Caching.Memory } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal bool CheckExpired(in DateTimeOffset now) => IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling + internal bool CheckExpired(in DateTimeOffset now) + => _state.IsExpired + || CheckForExpiredTime(now) + || (_tokens != null && _tokens.CheckForExpiredTokens(this)); internal void SetExpired(EvictionReason reason) { @@ -172,10 +166,11 @@ namespace Microsoft.Extensions.Caching.Memory { EvictionReason = reason; } - IsExpired = true; - DetachTokens(); + _state.IsExpired = true; + _tokens?.DetachTokens(); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling private bool CheckForExpiredTime(in DateTimeOffset now) { if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue) @@ -204,52 +199,7 @@ namespace Microsoft.Extensions.Caching.Memory } } - private bool CheckForExpiredTokens() - { - if (_expirationTokens == null) - { - return false; - } - - return CheckTokens(); - - bool CheckTokens() - { - for (int i = 0; i < _expirationTokens.Count; i++) - { - IChangeToken expiredToken = _expirationTokens[i]; - if (expiredToken.HasChanged) - { - SetExpired(EvictionReason.TokenExpired); - return true; - } - } - return false; - } - } - - internal void AttachTokens() - { - if (_expirationTokens != null) - { - lock (_lock) - { - for (int i = 0; i < _expirationTokens.Count; i++) - { - IChangeToken expirationToken = _expirationTokens[i]; - if (expirationToken.ActiveChangeCallbacks) - { - if (_expirationTokenRegistrations == null) - { - _expirationTokenRegistrations = new List(1); - } - IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this); - _expirationTokenRegistrations.Add(registration); - } - } - } - } - } + internal void AttachTokens() => _tokens?.AttachTokens(); private static void ExpirationTokensExpired(object obj) { @@ -262,60 +212,11 @@ namespace Microsoft.Extensions.Caching.Memory }, obj, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); } - private void DetachTokens() - { - // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock - lock (_lock) - { - IList registrations = _expirationTokenRegistrations; - if (registrations != null) - { - _expirationTokenRegistrations = null; - for (int i = 0; i < registrations.Count; i++) - { - IDisposable registration = registrations[i]; - registration.Dispose(); - } - } - } - } - - internal void InvokeEvictionCallbacks() - { - if (_postEvictionCallbacks != null) - { - Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), this, - CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); - } - } - - private static void InvokeCallbacks(CacheEntry entry) - { - IList callbackRegistrations = Interlocked.Exchange(ref entry._postEvictionCallbacks, null); - - if (callbackRegistrations == null) - { - return; - } - - for (int i = 0; i < callbackRegistrations.Count; i++) - { - PostEvictionCallbackRegistration registration = callbackRegistrations[i]; - - try - { - registration.EvictionCallback?.Invoke(entry.Key, entry.Value, entry.EvictionReason, registration.State); - } - catch (Exception e) - { - // This will be invoked on a background thread, don't let it throw. - entry._cache._logger.LogError(e, "EvictionCallback invoked failed"); - } - } - } + internal void InvokeEvictionCallbacks() => _tokens?.InvokeEvictionCallbacks(this); // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) - internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue; + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling + internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanPropagateTokens()) || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) { @@ -326,19 +227,7 @@ namespace Microsoft.Extensions.Caching.Memory // Copy expiration tokens and AbsoluteExpiration to the cache entries hierarchy. // We do this regardless of it gets cached because the tokens are associated with the value we'll return. - if (_expirationTokens != null) - { - lock (_lock) - { - lock (parent._lock) - { - foreach (IChangeToken expirationToken in _expirationTokens) - { - parent.AddExpirationToken(expirationToken); - } - } - } - } + _tokens?.PropagateTokens(parent); if (AbsoluteExpiration.HasValue) { @@ -349,24 +238,15 @@ namespace Microsoft.Extensions.Caching.Memory } } - private void Set(State option, bool value) + private CacheEntryTokens GetOrCreateTokens() { - int before, after; - - do + if (_tokens != null) { - before = _state; - after = value ? (_state | (int)option) : (_state & ~(int)option); - } while (Interlocked.CompareExchange(ref _state, after, before) != before); - } + return _tokens; + } - [Flags] - private enum State - { - Default = 0, - IsValueSet = 1 << 0, - IsExpired = 1 << 1, - IsDisposed = 1 << 2, + CacheEntryTokens result = new CacheEntryTokens(); + return Interlocked.CompareExchange(ref _tokens, result, null) ?? result; } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index 3ca46d0..9d34d63 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -482,5 +482,35 @@ namespace Microsoft.Extensions.Caching.Memory Assert.Null(cache.Get(key2)); Assert.Null(cache.Get(key4)); } + + [Fact] + public async Task OnceExpiredIsSetToTrueItRemainsTrue() + { + var cache = CreateCache(); + var entry = (CacheEntry)cache.CreateEntry("someKey"); + + await Task.WhenAll( + Task.Run(() => SetExpiredManyTimes(entry)), + Task.Run(() => SetExpiredManyTimes(entry))); + + Assert.True(entry.CheckExpired(DateTimeOffset.UtcNow)); + + static void SetExpiredManyTimes(CacheEntry cacheEntry) + { + var now = DateTimeOffset.UtcNow; + for (int i = 0; i < 1_000; i++) + { + cacheEntry.SetExpired(EvictionReason.Expired); // modifies CacheEntry._state + Assert.True(cacheEntry.CheckExpired(now)); + cacheEntry.Value = cacheEntry; // modifies CacheEntry._state + Assert.True(cacheEntry.CheckExpired(now)); + + cacheEntry.SetExpired(EvictionReason.Expired); // modifies CacheEntry._state + Assert.True(cacheEntry.CheckExpired(now)); + cacheEntry.Dispose(); // might modify CacheEntry._state + Assert.True(cacheEntry.CheckExpired(now)); + } + } + } } }