From 42fee8fcb3105436661e4fdf2fc827a7ae68edff Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 14 Sep 2021 21:44:23 +0300 Subject: [PATCH 01/14] Pack CacheEntry.Size --- .../src/CacheEntry.cs | 17 ++++++++++------- .../src/MemoryCache.cs | 14 +++++++------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index e9974c542c2259..055b0e6e5eb019 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -19,7 +19,7 @@ internal sealed partial class CacheEntry : ICacheEntry 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 long _size = -1; private CacheEntry _previous; // this field is not null only before the entry is added to the cache and tracking is enabled private object _value; private CacheEntryState _state; @@ -97,12 +97,11 @@ public TimeSpan? SlidingExpiration /// public CacheItemPriority Priority { get => _state.Priority; set => _state.Priority = value; } - /// - /// Gets or sets the size of the cache entry value. - /// - public long? Size + internal long Size => _size; + + long? ICacheEntry.Size { - get => _size; + get => _size < 0 ? null : _size; set { if (value < 0) @@ -110,7 +109,11 @@ public long? Size throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); } - _size = value; + // disallow entry size changes after it has been committed + if (_state.IsDisposed) + return; + + _size = value ?? -1; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 7c426014c4f5f3..be5c4e78ceb604 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -101,7 +101,7 @@ internal void SetEntry(CacheEntry entry) return; } - if (_options.SizeLimit.HasValue && !entry.Size.HasValue) + if (_options.SizeLimit.HasValue && entry.Size < 0) { throw new InvalidOperationException(SR.Format(SR.CacheEntryHasEmptySize, nameof(entry.Size), nameof(_options.SizeLimit))); } @@ -159,7 +159,7 @@ internal void SetEntry(CacheEntry entry) if (_options.SizeLimit.HasValue) { // The prior entry was removed, decrease the by the prior entry's size - Interlocked.Add(ref _cacheSize, -priorEntry.Size.Value); + Interlocked.Add(ref _cacheSize, -priorEntry.Size); } } else @@ -180,7 +180,7 @@ internal void SetEntry(CacheEntry entry) if (_options.SizeLimit.HasValue) { // Entry could not be added, reset cache size - Interlocked.Add(ref _cacheSize, -entry.Size.Value); + Interlocked.Add(ref _cacheSize, -entry.Size); } entry.SetExpired(EvictionReason.Replaced); entry.InvokeEvictionCallbacks(); @@ -256,7 +256,7 @@ public void Remove(object key) { if (_options.SizeLimit.HasValue) { - Interlocked.Add(ref _cacheSize, -entry.Size.Value); + Interlocked.Add(ref _cacheSize, -entry.Size); } entry.SetExpired(EvictionReason.Removed); @@ -272,7 +272,7 @@ private void RemoveEntry(CacheEntry entry) { if (_options.SizeLimit.HasValue) { - Interlocked.Add(ref _cacheSize, -entry.Size.Value); + Interlocked.Add(ref _cacheSize, -entry.Size); } entry.InvokeEvictionCallbacks(); } @@ -329,7 +329,7 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry) for (int i = 0; i < 100; i++) { long sizeRead = Interlocked.Read(ref _cacheSize); - newSize = sizeRead + entry.Size.Value; + newSize = sizeRead + entry.Size; if (newSize < 0 || newSize > _options.SizeLimit) { @@ -363,7 +363,7 @@ private static void OvercapacityCompaction(MemoryCache cache) double? lowWatermark = cache._options.SizeLimit * (1 - cache._options.CompactionPercentage); if (currentSize > lowWatermark) { - cache.Compact(currentSize - (long)lowWatermark, entry => entry.Size.Value); + cache.Compact(currentSize - (long)lowWatermark, entry => entry.Size); } cache._logger.LogDebug($"Overcapacity compaction executed. New size {Interlocked.Read(ref cache._cacheSize)}"); From f0cd5a6056e70e036592f0522d151cce41231153 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 14 Sep 2021 22:05:15 +0300 Subject: [PATCH 02/14] Pack AbsoluteExpirationRelativeToNow --- .../src/CacheEntry.cs | 15 +++++++-------- .../src/MemoryCache.cs | 4 ++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 055b0e6e5eb019..a7c5429be29c3f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -17,7 +17,7 @@ internal sealed partial class CacheEntry : ICacheEntry private readonly MemoryCache _cache; private CacheEntryTokens _tokens; // might be null if user is not using the tokens or callbacks - private TimeSpan? _absoluteExpirationRelativeToNow; + private TimeSpan _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long _size = -1; private CacheEntry _previous; // this field is not null only before the entry is added to the cache and tracking is enabled @@ -37,18 +37,17 @@ internal CacheEntry(object key, MemoryCache memoryCache) /// public DateTimeOffset? AbsoluteExpiration { get; set; } - /// - /// Gets or sets an absolute expiration time, relative to now. - /// - public TimeSpan? AbsoluteExpirationRelativeToNow + internal TimeSpan AbsoluteExpirationRelativeToNow => _absoluteExpirationRelativeToNow; + + TimeSpan? ICacheEntry.AbsoluteExpirationRelativeToNow { - get => _absoluteExpirationRelativeToNow; + get => _absoluteExpirationRelativeToNow.Ticks == 0 ? null : _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) + if (value is { Ticks: <= 0 }) { throw new ArgumentOutOfRangeException( nameof(AbsoluteExpirationRelativeToNow), @@ -56,7 +55,7 @@ public TimeSpan? AbsoluteExpirationRelativeToNow "The relative expiration value must be positive."); } - _absoluteExpirationRelativeToNow = value; + _absoluteExpirationRelativeToNow = value.GetValueOrDefault(); } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index be5c4e78ceb604..2887377cae18c0 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -111,9 +111,9 @@ internal void SetEntry(CacheEntry entry) // 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 // it was set by cascading it to its parent. - if (entry.AbsoluteExpirationRelativeToNow.HasValue) + if (entry.AbsoluteExpirationRelativeToNow.Ticks > 0) { - var absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow.Value; + var absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; if (!entry.AbsoluteExpiration.HasValue || absoluteExpiration < entry.AbsoluteExpiration.Value) { entry.AbsoluteExpiration = absoluteExpiration; From d4d723a7ee1f3cc72133503d3bbb0fb4d4df3e30 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 14 Sep 2021 22:11:03 +0300 Subject: [PATCH 03/14] Pack SlidingExpiration --- .../src/CacheEntry.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index a7c5429be29c3f..57d389d4cdd39b 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -18,7 +18,7 @@ internal sealed partial class CacheEntry : ICacheEntry private CacheEntryTokens _tokens; // might be null if user is not using the tokens or callbacks private TimeSpan _absoluteExpirationRelativeToNow; - private TimeSpan? _slidingExpiration; + private TimeSpan _slidingExpiration; private long _size = -1; private CacheEntry _previous; // this field is not null only before the entry is added to the cache and tracking is enabled private object _value; @@ -65,10 +65,10 @@ internal CacheEntry(object key, MemoryCache memoryCache) /// public TimeSpan? SlidingExpiration { - get => _slidingExpiration; + get => _slidingExpiration.Ticks == 0 ? null : _slidingExpiration; set { - if (value <= TimeSpan.Zero) + if (value is { Ticks: <= 0 }) { throw new ArgumentOutOfRangeException( nameof(SlidingExpiration), @@ -76,7 +76,7 @@ public TimeSpan? SlidingExpiration "The sliding expiration value must be positive."); } - _slidingExpiration = value; + _slidingExpiration = value.GetValueOrDefault(); } } @@ -179,7 +179,7 @@ internal void SetExpired(EvictionReason reason) [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling private bool CheckForExpiredTime(in DateTimeOffset now) { - if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue) + if (!AbsoluteExpiration.HasValue && _slidingExpiration.Ticks == 0) { return false; } @@ -194,7 +194,7 @@ bool FullCheck(in DateTimeOffset offset) return true; } - if (_slidingExpiration.HasValue + if (_slidingExpiration.Ticks > 0 && (offset - LastAccessed) >= _slidingExpiration) { SetExpired(EvictionReason.Expired); From f9f4517c70f4a7297891de53031bb7564f4fed33 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 14 Sep 2021 22:27:09 +0300 Subject: [PATCH 04/14] Cleanup CacheEntryTokens --- .../src/CacheEntry.CacheEntryTokens.cs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index b67106dd7e0d50..e97912e192cb19 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -29,9 +29,8 @@ internal void AttachTokens(CacheEntry cacheEntry) { lock (this) { - for (int i = 0; i < _expirationTokens.Count; i++) + foreach (IChangeToken expirationToken in _expirationTokens) { - IChangeToken expirationToken = _expirationTokens[i]; if (expirationToken.ActiveChangeCallbacks) { _expirationTokenRegistrations ??= new List(1); @@ -47,9 +46,8 @@ internal bool CheckForExpiredTokens(CacheEntry cacheEntry) { if (_expirationTokens != null) { - for (int i = 0; i < _expirationTokens.Count; i++) + foreach (IChangeToken expiredToken in _expirationTokens) { - IChangeToken expiredToken = _expirationTokens[i]; if (expiredToken.HasChanged) { cacheEntry.SetExpired(EvictionReason.TokenExpired); @@ -68,12 +66,10 @@ internal void PropagateTokens(CacheEntry parentEntry) { lock (this) { - lock (parentEntry.GetOrCreateTokens()) + CacheEntryTokens parentTokens = parentEntry.GetOrCreateTokens(); + lock (parentTokens) { - foreach (IChangeToken expirationToken in _expirationTokens) - { - parentEntry.AddExpirationToken(expirationToken); - } + parentTokens.ExpirationTokens.AddRange(_expirationTokens); } } } From 56bb50eabfe1db40375458ec2a4a19073012a904 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 14 Sep 2021 22:56:01 +0300 Subject: [PATCH 05/14] Pack LastAccessed and use DateTimes for expiration logic --- .../src/CacheEntry.cs | 16 +++--- .../src/MemoryCache.cs | 54 +++++++++---------- .../tests/CacheEntryScopeExpirationTests.cs | 12 ++--- 3 files changed, 39 insertions(+), 43 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 57d389d4cdd39b..f48b1ee14d284f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -128,7 +128,7 @@ public object Value } } - internal DateTimeOffset LastAccessed { get; set; } + internal DateTime LastAccessed { get; set; } internal EvictionReason EvictionReason { get => _state.EvictionReason; private set => _state.EvictionReason = value; } @@ -161,9 +161,9 @@ public void Dispose() } [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - internal bool CheckExpired(in DateTimeOffset now) + internal bool CheckExpired(DateTime utcNow) => _state.IsExpired - || CheckForExpiredTime(now) + || CheckForExpiredTime(utcNow) || (_tokens != null && _tokens.CheckForExpiredTokens(this)); internal void SetExpired(EvictionReason reason) @@ -177,25 +177,25 @@ internal void SetExpired(EvictionReason reason) } [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - private bool CheckForExpiredTime(in DateTimeOffset now) + private bool CheckForExpiredTime(DateTime utcNow) { if (!AbsoluteExpiration.HasValue && _slidingExpiration.Ticks == 0) { return false; } - return FullCheck(now); + return FullCheck(utcNow); - bool FullCheck(in DateTimeOffset offset) + bool FullCheck(DateTime utcNow) { - if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value <= offset) + if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value.UtcDateTime <= utcNow) { SetExpired(EvictionReason.Expired); return true; } if (_slidingExpiration.Ticks > 0 - && (offset - LastAccessed) >= _slidingExpiration) + && (utcNow - LastAccessed) >= _slidingExpiration) { SetExpired(EvictionReason.Expired); return true; diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 2887377cae18c0..d48800ad0c3f47 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; @@ -27,7 +26,7 @@ public class MemoryCache : IMemoryCache private long _cacheSize; private bool _disposed; - private DateTimeOffset _lastExpirationScan; + private DateTime _lastExpirationScan; /// /// Creates a new instance. @@ -58,15 +57,12 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory _entries = new ConcurrentDictionary(); - if (_options.Clock == null) - { - _options.Clock = new SystemClock(); - } - - _lastExpirationScan = _options.Clock.UtcNow; + _lastExpirationScan = UtcNow; TrackLinkedCacheEntries = _options.TrackLinkedCacheEntries; // we store the setting now so it's consistent for entire MemoryCache lifetime } + private DateTime UtcNow => _options.Clock?.UtcNow.UtcDateTime ?? DateTime.UtcNow; + /// /// Cleans up the background collection events. /// @@ -106,7 +102,7 @@ internal void SetEntry(CacheEntry entry) throw new InvalidOperationException(SR.Format(SR.CacheEntryHasEmptySize, nameof(entry.Size), nameof(_options.SizeLimit))); } - DateTimeOffset utcNow = _options.Clock.UtcNow; + DateTime utcNow = UtcNow; // 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 @@ -211,7 +207,7 @@ public bool TryGetValue(object key, out object result) ValidateCacheKey(key); CheckDisposed(); - DateTimeOffset utcNow = _options.Clock.UtcNow; + DateTime utcNow = UtcNow; if (_entries.TryGetValue(key, out CacheEntry entry)) { @@ -263,7 +259,7 @@ public void Remove(object key) entry.InvokeEvictionCallbacks(); } - StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); + StartScanForExpiredItemsIfNeeded(UtcNow); } private void RemoveEntry(CacheEntry entry) @@ -282,38 +278,38 @@ internal void EntryExpired(CacheEntry entry) { // TODO: For efficiency consider processing these expirations in batches. RemoveEntry(entry); - StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); + StartScanForExpiredItemsIfNeeded(UtcNow); } // Called by multiple actions to see how long it's been since we last checked for expired items. // If sufficient time has elapsed then a scan is initiated on a background task. [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void StartScanForExpiredItemsIfNeeded(DateTimeOffset utcNow) + private void StartScanForExpiredItemsIfNeeded(DateTime utcNow) { if (_options.ExpirationScanFrequency < utcNow - _lastExpirationScan) { ScheduleTask(utcNow); } - void ScheduleTask(DateTimeOffset utcNow) + void ScheduleTask(DateTime utcNow) { _lastExpirationScan = utcNow; - Task.Factory.StartNew(state => ScanForExpiredItems((MemoryCache)state), this, + Task.Factory.StartNew(state => ((MemoryCache)state).ScanForExpiredItems(), this, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); } } - private static void ScanForExpiredItems(MemoryCache cache) + private void ScanForExpiredItems() { - DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow; + DateTime utcNow = _lastExpirationScan = UtcNow; - foreach (KeyValuePair item in cache._entries) + foreach (KeyValuePair item in _entries) { CacheEntry entry = item.Value; - if (entry.CheckExpired(now)) + if (entry.CheckExpired(utcNow)) { - cache.RemoveEntry(entry); + RemoveEntry(entry); } } } @@ -351,22 +347,22 @@ private void TriggerOvercapacityCompaction() _logger.LogDebug("Overcapacity compaction triggered"); // Spawn background thread for compaction - ThreadPool.QueueUserWorkItem(s => OvercapacityCompaction((MemoryCache)s), this); + ThreadPool.QueueUserWorkItem(s => ((MemoryCache)s).OvercapacityCompaction(), this); } - private static void OvercapacityCompaction(MemoryCache cache) + private void OvercapacityCompaction() { - long currentSize = Interlocked.Read(ref cache._cacheSize); + long currentSize = Interlocked.Read(ref _cacheSize); - cache._logger.LogDebug($"Overcapacity compaction executing. Current size {currentSize}"); + _logger.LogDebug($"Overcapacity compaction executing. Current size {currentSize}"); - double? lowWatermark = cache._options.SizeLimit * (1 - cache._options.CompactionPercentage); + double? lowWatermark = _options.SizeLimit * (1 - _options.CompactionPercentage); if (currentSize > lowWatermark) { - cache.Compact(currentSize - (long)lowWatermark, entry => entry.Size); + Compact(currentSize - (long)lowWatermark, entry => entry.Size); } - cache._logger.LogDebug($"Overcapacity compaction executed. New size {Interlocked.Read(ref cache._cacheSize)}"); + _logger.LogDebug($"Overcapacity compaction executed. New size {Interlocked.Read(ref _cacheSize)}"); } /// Remove at least the given percentage (0.10 for 10%) of the total entries (or estimated memory?), according to the following policy: @@ -391,11 +387,11 @@ private void Compact(long removalSizeTarget, Func computeEntry long removedSize = 0; // Sort items by expired & priority status - DateTimeOffset now = _options.Clock.UtcNow; + DateTime utcNow = UtcNow; foreach (KeyValuePair item in _entries) { CacheEntry entry = item.Value; - if (entry.CheckExpired(now)) + if (entry.CheckExpired(utcNow)) { entriesToRemove.Add(entry); removedSize += computeEntrySize(entry); diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index c8f242e9b46bad..a00f54ce94a0ce 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -519,22 +519,22 @@ await Task.WhenAll( Task.Run(() => SetExpiredManyTimes(entry)), Task.Run(() => SetExpiredManyTimes(entry))); - Assert.True(entry.CheckExpired(DateTimeOffset.UtcNow)); + Assert.True(entry.CheckExpired(DateTime.UtcNow)); static void SetExpiredManyTimes(CacheEntry cacheEntry) { - var now = DateTimeOffset.UtcNow; + var utcNow = DateTime.UtcNow; for (int i = 0; i < 1_000; i++) { cacheEntry.SetExpired(EvictionReason.Expired); // modifies CacheEntry._state - Assert.True(cacheEntry.CheckExpired(now)); + Assert.True(cacheEntry.CheckExpired(utcNow)); cacheEntry.Value = cacheEntry; // modifies CacheEntry._state - Assert.True(cacheEntry.CheckExpired(now)); + Assert.True(cacheEntry.CheckExpired(utcNow)); cacheEntry.SetExpired(EvictionReason.Expired); // modifies CacheEntry._state - Assert.True(cacheEntry.CheckExpired(now)); + Assert.True(cacheEntry.CheckExpired(utcNow)); cacheEntry.Dispose(); // might modify CacheEntry._state - Assert.True(cacheEntry.CheckExpired(now)); + Assert.True(cacheEntry.CheckExpired(utcNow)); } } } From 49fcbeb7875392943e6b47a83c857dffa729f65f Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 14 Sep 2021 23:57:54 +0300 Subject: [PATCH 06/14] Pack AbsoluteExpiration --- .../src/CacheEntry.cs | 56 +++++++++++++++---- .../src/MemoryCache.cs | 4 +- .../tests/CacheEntryScopeExpirationTests.cs | 32 +++++------ 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index f48b1ee14d284f..be6925cb787d0c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -23,6 +24,8 @@ internal sealed partial class CacheEntry : ICacheEntry private CacheEntry _previous; // this field is not null only before the entry is added to the cache and tracking is enabled private object _value; private CacheEntryState _state; + private short _absoluteExpirationOffsetMinutes; + private DateTime _absoluteExpiration; internal CacheEntry(object key, MemoryCache memoryCache) { @@ -32,10 +35,41 @@ internal CacheEntry(object key, MemoryCache memoryCache) _state = new CacheEntryState(CacheItemPriority.Normal); } - /// - /// Gets or sets an absolute expiration date for the cache entry. - /// - public DateTimeOffset? AbsoluteExpiration { get; set; } + internal bool HasAbsoluteExpiration => Unsafe.As(ref _absoluteExpiration) != 0; + + internal DateTime AbsoluteExpiration => _absoluteExpiration; + + internal void SetAbsoluteExpirationUtc(DateTime value) + { + Debug.Assert(value.Kind == DateTimeKind.Utc); + _absoluteExpiration = value; + _absoluteExpirationOffsetMinutes = 0; + } + + DateTimeOffset? ICacheEntry.AbsoluteExpiration + { + get + { + if (!HasAbsoluteExpiration) + return null; + + var offset = new TimeSpan(_absoluteExpirationOffsetMinutes * TimeSpan.TicksPerMinute); + return new DateTimeOffset(_absoluteExpiration.Ticks + offset.Ticks, offset); + } + set + { + if (value is null) + { + _absoluteExpiration = default; + _absoluteExpirationOffsetMinutes = default; + } + else + { + _absoluteExpiration = value.GetValueOrDefault().UtcDateTime; + _absoluteExpirationOffsetMinutes = (short)(value.GetValueOrDefault().Offset.Ticks / TimeSpan.TicksPerMinute); + } + } + } internal TimeSpan AbsoluteExpirationRelativeToNow => _absoluteExpirationRelativeToNow; @@ -179,7 +213,7 @@ internal void SetExpired(EvictionReason reason) [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling private bool CheckForExpiredTime(DateTime utcNow) { - if (!AbsoluteExpiration.HasValue && _slidingExpiration.Ticks == 0) + if (!HasAbsoluteExpiration && _slidingExpiration.Ticks == 0) { return false; } @@ -188,7 +222,7 @@ private bool CheckForExpiredTime(DateTime utcNow) bool FullCheck(DateTime utcNow) { - if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value.UtcDateTime <= utcNow) + if (HasAbsoluteExpiration && _absoluteExpiration <= utcNow) { SetExpired(EvictionReason.Expired); return true; @@ -222,7 +256,7 @@ private static void ExpirationTokensExpired(object obj) // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanPropagateTokens()) || AbsoluteExpiration.HasValue; + internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanPropagateTokens()) || HasAbsoluteExpiration; internal void PropagateOptions(CacheEntry parent) { @@ -235,12 +269,10 @@ internal void PropagateOptions(CacheEntry parent) // We do this regardless of it gets cached because the tokens are associated with the value we'll return. _tokens?.PropagateTokens(parent); - if (AbsoluteExpiration.HasValue) + if (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || _absoluteExpiration < parent._absoluteExpiration)) { - if (!parent.AbsoluteExpiration.HasValue || AbsoluteExpiration < parent.AbsoluteExpiration) - { - parent.AbsoluteExpiration = AbsoluteExpiration; - } + parent._absoluteExpiration = _absoluteExpiration; + parent._absoluteExpirationOffsetMinutes = _absoluteExpirationOffsetMinutes; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index d48800ad0c3f47..f3d9cad5264038 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -110,9 +110,9 @@ internal void SetEntry(CacheEntry entry) if (entry.AbsoluteExpirationRelativeToNow.Ticks > 0) { var absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; - if (!entry.AbsoluteExpiration.HasValue || absoluteExpiration < entry.AbsoluteExpiration.Value) + if (!entry.HasAbsoluteExpiration || absoluteExpiration < entry.AbsoluteExpiration) { - entry.AbsoluteExpiration = absoluteExpiration; + entry.SetAbsoluteExpirationUtc(absoluteExpiration); } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index a00f54ce94a0ce..b932372bf3a3c7 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -44,8 +44,8 @@ public void SetPopulates_ExpirationTokens_IntoScopedLink(bool trackLinkedCacheEn cache.Set(key, obj, new MemoryCacheEntryOptions().AddExpirationToken(expirationToken)); } - Assert.Equal(trackLinkedCacheEntries ? 1 : 0, ((CacheEntry)entry).ExpirationTokens.Count); - Assert.Null(((CacheEntry)entry).AbsoluteExpiration); + Assert.Equal(trackLinkedCacheEntries ? 1 : 0, entry.ExpirationTokens.Count); + Assert.Null(entry.AbsoluteExpiration); } [Theory] @@ -67,8 +67,8 @@ public void SetPopulates_AbsoluteExpiration_IntoScopeLink(bool trackLinkedCacheE cache.Set(key, obj, new MemoryCacheEntryOptions().SetAbsoluteExpiration(time)); } - Assert.Empty(((CacheEntry)entry).ExpirationTokens); - Assert.Equal(trackLinkedCacheEntries ? time : null, ((CacheEntry)entry).AbsoluteExpiration); + Assert.Empty(entry.ExpirationTokens); + Assert.Equal(trackLinkedCacheEntries ? time : null, entry.AbsoluteExpiration); } [Theory] @@ -353,8 +353,8 @@ public void GetWithImplicitLinkPopulatesExpirationTokens(bool trackLinkedCacheEn Assert.Null(CacheEntryHelper.Current); - Assert.Equal(trackLinkedCacheEntries ? 1 : 0, ((CacheEntry)entry).ExpirationTokens.Count); - Assert.Null(((CacheEntry)entry).AbsoluteExpiration); + Assert.Equal(trackLinkedCacheEntries ? 1 : 0, entry.ExpirationTokens.Count); + Assert.Null(entry.AbsoluteExpiration); } [Theory] @@ -389,10 +389,10 @@ public void LinkContextsCanNest(bool trackLinkedCacheEntries) Assert.Null(CacheEntryHelper.Current); - Assert.Single(((CacheEntry)entry1).ExpirationTokens); - Assert.Null(((CacheEntry)entry1).AbsoluteExpiration); - Assert.Equal(trackLinkedCacheEntries ? 1 : 0, ((CacheEntry)entry).ExpirationTokens.Count); - Assert.Null(((CacheEntry)entry).AbsoluteExpiration); + Assert.Single(entry1.ExpirationTokens); + Assert.Null(entry1.AbsoluteExpiration); + Assert.Equal(trackLinkedCacheEntries ? 1 : 0, entry.ExpirationTokens.Count); + Assert.Null(entry.AbsoluteExpiration); } [Theory] @@ -428,13 +428,13 @@ public void NestedLinkContextsCanAggregate(bool trackLinkedCacheEntries) } } - Assert.Equal(trackLinkedCacheEntries ? 2 : 1, ((CacheEntry)entry1).ExpirationTokens.Count()); - Assert.NotNull(((CacheEntry)entry1).AbsoluteExpiration); - Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(10), ((CacheEntry)entry1).AbsoluteExpiration); + Assert.Equal(trackLinkedCacheEntries ? 2 : 1, entry1.ExpirationTokens.Count()); + Assert.NotNull(entry1.AbsoluteExpiration); + Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(10), entry1.AbsoluteExpiration); - Assert.Single(((CacheEntry)entry2).ExpirationTokens); - Assert.NotNull(((CacheEntry)entry2).AbsoluteExpiration); - Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(15), ((CacheEntry)entry2).AbsoluteExpiration); + Assert.Single(entry2.ExpirationTokens); + Assert.NotNull(entry2.AbsoluteExpiration); + Assert.Equal(clock.UtcNow + TimeSpan.FromSeconds(15), entry2.AbsoluteExpiration); } [Fact] From 1bce2be119e95c9bb6191ea62d1ceb72f5f0e758 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Wed, 15 Sep 2021 00:12:46 +0300 Subject: [PATCH 07/14] Remove CacheEntryState --- .../src/CacheEntry.CacheEntryState.cs | 61 ------------------- .../src/CacheEntry.cs | 28 +++++---- 2 files changed, 16 insertions(+), 73 deletions(-) delete mode 100644 src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs deleted file mode 100644 index a3774fef395de1..00000000000000 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ /dev/null @@ -1,61 +0,0 @@ -// 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 sealed 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.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index be6925cb787d0c..3ceee7dbbbd78c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -23,16 +23,20 @@ internal sealed partial class CacheEntry : ICacheEntry private long _size = -1; private CacheEntry _previous; // this field is not null only before the entry is added to the cache and tracking is enabled private object _value; - private CacheEntryState _state; - private short _absoluteExpirationOffsetMinutes; private DateTime _absoluteExpiration; + private short _absoluteExpirationOffsetMinutes; + private bool _isDisposed; + private bool _isExpired; + private bool _isValueSet; + private byte _evictionReason; + private byte _priority; internal CacheEntry(object key, MemoryCache memoryCache) { Key = key ?? throw new ArgumentNullException(nameof(key)); _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); _previous = memoryCache.TrackLinkedCacheEntries ? CacheEntryHelper.EnterScope(this) : null; - _state = new CacheEntryState(CacheItemPriority.Normal); + _priority = (byte)CacheItemPriority.Normal; } internal bool HasAbsoluteExpiration => Unsafe.As(ref _absoluteExpiration) != 0; @@ -128,7 +132,7 @@ public TimeSpan? SlidingExpiration /// 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 => _state.Priority; set => _state.Priority = value; } + public CacheItemPriority Priority { get => (CacheItemPriority)_priority; set => _priority = (byte)value; } internal long Size => _size; @@ -143,7 +147,7 @@ public TimeSpan? SlidingExpiration } // disallow entry size changes after it has been committed - if (_state.IsDisposed) + if (_isDisposed) return; _size = value ?? -1; @@ -158,19 +162,19 @@ public object Value set { _value = value; - _state.IsValueSet = true; + _isValueSet = true; } } internal DateTime LastAccessed { get; set; } - internal EvictionReason EvictionReason { get => _state.EvictionReason; private set => _state.EvictionReason = value; } + internal EvictionReason EvictionReason { get => (EvictionReason)_evictionReason; private set => _evictionReason = (byte)value; } public void Dispose() { - if (!_state.IsDisposed) + if (!_isDisposed) { - _state.IsDisposed = true; + _isDisposed = true; if (_cache.TrackLinkedCacheEntries) { @@ -180,7 +184,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 (_state.IsValueSet) + if (_isValueSet) { _cache.SetEntry(this); @@ -196,7 +200,7 @@ public void Dispose() [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling internal bool CheckExpired(DateTime utcNow) - => _state.IsExpired + => _isExpired || CheckForExpiredTime(utcNow) || (_tokens != null && _tokens.CheckForExpiredTokens(this)); @@ -206,7 +210,7 @@ internal void SetExpired(EvictionReason reason) { EvictionReason = reason; } - _state.IsExpired = true; + _isExpired = true; _tokens?.DetachTokens(); } From 9e31d49f4d0bbb2e1156cfb2b5e9b2ae9224660f Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Wed, 15 Sep 2021 00:53:47 +0300 Subject: [PATCH 08/14] Address PR feedback --- .../src/CacheEntry.CacheEntryTokens.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index e97912e192cb19..476aaa97a27f63 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -25,12 +25,13 @@ private sealed class CacheEntryTokens internal void AttachTokens(CacheEntry cacheEntry) { - if (_expirationTokens != null) + if (_expirationTokens is { } expirationTokens) { lock (this) { - foreach (IChangeToken expirationToken in _expirationTokens) + for (int i = 0; i < expirationTokens.Count; i++) { + IChangeToken expirationToken = expirationTokens[i]; if (expirationToken.ActiveChangeCallbacks) { _expirationTokenRegistrations ??= new List(1); @@ -44,10 +45,11 @@ internal void AttachTokens(CacheEntry cacheEntry) internal bool CheckForExpiredTokens(CacheEntry cacheEntry) { - if (_expirationTokens != null) + if (_expirationTokens is { } expirationTokens) { - foreach (IChangeToken expiredToken in _expirationTokens) + for (int i = 0; i < expirationTokens.Count; i++) { + IChangeToken expiredToken = expirationTokens[i]; if (expiredToken.HasChanged) { cacheEntry.SetExpired(EvictionReason.TokenExpired); From 9ad0a117cfe0801f96fc84dc514941648aae4218 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Fri, 1 Oct 2021 15:49:51 +0300 Subject: [PATCH 09/14] Rebase cleanup --- .../src/MemoryCache.cs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index f3d9cad5264038..42f61189eca64a 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -131,15 +131,10 @@ internal void SetEntry(CacheEntry entry) { RemoveEntry(priorEntry); } - StartScanForExpiredItemsIfNeeded(utcNow); - return; } - - bool exceedsCapacity = UpdateCacheSizeExceedsCapacity(entry); - if (!exceedsCapacity) + else if (!UpdateCacheSizeExceedsCapacity(entry)) { - bool entryAdded = false; - + bool entryAdded; if (priorEntry == null) { // Try to add the new entry if no previous entries exist. @@ -182,10 +177,7 @@ internal void SetEntry(CacheEntry entry) entry.InvokeEvictionCallbacks(); } - if (priorEntry != null) - { - priorEntry.InvokeEvictionCallbacks(); - } + priorEntry?.InvokeEvictionCallbacks(); } else { From 10b1015fee84e99b1d2c7567745b02264be26cb3 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Wed, 6 Oct 2021 15:15:50 +0300 Subject: [PATCH 10/14] Address PR feedback --- .../src/CacheEntry.CacheEntryTokens.cs | 6 ++++-- .../src/MemoryCache.cs | 12 +++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index 476aaa97a27f63..4a3a60ea4e31fa 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -25,7 +25,8 @@ private sealed class CacheEntryTokens internal void AttachTokens(CacheEntry cacheEntry) { - if (_expirationTokens is { } expirationTokens) + var expirationTokens = _expirationTokens; + if (expirationTokens is not null) { lock (this) { @@ -45,7 +46,8 @@ internal void AttachTokens(CacheEntry cacheEntry) internal bool CheckForExpiredTokens(CacheEntry cacheEntry) { - if (_expirationTokens is { } expirationTokens) + var expirationTokens = _expirationTokens; + if (expirationTokens is not null) { for (int i = 0; i < expirationTokens.Count; i++) { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 42f61189eca64a..9de8d293d0d380 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -313,11 +313,10 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry) return false; } - long newSize = 0L; for (int i = 0; i < 100; i++) { long sizeRead = Interlocked.Read(ref _cacheSize); - newSize = sizeRead + entry.Size; + long newSize = sizeRead + entry.Size; if (newSize < 0 || newSize > _options.SizeLimit) { @@ -336,7 +335,8 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry) private void TriggerOvercapacityCompaction() { - _logger.LogDebug("Overcapacity compaction triggered"); + if (_logger.IsEnabled(LogLevel.Debug)) + _logger.LogDebug("Overcapacity compaction triggered"); // Spawn background thread for compaction ThreadPool.QueueUserWorkItem(s => ((MemoryCache)s).OvercapacityCompaction(), this); @@ -346,7 +346,8 @@ private void OvercapacityCompaction() { long currentSize = Interlocked.Read(ref _cacheSize); - _logger.LogDebug($"Overcapacity compaction executing. Current size {currentSize}"); + if (_logger.IsEnabled(LogLevel.Debug)) + _logger.LogDebug($"Overcapacity compaction executing. Current size {currentSize}"); double? lowWatermark = _options.SizeLimit * (1 - _options.CompactionPercentage); if (currentSize > lowWatermark) @@ -354,7 +355,8 @@ private void OvercapacityCompaction() Compact(currentSize - (long)lowWatermark, entry => entry.Size); } - _logger.LogDebug($"Overcapacity compaction executed. New size {Interlocked.Read(ref _cacheSize)}"); + if (_logger.IsEnabled(LogLevel.Debug)) + _logger.LogDebug($"Overcapacity compaction executed. New size {Interlocked.Read(ref _cacheSize)}"); } /// Remove at least the given percentage (0.10 for 10%) of the total entries (or estimated memory?), according to the following policy: From b4a1843887e2be97a904352d46a4c1e86e0debc9 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Wed, 20 Oct 2021 21:53:22 +0300 Subject: [PATCH 11/14] More optimizations --- .../src/CacheEntry.cs | 51 ++++++++++++------- .../src/CacheEntryHelper.cs | 32 ------------ .../src/MemoryCache.cs | 44 +++++++++------- .../src/MemoryCacheOptions.cs | 10 ++-- .../src/MemoryDistributedCache.cs | 2 +- .../tests/CacheEntryScopeExpirationTests.cs | 12 ++--- .../tests/MemoryCacheSetAndRemoveTests.cs | 4 +- 7 files changed, 76 insertions(+), 79 deletions(-) delete mode 100644 src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 3ceee7dbbbd78c..fbdd2758631290 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -14,6 +14,7 @@ namespace Microsoft.Extensions.Caching.Memory internal sealed partial class CacheEntry : ICacheEntry { private static readonly Action ExpirationCallback = ExpirationTokensExpired; + private static readonly AsyncLocal _current = new AsyncLocal(); private readonly MemoryCache _cache; @@ -29,17 +30,26 @@ internal sealed partial class CacheEntry : ICacheEntry private bool _isExpired; private bool _isValueSet; private byte _evictionReason; - private byte _priority; + private byte _priority = (byte)CacheItemPriority.Normal; internal CacheEntry(object key, MemoryCache memoryCache) { Key = key ?? throw new ArgumentNullException(nameof(key)); _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); - _previous = memoryCache.TrackLinkedCacheEntries ? CacheEntryHelper.EnterScope(this) : null; - _priority = (byte)CacheItemPriority.Normal; + if (memoryCache.TrackLinkedCacheEntries) + { + var holder = _current; + _previous = holder.Value; + holder.Value = this; + } } - internal bool HasAbsoluteExpiration => Unsafe.As(ref _absoluteExpiration) != 0; + // internal for testing + internal static CacheEntry Current => _current.Value; + + private ref ulong RawAbsoluteExpiration => ref Unsafe.As(ref _absoluteExpiration); + + internal bool HasAbsoluteExpiration => RawAbsoluteExpiration != 0; internal DateTime AbsoluteExpiration => _absoluteExpiration; @@ -69,8 +79,9 @@ internal void SetAbsoluteExpirationUtc(DateTime value) } else { - _absoluteExpiration = value.GetValueOrDefault().UtcDateTime; - _absoluteExpirationOffsetMinutes = (short)(value.GetValueOrDefault().Offset.Ticks / TimeSpan.TicksPerMinute); + DateTimeOffset expiration = value.GetValueOrDefault(); + _absoluteExpiration = expiration.UtcDateTime; + _absoluteExpirationOffsetMinutes = (short)(expiration.Offset.Ticks / TimeSpan.TicksPerMinute); } } } @@ -154,7 +165,7 @@ public TimeSpan? SlidingExpiration } } - public object Key { get; private set; } + public object Key { get; } public object Value { @@ -178,7 +189,8 @@ public void Dispose() if (_cache.TrackLinkedCacheEntries) { - CacheEntryHelper.ExitScope(this, _previous); + Debug.Assert(_current.Value == this, "Entries disposed in invalid order"); + _current.Value = _previous; } // Don't commit or propagate options if the CacheEntry Value was never set. @@ -188,9 +200,15 @@ public void Dispose() { _cache.SetEntry(this); - if (_previous != null && CanPropagateOptions()) + CacheEntry parent = _previous; + if (parent != null) { - PropagateOptions(_previous); + if (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || RawAbsoluteExpiration < parent.RawAbsoluteExpiration)) + { + parent._absoluteExpiration = _absoluteExpiration; + parent._absoluteExpirationOffsetMinutes = _absoluteExpirationOffsetMinutes; + } + _tokens?.PropagateTokens(parent); } } @@ -258,12 +276,11 @@ private static void ExpirationTokensExpired(object obj) internal void InvokeEvictionCallbacks() => _tokens?.InvokeEvictionCallbacks(this); - // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) - [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanPropagateTokens()) || HasAbsoluteExpiration; + internal bool CanPropagateTokens() => _tokens != null && _tokens.CanPropagateTokens(); - internal void PropagateOptions(CacheEntry parent) + internal void PropagateOptionsToCurrent() { + CacheEntry parent = _current.Value; if (parent == null) { return; @@ -271,13 +288,13 @@ internal void PropagateOptions(CacheEntry parent) // 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. - _tokens?.PropagateTokens(parent); - - if (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || _absoluteExpiration < parent._absoluteExpiration)) + if (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || RawAbsoluteExpiration < parent.RawAbsoluteExpiration)) { parent._absoluteExpiration = _absoluteExpiration; parent._absoluteExpirationOffsetMinutes = _absoluteExpirationOffsetMinutes; } + + _tokens?.PropagateTokens(parent); } private CacheEntryTokens GetOrCreateTokens() diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs deleted file mode 100644 index e06ee0fb0100ea..00000000000000 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs +++ /dev/null @@ -1,32 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Diagnostics; -using System.Threading; - -namespace Microsoft.Extensions.Caching.Memory -{ - internal static class CacheEntryHelper - { - private static readonly AsyncLocal _current = new AsyncLocal(); - - internal static CacheEntry Current - { - get => _current.Value; - private set => _current.Value = value; - } - - internal static CacheEntry EnterScope(CacheEntry current) - { - CacheEntry previous = Current; - Current = current; - return previous; - } - - internal static void ExitScope(CacheEntry current, CacheEntry previous) - { - Debug.Assert(Current == current, "Entries disposed in invalid order"); - Current = previous; - } - } -} diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 9de8d293d0d380..295632f534de02 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -74,7 +74,7 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory public int Count => _entries.Count; // internal for testing - internal long Size { get => Interlocked.Read(ref _cacheSize); } + internal long Size => Volatile.Read(ref _cacheSize); internal bool TrackLinkedCacheEntries { get; } @@ -97,7 +97,7 @@ internal void SetEntry(CacheEntry entry) return; } - if (_options.SizeLimit.HasValue && entry.Size < 0) + if (_options.HasSizeLimit && entry.Size < 0) { throw new InvalidOperationException(SR.Format(SR.CacheEntryHasEmptySize, nameof(entry.Size), nameof(_options.SizeLimit))); } @@ -147,7 +147,7 @@ internal void SetEntry(CacheEntry entry) if (entryAdded) { - if (_options.SizeLimit.HasValue) + if (_options.HasSizeLimit) { // The prior entry was removed, decrease the by the prior entry's size Interlocked.Add(ref _cacheSize, -priorEntry.Size); @@ -168,7 +168,7 @@ internal void SetEntry(CacheEntry entry) } else { - if (_options.SizeLimit.HasValue) + if (_options.HasSizeLimit) { // Entry could not be added, reset cache size Interlocked.Add(ref _cacheSize, -entry.Size); @@ -201,8 +201,9 @@ public bool TryGetValue(object key, out object result) DateTime utcNow = UtcNow; - if (_entries.TryGetValue(key, out CacheEntry entry)) + if (_entries.TryGetValue(key, out CacheEntry tmp)) { + CacheEntry entry = tmp; // Check if expired due to expiration tokens, timers, etc. and if so, remove it. // Allow a stale Replaced value to be returned due to concurrent calls to SetExpired during SetEntry. if (!entry.CheckExpired(utcNow) || entry.EvictionReason == EvictionReason.Replaced) @@ -210,11 +211,11 @@ public bool TryGetValue(object key, out object result) entry.LastAccessed = utcNow; result = entry.Value; - if (TrackLinkedCacheEntries && entry.CanPropagateOptions()) + if (TrackLinkedCacheEntries && (entry.CanPropagateTokens() || entry.HasAbsoluteExpiration)) { // 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); + entry.PropagateOptionsToCurrent(); } StartScanForExpiredItemsIfNeeded(utcNow); @@ -242,7 +243,7 @@ public void Remove(object key) CheckDisposed(); if (_entries.TryRemove(key, out CacheEntry entry)) { - if (_options.SizeLimit.HasValue) + if (_options.HasSizeLimit) { Interlocked.Add(ref _cacheSize, -entry.Size); } @@ -258,7 +259,7 @@ private void RemoveEntry(CacheEntry entry) { if (EntriesCollection.Remove(new KeyValuePair(entry.Key, entry))) { - if (_options.SizeLimit.HasValue) + if (_options.HasSizeLimit) { Interlocked.Add(ref _cacheSize, -entry.Size); } @@ -308,26 +309,29 @@ private void ScanForExpiredItems() private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry) { - if (!_options.SizeLimit.HasValue) + long sizeLimit = _options.SizeLimitValue; + if (sizeLimit < 0) { return false; } + long sizeRead = Volatile.Read(ref _cacheSize); for (int i = 0; i < 100; i++) { - long sizeRead = Interlocked.Read(ref _cacheSize); long newSize = sizeRead + entry.Size; - if (newSize < 0 || newSize > _options.SizeLimit) + if ((ulong)newSize > (ulong)sizeLimit) { // Overflow occurred, return true without updating the cache size return true; } - if (sizeRead == Interlocked.CompareExchange(ref _cacheSize, newSize, sizeRead)) + long original = Interlocked.CompareExchange(ref _cacheSize, newSize, sizeRead); + if (sizeRead == original) { return false; } + sizeRead = original; } return true; @@ -344,19 +348,23 @@ private void TriggerOvercapacityCompaction() private void OvercapacityCompaction() { - long currentSize = Interlocked.Read(ref _cacheSize); + long currentSize = Volatile.Read(ref _cacheSize); if (_logger.IsEnabled(LogLevel.Debug)) _logger.LogDebug($"Overcapacity compaction executing. Current size {currentSize}"); - double? lowWatermark = _options.SizeLimit * (1 - _options.CompactionPercentage); - if (currentSize > lowWatermark) + long sizeLimit = _options.SizeLimitValue; + if (sizeLimit >= 0) { - Compact(currentSize - (long)lowWatermark, entry => entry.Size); + long lowWatermark = sizeLimit - (long)(sizeLimit * _options.CompactionPercentage); + if (currentSize > lowWatermark) + { + Compact(currentSize - lowWatermark, entry => entry.Size); + } } if (_logger.IsEnabled(LogLevel.Debug)) - _logger.LogDebug($"Overcapacity compaction executed. New size {Interlocked.Read(ref _cacheSize)}"); + _logger.LogDebug($"Overcapacity compaction executed. New size {Volatile.Read(ref _cacheSize)}"); } /// Remove at least the given percentage (0.10 for 10%) of the total entries (or estimated memory?), according to the following policy: diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs index 9fd9dfaed4d0a9..48cae01a5a2ab1 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs @@ -9,7 +9,7 @@ namespace Microsoft.Extensions.Caching.Memory { public class MemoryCacheOptions : IOptions { - private long? _sizeLimit; + private long _sizeLimit = -1; private double _compactionPercentage = 0.05; public ISystemClock Clock { get; set; } @@ -19,12 +19,16 @@ public class MemoryCacheOptions : IOptions /// public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1); + internal bool HasSizeLimit => _sizeLimit >= 0; + + internal long SizeLimitValue => _sizeLimit; + /// /// Gets or sets the maximum size of the cache. /// public long? SizeLimit { - get => _sizeLimit; + get => _sizeLimit < 0 ? null : _sizeLimit; set { if (value < 0) @@ -32,7 +36,7 @@ public long? SizeLimit throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); } - _sizeLimit = value; + _sizeLimit = value ?? -1; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryDistributedCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryDistributedCache.cs index e867c7ecec0fdc..4bee051263267b 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryDistributedCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryDistributedCache.cs @@ -13,7 +13,7 @@ namespace Microsoft.Extensions.Caching.Distributed { public class MemoryDistributedCache : IDistributedCache { - private readonly IMemoryCache _memCache; + private readonly MemoryCache _memCache; public MemoryDistributedCache(IOptions optionsAccessor) : this(optionsAccessor, NullLoggerFactory.Instance) { } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index b932372bf3a3c7..0b7885ba81994d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -340,7 +340,7 @@ public void GetWithImplicitLinkPopulatesExpirationTokens(bool trackLinkedCacheEn string key = "myKey"; string key1 = "myKey1"; - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); ICacheEntry entry; using (entry = cache.CreateEntry(key)) @@ -351,7 +351,7 @@ public void GetWithImplicitLinkPopulatesExpirationTokens(bool trackLinkedCacheEn cache.Set(key1, obj, new MemoryCacheEntryOptions().AddExpirationToken(expirationToken)); } - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); Assert.Equal(trackLinkedCacheEntries ? 1 : 0, entry.ExpirationTokens.Count); Assert.Null(entry.AbsoluteExpiration); @@ -367,7 +367,7 @@ public void LinkContextsCanNest(bool trackLinkedCacheEntries) string key = "myKey"; string key1 = "myKey1"; - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); ICacheEntry entry; ICacheEntry entry1; @@ -387,7 +387,7 @@ public void LinkContextsCanNest(bool trackLinkedCacheEntries) VerifyCurrentEntry(trackLinkedCacheEntries, entry); } - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); Assert.Single(entry1.ExpirationTokens); Assert.Null(entry1.AbsoluteExpiration); @@ -543,11 +543,11 @@ private static void VerifyCurrentEntry(bool trackLinkedCacheEntries, ICacheEntry { if (trackLinkedCacheEntries) { - Assert.Same(entry, CacheEntryHelper.Current); + Assert.Same(entry, CacheEntry.Current); } else { - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs index 65810e352e9270..dcda7a0a74ed63 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs @@ -185,7 +185,7 @@ public void GetOrCreate_WillNotCreateEmptyValue_WhenFactoryThrows(bool trackLink Assert.False(cache.TryGetValue(key, out int obj)); // verify that throwing an exception doesn't leak CacheEntry objects - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); } [Theory] @@ -209,7 +209,7 @@ await cache.GetOrCreateAsync(key, entry => Assert.False(cache.TryGetValue(key, out int obj)); // verify that throwing an exception doesn't leak CacheEntry objects - Assert.Null(CacheEntryHelper.Current); + Assert.Null(CacheEntry.Current); } [Theory] From 390b5c262fe88c2ef0af82149ecaa7f822a82518 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 29 Mar 2022 22:35:17 +0100 Subject: [PATCH 12/14] Fix nullability and var use. --- .../src/CacheEntry.CacheEntryTokens.cs | 4 ++-- .../src/CacheEntry.cs | 8 ++++---- .../src/MemoryCache.cs | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index 4b2ace583c55dd..93e258e56e832d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -26,7 +26,7 @@ private sealed class CacheEntryTokens internal void AttachTokens(CacheEntry cacheEntry) { - var expirationTokens = _expirationTokens; + List? expirationTokens = _expirationTokens; if (expirationTokens is not null) { lock (this) @@ -47,7 +47,7 @@ internal void AttachTokens(CacheEntry cacheEntry) internal bool CheckForExpiredTokens(CacheEntry cacheEntry) { - var expirationTokens = _expirationTokens; + List? expirationTokens = _expirationTokens; if (expirationTokens is not null) { for (int i = 0; i < expirationTokens.Count; i++) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 496f5f2cac54a7..06291cd9041fa2 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -15,7 +15,7 @@ namespace Microsoft.Extensions.Caching.Memory internal sealed partial class CacheEntry : ICacheEntry { private static readonly Action ExpirationCallback = ExpirationTokensExpired; - private static readonly AsyncLocal _current = new AsyncLocal(); + private static readonly AsyncLocal _current = new AsyncLocal(); private readonly MemoryCache _cache; @@ -39,14 +39,14 @@ internal CacheEntry(object key!!, MemoryCache memoryCache!!) _cache = memoryCache; if (memoryCache.TrackLinkedCacheEntries) { - var holder = _current; + AsyncLocal holder = _current; _previous = holder.Value; holder.Value = this; } } // internal for testing - internal static CacheEntry Current => _current.Value; + internal static CacheEntry? Current => _current.Value; private ref ulong RawAbsoluteExpiration => ref Unsafe.As(ref _absoluteExpiration); @@ -203,7 +203,7 @@ public void Dispose() { _cache.SetEntry(this); - CacheEntry parent = _previous; + CacheEntry? parent = _previous; if (parent != null) { if (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || RawAbsoluteExpiration < parent.RawAbsoluteExpiration)) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index bf709d72dde06d..2a9347bcfc3761 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -97,7 +97,7 @@ internal void SetEntry(CacheEntry entry) // it was set by cascading it to its parent. if (entry.AbsoluteExpirationRelativeToNow.Ticks > 0) { - var absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; + DateTime absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; if (!entry.HasAbsoluteExpiration || absoluteExpiration < entry.AbsoluteExpiration) { entry.SetAbsoluteExpirationUtc(absoluteExpiration); @@ -190,7 +190,7 @@ public bool TryGetValue(object key!!, out object? result) DateTime utcNow = UtcNow; CoherentState coherentState = _coherentState; // Clear() can update the reference in the meantime - if (coherentState._entries.TryGetValue(key, out CacheEntry tmp)) + if (coherentState._entries.TryGetValue(key, out CacheEntry? tmp)) { CacheEntry entry = tmp; // Check if expired due to expiration tokens, timers, etc. and if so, remove it. @@ -252,7 +252,7 @@ public void Clear() CheckDisposed(); CoherentState oldState = Interlocked.Exchange(ref _coherentState, new CoherentState()); - foreach (var entry in oldState._entries) + foreach (KeyValuePair entry in oldState._entries) { entry.Value.SetExpired(EvictionReason.Removed); entry.Value.InvokeEvictionCallbacks(); @@ -308,9 +308,9 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CoherentState cohe return false; } + long sizeRead = coherentState.Size; for (int i = 0; i < 100; i++) { - long sizeRead = coherentState.Size; long newSize = sizeRead + entry.Size; if ((ulong)newSize > (ulong)sizeLimit) @@ -499,7 +499,7 @@ private sealed class CoherentState internal int Count => _entries.Count; - internal long Size => Interlocked.Read(ref _cacheSize); + internal long Size => Volatile.Read(ref _cacheSize); internal void RemoveEntry(CacheEntry entry, MemoryCacheOptions options) { From e1deafbdf067ab7b8d851188c6ff9a5af1e4c6c5 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Tue, 29 Mar 2022 22:59:00 +0100 Subject: [PATCH 13/14] Fix nullability --- .../Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 06291cd9041fa2..2f7e8ac91fd8e6 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -39,7 +39,7 @@ internal CacheEntry(object key!!, MemoryCache memoryCache!!) _cache = memoryCache; if (memoryCache.TrackLinkedCacheEntries) { - AsyncLocal holder = _current; + AsyncLocal holder = _current; _previous = holder.Value; holder.Value = this; } From 27f046a1bb7eaa942c65faea0fa8268646a89e48 Mon Sep 17 00:00:00 2001 From: Pent Ploompuu Date: Mon, 25 Apr 2022 23:10:42 +0300 Subject: [PATCH 14/14] Address PR feedback --- .../src/CacheEntry.cs | 93 ++++++++++--------- .../src/MemoryCache.cs | 8 +- .../src/MemoryCacheOptions.cs | 6 +- 3 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index ba698b4ce8478f..28eafc51ef5db5 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -22,10 +22,10 @@ internal sealed partial class CacheEntry : ICacheEntry private CacheEntryTokens? _tokens; // might be null if user is not using the tokens or callbacks private TimeSpan _absoluteExpirationRelativeToNow; private TimeSpan _slidingExpiration; - private long _size = -1; + private long _size = NotSet; private CacheEntry? _previous; // this field is not null only before the entry is added to the cache and tracking is enabled private object? _value; - private DateTime _absoluteExpiration; + private long _absoluteExpirationTicks = NotSet; private short _absoluteExpirationOffsetMinutes; private bool _isDisposed; private bool _isExpired; @@ -33,6 +33,8 @@ internal sealed partial class CacheEntry : ICacheEntry private byte _evictionReason; private byte _priority = (byte)CacheItemPriority.Normal; + private const int NotSet = -1; + internal CacheEntry(object key, MemoryCache memoryCache) { ThrowHelper.ThrowIfNull(key); @@ -51,40 +53,37 @@ internal CacheEntry(object key, MemoryCache memoryCache) // internal for testing internal static CacheEntry? Current => _current.Value; - private ref ulong RawAbsoluteExpiration => ref Unsafe.As(ref _absoluteExpiration); - - internal bool HasAbsoluteExpiration => RawAbsoluteExpiration != 0; - - internal DateTime AbsoluteExpiration => _absoluteExpiration; - - internal void SetAbsoluteExpirationUtc(DateTime value) + internal long AbsoluteExpirationTicks { - Debug.Assert(value.Kind == DateTimeKind.Utc); - _absoluteExpiration = value; - _absoluteExpirationOffsetMinutes = 0; + get => _absoluteExpirationTicks; + set + { + _absoluteExpirationTicks = value; + _absoluteExpirationOffsetMinutes = 0; + } } DateTimeOffset? ICacheEntry.AbsoluteExpiration { get { - if (!HasAbsoluteExpiration) + if (_absoluteExpirationTicks < 0) return null; var offset = new TimeSpan(_absoluteExpirationOffsetMinutes * TimeSpan.TicksPerMinute); - return new DateTimeOffset(_absoluteExpiration.Ticks + offset.Ticks, offset); + return new DateTimeOffset(_absoluteExpirationTicks + offset.Ticks, offset); } set { if (value is null) { - _absoluteExpiration = default; + _absoluteExpirationTicks = NotSet; _absoluteExpirationOffsetMinutes = default; } else { DateTimeOffset expiration = value.GetValueOrDefault(); - _absoluteExpiration = expiration.UtcDateTime; + _absoluteExpirationTicks = expiration.UtcTicks; _absoluteExpirationOffsetMinutes = (short)(expiration.Offset.Ticks / TimeSpan.TicksPerMinute); } } @@ -163,11 +162,7 @@ public TimeSpan? SlidingExpiration throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); } - // disallow entry size changes after it has been committed - if (_isDisposed) - return; - - _size = value ?? -1; + _size = value ?? NotSet; } } @@ -195,31 +190,40 @@ public void Dispose() if (_cache.TrackLinkedCacheEntries) { - Debug.Assert(_current.Value == this, "Entries disposed in invalid order"); - _current.Value = _previous; + CommitWithTracking(); } - - // 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) + else if (_isValueSet) { _cache.SetEntry(this); + } + } + } + + private void CommitWithTracking() + { + Debug.Assert(_current.Value == this, "Entries disposed in invalid order"); + _current.Value = _previous; + + // 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) + { + _cache.SetEntry(this); - CacheEntry? parent = _previous; - if (parent != null) + CacheEntry? parent = _previous; + if (parent != null) + { + if ((ulong)_absoluteExpirationTicks < (ulong)parent._absoluteExpirationTicks) { - if (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || RawAbsoluteExpiration < parent.RawAbsoluteExpiration)) - { - parent._absoluteExpiration = _absoluteExpiration; - parent._absoluteExpirationOffsetMinutes = _absoluteExpirationOffsetMinutes; - } - _tokens?.PropagateTokens(parent); + parent._absoluteExpirationTicks = _absoluteExpirationTicks; + parent._absoluteExpirationOffsetMinutes = _absoluteExpirationOffsetMinutes; } + _tokens?.PropagateTokens(parent); } - - _previous = null; // we don't want to root unnecessary objects } + + _previous = null; // we don't want to root unnecessary objects } [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling @@ -241,7 +245,7 @@ internal void SetExpired(EvictionReason reason) [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling private bool CheckForExpiredTime(DateTime utcNow) { - if (!HasAbsoluteExpiration && _slidingExpiration.Ticks == 0) + if (_absoluteExpirationTicks < 0 && _slidingExpiration.Ticks == 0) { return false; } @@ -250,7 +254,7 @@ private bool CheckForExpiredTime(DateTime utcNow) bool FullCheck(DateTime utcNow) { - if (HasAbsoluteExpiration && _absoluteExpiration <= utcNow) + if ((ulong)_absoluteExpirationTicks <= (ulong)utcNow.Ticks) { SetExpired(EvictionReason.Expired); return true; @@ -282,21 +286,18 @@ private static void ExpirationTokensExpired(object obj) internal void InvokeEvictionCallbacks() => _tokens?.InvokeEvictionCallbacks(this); - internal bool CanPropagateTokens() => _tokens != null && _tokens.CanPropagateTokens(); - internal void PropagateOptionsToCurrent() { - CacheEntry? parent = _current.Value; - if (parent == null) + if ((_tokens == null || !_tokens.CanPropagateTokens()) && _absoluteExpirationTicks < 0 || _current.Value is not CacheEntry parent) { return; } // 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 (HasAbsoluteExpiration && (!parent.HasAbsoluteExpiration || RawAbsoluteExpiration < parent.RawAbsoluteExpiration)) + if ((ulong)_absoluteExpirationTicks < (ulong)parent._absoluteExpirationTicks) { - parent._absoluteExpiration = _absoluteExpiration; + parent._absoluteExpirationTicks = _absoluteExpirationTicks; parent._absoluteExpirationOffsetMinutes = _absoluteExpirationOffsetMinutes; } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 0fe73624a88bde..445a575b371e8f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -110,10 +110,10 @@ internal void SetEntry(CacheEntry entry) // it was set by cascading it to its parent. if (entry.AbsoluteExpirationRelativeToNow.Ticks > 0) { - DateTime absoluteExpiration = utcNow + entry.AbsoluteExpirationRelativeToNow; - if (!entry.HasAbsoluteExpiration || absoluteExpiration < entry.AbsoluteExpiration) + long absoluteExpiration = (utcNow + entry.AbsoluteExpirationRelativeToNow).Ticks; + if ((ulong)absoluteExpiration < (ulong)entry.AbsoluteExpirationTicks) { - entry.SetAbsoluteExpirationUtc(absoluteExpiration); + entry.AbsoluteExpirationTicks = absoluteExpiration; } } @@ -215,7 +215,7 @@ public bool TryGetValue(object key, out object? result) entry.LastAccessed = utcNow; result = entry.Value; - if (TrackLinkedCacheEntries && (entry.CanPropagateTokens() || entry.HasAbsoluteExpiration)) + if (TrackLinkedCacheEntries) { // When this entry is retrieved in the scope of creating another entry, // that entry needs a copy of these expiration tokens. diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs index daeeecb0dce1f3..efb5b197d9378c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs @@ -9,9 +9,11 @@ namespace Microsoft.Extensions.Caching.Memory { public class MemoryCacheOptions : IOptions { - private long _sizeLimit = -1; + private long _sizeLimit = NotSet; private double _compactionPercentage = 0.05; + private const int NotSet = -1; + public ISystemClock? Clock { get; set; } /// @@ -36,7 +38,7 @@ public long? SizeLimit throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(value)} must be non-negative."); } - _sizeLimit = value ?? -1; + _sizeLimit = value ?? NotSet; } }