From 62b4a1fca122a1c1680963687501c0c372be2b94 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 2 Dec 2020 18:04:26 +0100 Subject: [PATCH] Reduce CacheEntry size (#45410) * replace 3 boolean fields with one enum flag field * use auto property for AbsoluteExpiration * simplify the code by using expression bodies * don't use internal fields of CacheEntry, the public properties are enough * expose private methods of MemoryCache and replace the usage of delegates with dependency to MemoryCache 2 fields less for MemoryCache and three for every CacheEntry --- .../src/CacheEntry.cs | 165 +++++++----------- .../src/MemoryCache.cs | 48 ++--- .../tests/CacheEntryScopeExpirationTests.cs | 34 ++-- 3 files changed, 94 insertions(+), 153 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 7b2a098d95057..47e3c5c8fd417 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -13,86 +13,44 @@ namespace Microsoft.Extensions.Caching.Memory { internal class CacheEntry : ICacheEntry { - private bool _disposed; private static readonly Action ExpirationCallback = ExpirationTokensExpired; - private readonly Action _notifyCacheOfExpiration; - private readonly Action _notifyCacheEntryCommit; + + private readonly object _lock = new object(); + private readonly MemoryCache _cache; + private IList _expirationTokenRegistrations; private IList _postEvictionCallbacks; - private bool _isExpired; - private readonly ILogger _logger; - - internal IList _expirationTokens; - internal DateTimeOffset? _absoluteExpiration; - internal TimeSpan? _absoluteExpirationRelativeToNow; + private IList _expirationTokens; + private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; private IDisposable _scope; private object _value; - private bool _valueHasBeenSet; - - internal readonly object _lock = new object(); + private int _state; // actually a [Flag] enum called "State" - internal CacheEntry( - object key, - Action notifyCacheEntryCommit, - Action notifyCacheOfExpiration, - ILogger logger) + internal CacheEntry(object key, MemoryCache memoryCache) { - if (key == null) - { - throw new ArgumentNullException(nameof(key)); - } - - if (notifyCacheEntryCommit == null) - { - throw new ArgumentNullException(nameof(notifyCacheEntryCommit)); - } - - if (notifyCacheOfExpiration == null) - { - throw new ArgumentNullException(nameof(notifyCacheOfExpiration)); - } - - if (logger == null) - { - throw new ArgumentNullException(nameof(logger)); - } - - Key = key; - _notifyCacheEntryCommit = notifyCacheEntryCommit; - _notifyCacheOfExpiration = notifyCacheOfExpiration; - + Key = key ?? throw new ArgumentNullException(nameof(key)); + _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); _scope = CacheEntryHelper.EnterScope(this); - _logger = logger; } /// /// Gets or sets an absolute expiration date for the cache entry. /// - public DateTimeOffset? AbsoluteExpiration - { - get - { - return _absoluteExpiration; - } - set - { - _absoluteExpiration = value; - } - } + public DateTimeOffset? AbsoluteExpiration { get; set; } /// /// Gets or sets an absolute expiration time, relative to now. /// public TimeSpan? AbsoluteExpirationRelativeToNow { - get - { - return _absoluteExpirationRelativeToNow; - } + get => _absoluteExpirationRelativeToNow; set { + // this method does not set AbsoluteExpiration as it would require calling Clock.UtcNow twice: + // once here and once in MemoryCache.SetEntry + if (value <= TimeSpan.Zero) { throw new ArgumentOutOfRangeException( @@ -111,10 +69,7 @@ public TimeSpan? AbsoluteExpirationRelativeToNow /// public TimeSpan? SlidingExpiration { - get - { - return _slidingExpiration; - } + get => _slidingExpiration; set { if (value <= TimeSpan.Zero) @@ -124,6 +79,7 @@ public TimeSpan? SlidingExpiration value, "The sliding expiration value must be positive."); } + _slidingExpiration = value; } } @@ -131,34 +87,12 @@ public TimeSpan? SlidingExpiration /// /// Gets the instances which cause the cache entry to expire. /// - public IList ExpirationTokens - { - get - { - if (_expirationTokens == null) - { - _expirationTokens = new List(); - } - - return _expirationTokens; - } - } + public IList ExpirationTokens => _expirationTokens ??= new List(); /// /// Gets or sets the callbacks will be fired after the cache entry is evicted from the cache. /// - public IList PostEvictionCallbacks - { - get - { - if (_postEvictionCallbacks == null) - { - _postEvictionCallbacks = new List(); - } - - return _postEvictionCallbacks; - } - } + public IList PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); /// /// Gets or sets the priority for keeping the cache entry in the cache during a @@ -191,7 +125,7 @@ public object Value set { _value = value; - _valueHasBeenSet = true; + IsValueSet = true; } } @@ -199,11 +133,17 @@ public object Value 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); } + public void Dispose() { - if (!_disposed) + if (!IsDisposed) { - _disposed = true; + IsDisposed = true; // Ensure the _scope reference is cleared because it can reference other CacheEntry instances. // This CacheEntry is going to be put into a MemoryCache, and we don't want to root unnecessary objects. @@ -213,9 +153,9 @@ public void Dispose() // 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 (_valueHasBeenSet) + if (IsValueSet) { - _notifyCacheEntryCommit(this); + _cache.SetEntry(this); if (CanPropagateOptions()) { @@ -226,10 +166,7 @@ public void Dispose() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal bool CheckExpired(in DateTimeOffset now) - { - return _isExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); - } + internal bool CheckExpired(in DateTimeOffset now) => IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); internal void SetExpired(EvictionReason reason) { @@ -237,13 +174,13 @@ internal void SetExpired(EvictionReason reason) { EvictionReason = reason; } - _isExpired = true; + IsExpired = true; DetachTokens(); } private bool CheckForExpiredTime(in DateTimeOffset now) { - if (!_absoluteExpiration.HasValue && !_slidingExpiration.HasValue) + if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue) { return false; } @@ -252,7 +189,7 @@ private bool CheckForExpiredTime(in DateTimeOffset now) bool FullCheck(in DateTimeOffset offset) { - if (_absoluteExpiration.HasValue && _absoluteExpiration.Value <= offset) + if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value <= offset) { SetExpired(EvictionReason.Expired); return true; @@ -323,12 +260,13 @@ private static void ExpirationTokensExpired(object obj) { var entry = (CacheEntry)state; entry.SetExpired(EvictionReason.TokenExpired); - entry._notifyCacheOfExpiration(entry); + entry._cache.EntryExpired(entry); }, 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; @@ -373,12 +311,13 @@ private static void InvokeCallbacks(CacheEntry entry) catch (Exception e) { // This will be invoked on a background thread, don't let it throw. - entry._logger.LogError(e, "EvictionCallback invoked failed"); + entry._cache._logger.LogError(e, "EvictionCallback invoked failed"); } } } - internal bool CanPropagateOptions() => _expirationTokens != null || _absoluteExpiration.HasValue; + // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) + internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) { @@ -403,13 +342,33 @@ internal void PropagateOptions(CacheEntry parent) } } - if (_absoluteExpiration.HasValue) + if (AbsoluteExpiration.HasValue) { - if (!parent._absoluteExpiration.HasValue || _absoluteExpiration < parent._absoluteExpiration) + if (!parent.AbsoluteExpiration.HasValue || AbsoluteExpiration < parent.AbsoluteExpiration) { - parent._absoluteExpiration = _absoluteExpiration; + parent.AbsoluteExpiration = AbsoluteExpiration; } } } + + private void Set(State option, bool value) + { + int before, after; + + do + { + before = _state; + after = value ? (_state | (int)option) : (_state & ~(int)option); + } while (Interlocked.CompareExchange(ref _state, after, before) != before); + } + + [Flags] + private enum State + { + Default = 0, + IsValueSet = 1 << 0, + IsExpired = 1 << 1, + IsDisposed = 1 << 2, + } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 8075a08dee5a8..e863e4d1a221d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -20,17 +20,13 @@ namespace Microsoft.Extensions.Caching.Memory /// public class MemoryCache : IMemoryCache { + internal readonly ILogger _logger; + + private readonly MemoryCacheOptions _options; private readonly ConcurrentDictionary _entries; + private long _cacheSize; private bool _disposed; - private readonly ILogger _logger; - - // We store the delegates locally to prevent allocations - // every time a new CacheEntry is created. - private readonly Action _setEntry; - private readonly Action _entryExpirationNotification; - - private readonly MemoryCacheOptions _options; private DateTimeOffset _lastExpirationScan; /// @@ -61,8 +57,6 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory _logger = loggerFactory.CreateLogger(); _entries = new ConcurrentDictionary(); - _setEntry = SetEntry; - _entryExpirationNotification = EntryExpired; if (_options.Clock == null) { @@ -75,18 +69,12 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory /// /// Cleans up the background collection events. /// - ~MemoryCache() - { - Dispose(false); - } + ~MemoryCache() => Dispose(false); /// /// Gets the count of the current entries for diagnostic purposes. /// - public int Count - { - get { return _entries.Count; } - } + public int Count => _entries.Count; // internal for testing internal long Size { get => Interlocked.Read(ref _cacheSize); } @@ -97,18 +85,12 @@ public int Count public ICacheEntry CreateEntry(object key) { CheckDisposed(); - ValidateCacheKey(key); - return new CacheEntry( - key, - _setEntry, - _entryExpirationNotification, - _logger - ); + return new CacheEntry(key, this); } - private void SetEntry(CacheEntry entry) + internal void SetEntry(CacheEntry entry) { if (_disposed) { @@ -124,13 +106,13 @@ private void SetEntry(CacheEntry entry) DateTimeOffset utcNow = _options.Clock.UtcNow; DateTimeOffset? absoluteExpiration = null; - if (entry._absoluteExpirationRelativeToNow.HasValue) + if (entry.AbsoluteExpirationRelativeToNow.HasValue) { - absoluteExpiration = utcNow + entry._absoluteExpirationRelativeToNow; + absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; } - else if (entry._absoluteExpiration.HasValue) + else if (entry.AbsoluteExpiration.HasValue) { - absoluteExpiration = entry._absoluteExpiration; + absoluteExpiration = entry.AbsoluteExpiration; } // Applying the option's absolute expiration only if it's not already smaller. @@ -138,9 +120,9 @@ private void SetEntry(CacheEntry entry) // it was set by cascading it to its parent. if (absoluteExpiration.HasValue) { - if (!entry._absoluteExpiration.HasValue || absoluteExpiration.Value < entry._absoluteExpiration.Value) + if (!entry.AbsoluteExpiration.HasValue || absoluteExpiration.Value < entry.AbsoluteExpiration.Value) { - entry._absoluteExpiration = absoluteExpiration; + entry.AbsoluteExpiration = absoluteExpiration; } } @@ -306,7 +288,7 @@ private void RemoveEntry(CacheEntry entry) } } - private void EntryExpired(CacheEntry entry) + internal void EntryExpired(CacheEntry entry) { // TODO: For efficiency consider processing these expirations in batches. RemoveEntry(entry); diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index 2451e8b19b3a3..3ca46d0c024d6 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -41,8 +41,8 @@ public void SetPopulates_ExpirationTokens_IntoScopedLink() cache.Set(key, obj, new MemoryCacheEntryOptions().AddExpirationToken(expirationToken)); } - Assert.Single(((CacheEntry)entry)._expirationTokens); - Assert.Null(((CacheEntry)entry)._absoluteExpiration); + Assert.Single(((CacheEntry)entry).ExpirationTokens); + Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -62,9 +62,9 @@ public void SetPopulates_AbsoluteExpiration_IntoScopeLink() cache.Set(key, obj, new MemoryCacheEntryOptions().SetAbsoluteExpiration(time)); } - Assert.Null(((CacheEntry)entry)._expirationTokens); - Assert.NotNull(((CacheEntry)entry)._absoluteExpiration); - Assert.Equal(time, ((CacheEntry)entry)._absoluteExpiration); + Assert.Empty(((CacheEntry)entry).ExpirationTokens); + Assert.NotNull(((CacheEntry)entry).AbsoluteExpiration); + Assert.Equal(time, ((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -331,8 +331,8 @@ public void GetWithImplicitLinkPopulatesExpirationTokens() Assert.Null(CacheEntryHelper.Current); - Assert.Single(((CacheEntry)entry)._expirationTokens); - Assert.Null(((CacheEntry)entry)._absoluteExpiration); + Assert.Single(((CacheEntry)entry).ExpirationTokens); + Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -365,10 +365,10 @@ public void LinkContextsCanNest() Assert.Null(CacheEntryHelper.Current); - Assert.Single(((CacheEntry)entry1)._expirationTokens); - Assert.Null(((CacheEntry)entry1)._absoluteExpiration); - Assert.Single(((CacheEntry)entry)._expirationTokens); - Assert.Null(((CacheEntry)entry)._absoluteExpiration); + Assert.Single(((CacheEntry)entry1).ExpirationTokens); + Assert.Null(((CacheEntry)entry1).AbsoluteExpiration); + Assert.Single(((CacheEntry)entry).ExpirationTokens); + Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -402,13 +402,13 @@ public void NestedLinkContextsCanAggregate() } } - Assert.Equal(2, ((CacheEntry)entry1)._expirationTokens.Count()); - Assert.NotNull(((CacheEntry)entry1)._absoluteExpiration); - Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(10), ((CacheEntry)entry1)._absoluteExpiration); + Assert.Equal(2, ((CacheEntry)entry1).ExpirationTokens.Count()); + Assert.NotNull(((CacheEntry)entry1).AbsoluteExpiration); + Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(10), ((CacheEntry)entry1).AbsoluteExpiration); - Assert.Single(((CacheEntry)entry2)._expirationTokens); - Assert.NotNull(((CacheEntry)entry2)._absoluteExpiration); - Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(15), ((CacheEntry)entry2)._absoluteExpiration); + Assert.Single(((CacheEntry)entry2).ExpirationTokens); + Assert.NotNull(((CacheEntry)entry2).AbsoluteExpiration); + Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(15), ((CacheEntry)entry2).AbsoluteExpiration); } [Fact]