From f313e24942590ec7b81def448d5925b62d99cfad Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 08:05:19 +0100 Subject: [PATCH 01/13] replace 3 boolean fields with one enum flag field --- .../src/CacheEntry.cs | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 7b2a098d950573..92a241f03d2e72 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -13,13 +13,11 @@ 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 IList _expirationTokenRegistrations; private IList _postEvictionCallbacks; - private bool _isExpired; private readonly ILogger _logger; internal IList _expirationTokens; @@ -29,7 +27,7 @@ internal class CacheEntry : ICacheEntry private long? _size; private IDisposable _scope; private object _value; - private bool _valueHasBeenSet; + private State _state; internal readonly object _lock = new object(); @@ -191,7 +189,7 @@ public object Value set { _value = value; - _valueHasBeenSet = true; + IsValueSet = true; } } @@ -199,11 +197,17 @@ public object Value internal EvictionReason EvictionReason { get; private set; } + private bool IsDisposed { get => _state.HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } + + private bool IsExpired { get => _state.HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } + + private bool IsValueSet { get => _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,7 +217,7 @@ 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); @@ -228,7 +232,7 @@ public void Dispose() [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool CheckExpired(in DateTimeOffset now) { - return _isExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); + return IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); } internal void SetExpired(EvictionReason reason) @@ -237,7 +241,7 @@ internal void SetExpired(EvictionReason reason) { EvictionReason = reason; } - _isExpired = true; + IsExpired = true; DetachTokens(); } @@ -411,5 +415,16 @@ internal void PropagateOptions(CacheEntry parent) } } } + + private void Set(State option, bool value) => _state = value ? (_state | option) : (_state & ~option); + + [Flags] + private enum State + { + Default = 0, + IsValueSet = 1 << 0, + IsExpired = 1 << 1, + IsDisposed = 1 << 2, + } } } From 53259eba401f427aaa88e2daea88dff833b1bbb2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 08:23:57 +0100 Subject: [PATCH 02/13] use auto property for AbsoluteExpiration --- .../src/CacheEntry.cs | 25 ++++++------------- .../src/MemoryCache.cs | 8 +++--- .../tests/CacheEntryScopeExpirationTests.cs | 20 +++++++-------- 3 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 92a241f03d2e72..bb7895cff3b175 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -21,7 +21,6 @@ internal class CacheEntry : ICacheEntry private readonly ILogger _logger; internal IList _expirationTokens; - internal DateTimeOffset? _absoluteExpiration; internal TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; @@ -68,17 +67,7 @@ internal CacheEntry( /// /// 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. @@ -247,7 +236,7 @@ internal void SetExpired(EvictionReason reason) private bool CheckForExpiredTime(in DateTimeOffset now) { - if (!_absoluteExpiration.HasValue && !_slidingExpiration.HasValue) + if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue) { return false; } @@ -256,7 +245,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; @@ -382,7 +371,7 @@ private static void InvokeCallbacks(CacheEntry entry) } } - internal bool CanPropagateOptions() => _expirationTokens != null || _absoluteExpiration.HasValue; + internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) { @@ -407,11 +396,11 @@ 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; } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 8075a08dee5a8e..8e25f2be01259d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -128,9 +128,9 @@ private void SetEntry(CacheEntry entry) { 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 +138,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; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index 2451e8b19b3a32..b0c2b06911d583 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -42,7 +42,7 @@ public void SetPopulates_ExpirationTokens_IntoScopedLink() } Assert.Single(((CacheEntry)entry)._expirationTokens); - Assert.Null(((CacheEntry)entry)._absoluteExpiration); + Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -63,8 +63,8 @@ public void SetPopulates_AbsoluteExpiration_IntoScopeLink() } Assert.Null(((CacheEntry)entry)._expirationTokens); - Assert.NotNull(((CacheEntry)entry)._absoluteExpiration); - Assert.Equal(time, ((CacheEntry)entry)._absoluteExpiration); + Assert.NotNull(((CacheEntry)entry).AbsoluteExpiration); + Assert.Equal(time, ((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -332,7 +332,7 @@ public void GetWithImplicitLinkPopulatesExpirationTokens() Assert.Null(CacheEntryHelper.Current); Assert.Single(((CacheEntry)entry)._expirationTokens); - Assert.Null(((CacheEntry)entry)._absoluteExpiration); + Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -366,9 +366,9 @@ public void LinkContextsCanNest() Assert.Null(CacheEntryHelper.Current); Assert.Single(((CacheEntry)entry1)._expirationTokens); - Assert.Null(((CacheEntry)entry1)._absoluteExpiration); + Assert.Null(((CacheEntry)entry1).AbsoluteExpiration); Assert.Single(((CacheEntry)entry)._expirationTokens); - Assert.Null(((CacheEntry)entry)._absoluteExpiration); + Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } [Fact] @@ -403,12 +403,12 @@ 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.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.NotNull(((CacheEntry)entry2).AbsoluteExpiration); + Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(15), ((CacheEntry)entry2).AbsoluteExpiration); } [Fact] From 681c2b2c178b37309ff40f58d63420feccf874b6 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 08:33:04 +0100 Subject: [PATCH 03/13] simplify the code by using expression bodies --- .../src/CacheEntry.cs | 104 ++---------------- 1 file changed, 12 insertions(+), 92 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index bb7895cff3b175..143c05f92deb20 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -36,32 +36,12 @@ internal CacheEntry( Action notifyCacheOfExpiration, ILogger logger) { - 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)); + _notifyCacheEntryCommit = notifyCacheEntryCommit ?? throw new ArgumentNullException(nameof(notifyCacheEntryCommit)); + _notifyCacheOfExpiration = notifyCacheOfExpiration ?? throw new ArgumentNullException(nameof(notifyCacheOfExpiration)); + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _scope = CacheEntryHelper.EnterScope(this); - _logger = logger; } /// @@ -74,22 +54,8 @@ internal CacheEntry( /// public TimeSpan? AbsoluteExpirationRelativeToNow { - get - { - return _absoluteExpirationRelativeToNow; - } - set - { - if (value <= TimeSpan.Zero) - { - throw new ArgumentOutOfRangeException( - nameof(AbsoluteExpirationRelativeToNow), - value, - "The relative expiration value must be positive."); - } - - _absoluteExpirationRelativeToNow = value; - } + get => _absoluteExpirationRelativeToNow; + set => _absoluteExpirationRelativeToNow = value > TimeSpan.Zero ? value : throw new ArgumentOutOfRangeException(nameof(AbsoluteExpirationRelativeToNow), value, "The relative expiration value must be positive."); } /// @@ -98,54 +64,19 @@ public TimeSpan? AbsoluteExpirationRelativeToNow /// public TimeSpan? SlidingExpiration { - get - { - return _slidingExpiration; - } - set - { - if (value <= TimeSpan.Zero) - { - throw new ArgumentOutOfRangeException( - nameof(SlidingExpiration), - value, - "The sliding expiration value must be positive."); - } - _slidingExpiration = value; - } + get => _slidingExpiration; + set => _slidingExpiration = value > TimeSpan.Zero ? value : throw new ArgumentOutOfRangeException(nameof(SlidingExpiration), value, "The sliding expiration value must be positive."); } /// /// 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 @@ -159,15 +90,7 @@ public IList PostEvictionCallbacks public long? Size { get => _size; - set - { - if (value < 0) - { - throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); - } - - _size = value; - } + set => _size = value >= 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); } public object Key { get; private set; } @@ -219,10 +142,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) { From 655091c356cd5b71fc29d5444cdb171ffe3807ba Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 08:36:10 +0100 Subject: [PATCH 04/13] don't take a lock if _expirationTokenRegistrations is already set to null in DetachTokens --- .../src/CacheEntry.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 143c05f92deb20..f6e2bffee64286 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -242,16 +242,19 @@ private static void ExpirationTokensExpired(object obj) private void DetachTokens() { - lock (_lock) + if (_expirationTokenRegistrations != null) { - IList registrations = _expirationTokenRegistrations; - if (registrations != null) + lock (_lock) { - _expirationTokenRegistrations = null; - for (int i = 0; i < registrations.Count; i++) + IList registrations = _expirationTokenRegistrations; + if (registrations != null) { - IDisposable registration = registrations[i]; - registration.Dispose(); + _expirationTokenRegistrations = null; + for (int i = 0; i < registrations.Count; i++) + { + IDisposable registration = registrations[i]; + registration.Dispose(); + } } } } From f4f0ad9b47d4ea874c3e1832086db441c93d97e0 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 09:11:55 +0100 Subject: [PATCH 05/13] don't use internal fields of CacheEntry, the public properties are enough --- .../src/CacheEntry.cs | 12 ++++++------ .../src/MemoryCache.cs | 4 ++-- .../tests/CacheEntryScopeExpirationTests.cs | 14 +++++++------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index f6e2bffee64286..b4c8be90bdcdd2 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -14,22 +14,22 @@ namespace Microsoft.Extensions.Caching.Memory internal class CacheEntry : ICacheEntry { private static readonly Action ExpirationCallback = ExpirationTokensExpired; + + private readonly object _lock = new object(); private readonly Action _notifyCacheOfExpiration; private readonly Action _notifyCacheEntryCommit; - private IList _expirationTokenRegistrations; - private IList _postEvictionCallbacks; private readonly ILogger _logger; - internal IList _expirationTokens; - internal TimeSpan? _absoluteExpirationRelativeToNow; + private IList _expirationTokenRegistrations; + private IList _postEvictionCallbacks; + private IList _expirationTokens; + private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; private IDisposable _scope; private object _value; private State _state; - internal readonly object _lock = new object(); - internal CacheEntry( object key, Action notifyCacheEntryCommit, diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 8e25f2be01259d..494e828336cfb5 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -124,9 +124,9 @@ 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) { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index b0c2b06911d583..3ca46d0c024d61 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -41,7 +41,7 @@ public void SetPopulates_ExpirationTokens_IntoScopedLink() cache.Set(key, obj, new MemoryCacheEntryOptions().AddExpirationToken(expirationToken)); } - Assert.Single(((CacheEntry)entry)._expirationTokens); + Assert.Single(((CacheEntry)entry).ExpirationTokens); Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } @@ -62,7 +62,7 @@ public void SetPopulates_AbsoluteExpiration_IntoScopeLink() cache.Set(key, obj, new MemoryCacheEntryOptions().SetAbsoluteExpiration(time)); } - Assert.Null(((CacheEntry)entry)._expirationTokens); + Assert.Empty(((CacheEntry)entry).ExpirationTokens); Assert.NotNull(((CacheEntry)entry).AbsoluteExpiration); Assert.Equal(time, ((CacheEntry)entry).AbsoluteExpiration); } @@ -331,7 +331,7 @@ public void GetWithImplicitLinkPopulatesExpirationTokens() Assert.Null(CacheEntryHelper.Current); - Assert.Single(((CacheEntry)entry)._expirationTokens); + Assert.Single(((CacheEntry)entry).ExpirationTokens); Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } @@ -365,9 +365,9 @@ public void LinkContextsCanNest() Assert.Null(CacheEntryHelper.Current); - Assert.Single(((CacheEntry)entry1)._expirationTokens); + Assert.Single(((CacheEntry)entry1).ExpirationTokens); Assert.Null(((CacheEntry)entry1).AbsoluteExpiration); - Assert.Single(((CacheEntry)entry)._expirationTokens); + Assert.Single(((CacheEntry)entry).ExpirationTokens); Assert.Null(((CacheEntry)entry).AbsoluteExpiration); } @@ -402,11 +402,11 @@ public void NestedLinkContextsCanAggregate() } } - Assert.Equal(2, ((CacheEntry)entry1)._expirationTokens.Count()); + 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.Single(((CacheEntry)entry2).ExpirationTokens); Assert.NotNull(((CacheEntry)entry2).AbsoluteExpiration); Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(15), ((CacheEntry)entry2).AbsoluteExpiration); } From 4b39c18eb118a49a59ba57138febc4f99afe4647 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 09:44:52 +0100 Subject: [PATCH 06/13] 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 | 21 +++++---------- .../src/MemoryCache.cs | 26 +++++-------------- 2 files changed, 13 insertions(+), 34 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index b4c8be90bdcdd2..82d09639ca0f80 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -16,9 +16,7 @@ internal class CacheEntry : ICacheEntry private static readonly Action ExpirationCallback = ExpirationTokensExpired; private readonly object _lock = new object(); - private readonly Action _notifyCacheOfExpiration; - private readonly Action _notifyCacheEntryCommit; - private readonly ILogger _logger; + private readonly MemoryCache _cache; private IList _expirationTokenRegistrations; private IList _postEvictionCallbacks; @@ -30,17 +28,10 @@ internal class CacheEntry : ICacheEntry private object _value; private State _state; - internal CacheEntry( - object key, - Action notifyCacheEntryCommit, - Action notifyCacheOfExpiration, - ILogger logger) + internal CacheEntry(object key, MemoryCache memoryCache) { Key = key ?? throw new ArgumentNullException(nameof(key)); - _notifyCacheEntryCommit = notifyCacheEntryCommit ?? throw new ArgumentNullException(nameof(notifyCacheEntryCommit)); - _notifyCacheOfExpiration = notifyCacheOfExpiration ?? throw new ArgumentNullException(nameof(notifyCacheOfExpiration)); - _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - + _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); _scope = CacheEntryHelper.EnterScope(this); } @@ -131,7 +122,7 @@ public void Dispose() // so don't use this entry. if (IsValueSet) { - _notifyCacheEntryCommit(this); + _cache.SetEntry(this); if (CanPropagateOptions()) { @@ -236,7 +227,7 @@ 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); } @@ -289,7 +280,7 @@ 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"); } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 494e828336cfb5..903f270b1a54d0 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) { @@ -97,18 +91,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) { @@ -306,7 +294,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); From f84b6cbde3b9a532a51740c05e9f20b97e8d37ff Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 09:59:22 +0100 Subject: [PATCH 07/13] AbsoluteExpirationRelativeToNow can internally just use AbsoluteExpiration and current time --- .../src/CacheEntry.cs | 7 ++-- .../src/MemoryCache.cs | 33 ++++++------------- 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 82d09639ca0f80..e8b27a608e5d66 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -21,7 +21,6 @@ internal class CacheEntry : ICacheEntry private IList _expirationTokenRegistrations; private IList _postEvictionCallbacks; private IList _expirationTokens; - private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; private IDisposable _scope; @@ -45,8 +44,10 @@ internal CacheEntry(object key, MemoryCache memoryCache) /// public TimeSpan? AbsoluteExpirationRelativeToNow { - get => _absoluteExpirationRelativeToNow; - set => _absoluteExpirationRelativeToNow = value > TimeSpan.Zero ? value : throw new ArgumentOutOfRangeException(nameof(AbsoluteExpirationRelativeToNow), value, "The relative expiration value must be positive."); + get => AbsoluteExpiration.HasValue ? AbsoluteExpiration.Value - _cache.GetUtcNow() : null; + set => AbsoluteExpiration = value > TimeSpan.Zero + ? (_cache.GetUtcNow() + value) + : throw new ArgumentOutOfRangeException(nameof(AbsoluteExpirationRelativeToNow), value, "The relative expiration value must be positive."); } /// diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 903f270b1a54d0..06047024e9ab97 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -57,13 +57,7 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory _logger = loggerFactory.CreateLogger(); _entries = new ConcurrentDictionary(); - - if (_options.Clock == null) - { - _options.Clock = new SystemClock(); - } - - _lastExpirationScan = _options.Clock.UtcNow; + _lastExpirationScan = _options.Clock?.UtcNow ?? DateTimeOffset.UtcNow; } /// @@ -96,6 +90,8 @@ public ICacheEntry CreateEntry(object key) return new CacheEntry(key, this); } + internal DateTimeOffset GetUtcNow() => _options.Clock?.UtcNow ?? DateTimeOffset.UtcNow; + internal void SetEntry(CacheEntry entry) { if (_disposed) @@ -109,17 +105,7 @@ internal void SetEntry(CacheEntry entry) throw new InvalidOperationException($"Cache entry must specify a value for {nameof(entry.Size)} when {nameof(_options.SizeLimit)} is set."); } - DateTimeOffset utcNow = _options.Clock.UtcNow; - - DateTimeOffset? absoluteExpiration = null; - if (entry.AbsoluteExpirationRelativeToNow.HasValue) - { - absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; - } - else if (entry.AbsoluteExpiration.HasValue) - { - absoluteExpiration = entry.AbsoluteExpiration; - } + DateTimeOffset? absoluteExpiration = entry.AbsoluteExpiration; // Applying the option's absolute expiration only if it's not already smaller. // This can be the case if a dependent cache entry has a smaller value, and @@ -132,6 +118,7 @@ internal void SetEntry(CacheEntry entry) } } + DateTimeOffset utcNow = GetUtcNow(); // Initialize the last access timestamp at the time the entry is added entry.LastAccessed = utcNow; @@ -227,7 +214,7 @@ public bool TryGetValue(object key, out object result) ValidateCacheKey(key); CheckDisposed(); - DateTimeOffset utcNow = _options.Clock.UtcNow; + DateTimeOffset utcNow = GetUtcNow(); if (_entries.TryGetValue(key, out CacheEntry entry)) { @@ -279,7 +266,7 @@ public void Remove(object key) entry.InvokeEvictionCallbacks(); } - StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); + StartScanForExpiredItemsIfNeeded(GetUtcNow()); } private void RemoveEntry(CacheEntry entry) @@ -298,7 +285,7 @@ internal void EntryExpired(CacheEntry entry) { // TODO: For efficiency consider processing these expirations in batches. RemoveEntry(entry); - StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); + StartScanForExpiredItemsIfNeeded(GetUtcNow()); } // Called by multiple actions to see how long it's been since we last checked for expired items. @@ -321,7 +308,7 @@ void ScheduleTask(DateTimeOffset utcNow) private static void ScanForExpiredItems(MemoryCache cache) { - DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow; + DateTimeOffset now = cache._lastExpirationScan = cache.GetUtcNow(); foreach (CacheEntry entry in cache._entries.Values) { if (entry.CheckExpired(now)) @@ -404,7 +391,7 @@ private void Compact(long removalSizeTarget, Func computeEntry long removedSize = 0; // Sort items by expired & priority status - DateTimeOffset now = _options.Clock.UtcNow; + DateTimeOffset now = GetUtcNow(); foreach (CacheEntry entry in _entries.Values) { if (entry.CheckExpired(now)) From 850676ea8371aacb99c940bdc2cd4375c6064013 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 10:00:21 +0100 Subject: [PATCH 08/13] more expressions, less code --- .../src/CacheEntry.cs | 6 +---- .../src/MemoryCache.cs | 24 ++++--------------- 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index e8b27a608e5d66..e5625138e909c8 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -90,11 +90,7 @@ public long? Size public object Value { get => _value; - set - { - _value = value; - IsValueSet = true; - } + set { _value = value; IsValueSet = true; } } internal DateTimeOffset LastAccessed { get; set; } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 06047024e9ab97..b53d277b47225c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -43,18 +43,8 @@ public MemoryCache(IOptions optionsAccessor) /// The factory used to create loggers. public MemoryCache(IOptions optionsAccessor, ILoggerFactory loggerFactory) { - if (optionsAccessor == null) - { - throw new ArgumentNullException(nameof(optionsAccessor)); - } - - if (loggerFactory == null) - { - throw new ArgumentNullException(nameof(loggerFactory)); - } - - _options = optionsAccessor.Value; - _logger = loggerFactory.CreateLogger(); + _options = optionsAccessor?.Value ?? throw new ArgumentNullException(nameof(optionsAccessor)); + _logger = loggerFactory?.CreateLogger() ?? throw new ArgumentNullException(nameof(loggerFactory)); _entries = new ConcurrentDictionary(); _lastExpirationScan = _options.Clock?.UtcNow ?? DateTimeOffset.UtcNow; @@ -63,18 +53,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); } From 359326ba74a24e18d322bfabefdf4663beb74bcf Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 11:29:07 +0100 Subject: [PATCH 09/13] so this is how nullable works with conditions... --- .../src/CacheEntry.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index e5625138e909c8..af2b4428d17d48 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -45,9 +45,15 @@ internal CacheEntry(object key, MemoryCache memoryCache) public TimeSpan? AbsoluteExpirationRelativeToNow { get => AbsoluteExpiration.HasValue ? AbsoluteExpiration.Value - _cache.GetUtcNow() : null; - set => AbsoluteExpiration = value > TimeSpan.Zero - ? (_cache.GetUtcNow() + value) - : throw new ArgumentOutOfRangeException(nameof(AbsoluteExpirationRelativeToNow), value, "The relative expiration value must be positive."); + set + { + if (value.HasValue) + { + AbsoluteExpiration = value.Value > TimeSpan.Zero + ? (_cache.GetUtcNow() + value) + : throw new ArgumentOutOfRangeException(nameof(AbsoluteExpirationRelativeToNow), value, "The relative expiration value must be positive."); + } + } } /// @@ -57,7 +63,7 @@ public TimeSpan? AbsoluteExpirationRelativeToNow public TimeSpan? SlidingExpiration { get => _slidingExpiration; - set => _slidingExpiration = value > TimeSpan.Zero ? value : throw new ArgumentOutOfRangeException(nameof(SlidingExpiration), value, "The sliding expiration value must be positive."); + set => _slidingExpiration = !value.HasValue || value > TimeSpan.Zero ? value : throw new ArgumentOutOfRangeException(nameof(SlidingExpiration), value, "The sliding expiration value must be positive."); } /// @@ -82,7 +88,7 @@ public TimeSpan? SlidingExpiration public long? Size { get => _size; - set => _size = value >= 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); + set => _size = !value.HasValue || value >= 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); } public object Key { get; private set; } From cfd3ed60223d69cac482edbdfdbc0cb56a134b7a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 16:14:02 +0100 Subject: [PATCH 10/13] address code review feedback: make Set a thread-safe method --- .../src/CacheEntry.cs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index af2b4428d17d48..5a28397e9120cd 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -25,7 +25,7 @@ internal class CacheEntry : ICacheEntry private long? _size; private IDisposable _scope; private object _value; - private State _state; + private int _state; // actually a [Flag] enum called "State" internal CacheEntry(object key, MemoryCache memoryCache) { @@ -103,11 +103,11 @@ public object Value internal EvictionReason EvictionReason { get; private set; } - private bool IsDisposed { get => _state.HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } + private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } - private bool IsExpired { get => _state.HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } + private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } - private bool IsValueSet { get => _state.HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); } + private bool IsValueSet { get => ((State)_state).HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); } public void Dispose() { @@ -322,7 +322,16 @@ internal void PropagateOptions(CacheEntry parent) } } - private void Set(State option, bool value) => _state = value ? (_state | option) : (_state & ~option); + 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 From 628546a9924aa86b7784f6a2a03eec8e4d701a82 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 17:15:39 +0100 Subject: [PATCH 11/13] address code review feedback --- .../src/CacheEntry.cs | 58 ++++++++++++++----- .../src/MemoryCache.cs | 15 ++++- 2 files changed, 54 insertions(+), 19 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 5a28397e9120cd..0f7f24b1156abf 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -49,9 +49,15 @@ public TimeSpan? AbsoluteExpirationRelativeToNow { if (value.HasValue) { - AbsoluteExpiration = value.Value > TimeSpan.Zero - ? (_cache.GetUtcNow() + value) - : throw new ArgumentOutOfRangeException(nameof(AbsoluteExpirationRelativeToNow), value, "The relative expiration value must be positive."); + if (value <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(AbsoluteExpirationRelativeToNow), + value, + "The relative expiration value must be positive."); + } + + AbsoluteExpiration = _cache.GetUtcNow() + value; } } } @@ -63,7 +69,18 @@ public TimeSpan? AbsoluteExpirationRelativeToNow public TimeSpan? SlidingExpiration { get => _slidingExpiration; - set => _slidingExpiration = !value.HasValue || value > TimeSpan.Zero ? value : throw new ArgumentOutOfRangeException(nameof(SlidingExpiration), value, "The sliding expiration value must be positive."); + set + { + if (value <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(SlidingExpiration), + value, + "The sliding expiration value must be positive."); + } + + _slidingExpiration = value; + } } /// @@ -88,7 +105,15 @@ public TimeSpan? SlidingExpiration public long? Size { get => _size; - set => _size = !value.HasValue || value >= 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); + set + { + if (value < 0) + { + throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); + } + + _size = value; + } } public object Key { get; private set; } @@ -96,7 +121,11 @@ public long? Size public object Value { get => _value; - set { _value = value; IsValueSet = true; } + set + { + _value = value; + IsValueSet = true; + } } internal DateTimeOffset LastAccessed { get; set; } @@ -236,19 +265,16 @@ private static void ExpirationTokensExpired(object obj) private void DetachTokens() { - if (_expirationTokenRegistrations != null) + lock (_lock) { - lock (_lock) + IList registrations = _expirationTokenRegistrations; + if (registrations != null) { - IList registrations = _expirationTokenRegistrations; - if (registrations != null) + _expirationTokenRegistrations = null; + for (int i = 0; i < registrations.Count; i++) { - _expirationTokenRegistrations = null; - for (int i = 0; i < registrations.Count; i++) - { - IDisposable registration = registrations[i]; - registration.Dispose(); - } + IDisposable registration = registrations[i]; + registration.Dispose(); } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index b53d277b47225c..e0f8a93674c49f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -7,7 +7,6 @@ using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; @@ -43,8 +42,18 @@ public MemoryCache(IOptions optionsAccessor) /// The factory used to create loggers. public MemoryCache(IOptions optionsAccessor, ILoggerFactory loggerFactory) { - _options = optionsAccessor?.Value ?? throw new ArgumentNullException(nameof(optionsAccessor)); - _logger = loggerFactory?.CreateLogger() ?? throw new ArgumentNullException(nameof(loggerFactory)); + if (optionsAccessor == null) + { + throw new ArgumentNullException(nameof(optionsAccessor)); + } + + if (loggerFactory == null) + { + throw new ArgumentNullException(nameof(loggerFactory)); + } + + _options = optionsAccessor.Value; + _logger = loggerFactory.CreateLogger(); _entries = new ConcurrentDictionary(); _lastExpirationScan = _options.Clock?.UtcNow ?? DateTimeOffset.UtcNow; From 80eb6e00472841ede163b6b9b8fb1522bbe459c9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 17:17:28 +0100 Subject: [PATCH 12/13] Revert "AbsoluteExpirationRelativeToNow can internally just use AbsoluteExpiration and current time" because it was causing clock to get called twice This reverts commit f84b6cbde3b9a532a51740c05e9f20b97e8d37ff. # Conflicts: # src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs --- .../src/CacheEntry.cs | 21 ++++++------ .../src/MemoryCache.cs | 34 +++++++++++++------ 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 0f7f24b1156abf..aa81522288dedc 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -21,6 +21,7 @@ internal class CacheEntry : ICacheEntry private IList _expirationTokenRegistrations; private IList _postEvictionCallbacks; private IList _expirationTokens; + private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; private IDisposable _scope; @@ -44,21 +45,19 @@ internal CacheEntry(object key, MemoryCache memoryCache) /// public TimeSpan? AbsoluteExpirationRelativeToNow { - get => AbsoluteExpiration.HasValue ? AbsoluteExpiration.Value - _cache.GetUtcNow() : null; + get => _absoluteExpirationRelativeToNow; set { - if (value.HasValue) - { - if (value <= TimeSpan.Zero) - { - throw new ArgumentOutOfRangeException( - nameof(AbsoluteExpirationRelativeToNow), - value, - "The relative expiration value must be positive."); - } - AbsoluteExpiration = _cache.GetUtcNow() + value; + if (value <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(AbsoluteExpirationRelativeToNow), + value, + "The relative expiration value must be positive."); } + + _absoluteExpirationRelativeToNow = value; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index e0f8a93674c49f..e863e4d1a221db 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; @@ -56,7 +57,13 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory _logger = loggerFactory.CreateLogger(); _entries = new ConcurrentDictionary(); - _lastExpirationScan = _options.Clock?.UtcNow ?? DateTimeOffset.UtcNow; + + if (_options.Clock == null) + { + _options.Clock = new SystemClock(); + } + + _lastExpirationScan = _options.Clock.UtcNow; } /// @@ -83,8 +90,6 @@ public ICacheEntry CreateEntry(object key) return new CacheEntry(key, this); } - internal DateTimeOffset GetUtcNow() => _options.Clock?.UtcNow ?? DateTimeOffset.UtcNow; - internal void SetEntry(CacheEntry entry) { if (_disposed) @@ -98,7 +103,17 @@ internal void SetEntry(CacheEntry entry) throw new InvalidOperationException($"Cache entry must specify a value for {nameof(entry.Size)} when {nameof(_options.SizeLimit)} is set."); } - DateTimeOffset? absoluteExpiration = entry.AbsoluteExpiration; + DateTimeOffset utcNow = _options.Clock.UtcNow; + + DateTimeOffset? absoluteExpiration = null; + if (entry.AbsoluteExpirationRelativeToNow.HasValue) + { + absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; + } + else if (entry.AbsoluteExpiration.HasValue) + { + absoluteExpiration = entry.AbsoluteExpiration; + } // Applying the option's absolute expiration only if it's not already smaller. // This can be the case if a dependent cache entry has a smaller value, and @@ -111,7 +126,6 @@ internal void SetEntry(CacheEntry entry) } } - DateTimeOffset utcNow = GetUtcNow(); // Initialize the last access timestamp at the time the entry is added entry.LastAccessed = utcNow; @@ -207,7 +221,7 @@ public bool TryGetValue(object key, out object result) ValidateCacheKey(key); CheckDisposed(); - DateTimeOffset utcNow = GetUtcNow(); + DateTimeOffset utcNow = _options.Clock.UtcNow; if (_entries.TryGetValue(key, out CacheEntry entry)) { @@ -259,7 +273,7 @@ public void Remove(object key) entry.InvokeEvictionCallbacks(); } - StartScanForExpiredItemsIfNeeded(GetUtcNow()); + StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); } private void RemoveEntry(CacheEntry entry) @@ -278,7 +292,7 @@ internal void EntryExpired(CacheEntry entry) { // TODO: For efficiency consider processing these expirations in batches. RemoveEntry(entry); - StartScanForExpiredItemsIfNeeded(GetUtcNow()); + StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); } // Called by multiple actions to see how long it's been since we last checked for expired items. @@ -301,7 +315,7 @@ void ScheduleTask(DateTimeOffset utcNow) private static void ScanForExpiredItems(MemoryCache cache) { - DateTimeOffset now = cache._lastExpirationScan = cache.GetUtcNow(); + DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow; foreach (CacheEntry entry in cache._entries.Values) { if (entry.CheckExpired(now)) @@ -384,7 +398,7 @@ private void Compact(long removalSizeTarget, Func computeEntry long removedSize = 0; // Sort items by expired & priority status - DateTimeOffset now = GetUtcNow(); + DateTimeOffset now = _options.Clock.UtcNow; foreach (CacheEntry entry in _entries.Values) { if (entry.CheckExpired(now)) From baf73b3f4cbb6619ab540d5708254f80724cd2aa Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 1 Dec 2020 17:27:01 +0100 Subject: [PATCH 13/13] add comments --- .../Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index aa81522288dedc..47e3c5c8fd4177 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -48,6 +48,8 @@ public TimeSpan? 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) { @@ -264,6 +266,7 @@ private static void ExpirationTokensExpired(object obj) private void DetachTokens() { + // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock lock (_lock) { IList registrations = _expirationTokenRegistrations; @@ -313,6 +316,7 @@ private static void InvokeCallbacks(CacheEntry entry) } } + // 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)