From dd6f6d3df4c9105b925e72e876fabf288244e81f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 16:27:27 +0100 Subject: [PATCH 1/7] don't get current time and check if scan for expired items should be performed, use Timer instead --- .../src/MemoryCache.cs | 41 +++++-------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index e863e4d1a221d..db40c34028e79 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -4,9 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Runtime.CompilerServices; using System.Threading; -using System.Threading.Tasks; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -24,10 +22,10 @@ public class MemoryCache : IMemoryCache private readonly MemoryCacheOptions _options; private readonly ConcurrentDictionary _entries; + private readonly System.Timers.Timer _timer; private long _cacheSize; private bool _disposed; - private DateTimeOffset _lastExpirationScan; /// /// Creates a new instance. @@ -63,7 +61,9 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory _options.Clock = new SystemClock(); } - _lastExpirationScan = _options.Clock.UtcNow; + _timer = new System.Timers.Timer(Math.Max(1, _options.ExpirationScanFrequency.TotalMilliseconds)); + _timer.Elapsed += OnTimerElapsed; + _timer.Start(); } /// @@ -211,8 +211,6 @@ internal void SetEntry(CacheEntry entry) RemoveEntry(priorEntry); } } - - StartScanForExpiredItemsIfNeeded(utcNow); } /// @@ -239,8 +237,6 @@ public bool TryGetValue(object key, out object result) entry.PropagateOptions(CacheEntryHelper.Current); } - StartScanForExpiredItemsIfNeeded(utcNow); - return true; } else @@ -250,8 +246,6 @@ public bool TryGetValue(object key, out object result) } } - StartScanForExpiredItemsIfNeeded(utcNow); - result = null; return false; } @@ -272,8 +266,6 @@ public void Remove(object key) entry.SetExpired(EvictionReason.Removed); entry.InvokeEvictionCallbacks(); } - - StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); } private void RemoveEntry(CacheEntry entry) @@ -292,30 +284,13 @@ internal void EntryExpired(CacheEntry entry) { // TODO: For efficiency consider processing these expirations in batches. RemoveEntry(entry); - StartScanForExpiredItemsIfNeeded(_options.Clock.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) - { - if (_options.ExpirationScanFrequency < utcNow - _lastExpirationScan) - { - ScheduleTask(utcNow); - } - - void ScheduleTask(DateTimeOffset utcNow) - { - _lastExpirationScan = utcNow; - Task.Factory.StartNew(state => ScanForExpiredItems((MemoryCache)state), this, - CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); - } - } + private void OnTimerElapsed(object sender, System.Timers.ElapsedEventArgs e) => ScanForExpiredItems(this); private static void ScanForExpiredItems(MemoryCache cache) { - DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow; + DateTimeOffset now = cache._options.Clock.UtcNow; foreach (CacheEntry entry in cache._entries.Values) { if (entry.CheckExpired(now)) @@ -483,6 +458,10 @@ protected virtual void Dispose(bool disposing) GC.SuppressFinalize(this); } + _timer.Stop(); + _timer.Elapsed -= OnTimerElapsed; + _timer.Dispose(); + _disposed = true; } } From c62b676a6542d6a55ed5aa37ec6c51bbeec04445 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 16:28:24 +0100 Subject: [PATCH 2/7] remove duplicated code from tests --- .../tests/CacheEntryScopeExpirationTests.cs | 15 ++------ .../tests/Infrastructure/CacheFactory.cs | 36 +++++++++++++++++++ .../tests/TimeExpirationTests.cs | 10 ++---- .../tests/TokenExpirationTests.cs | 15 ++------ 4 files changed, 42 insertions(+), 34 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.Caching.Memory/tests/Infrastructure/CacheFactory.cs diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index 3ca46d0c024d6..e9336acd599e1 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -8,23 +8,12 @@ using Microsoft.Extensions.Internal; using Xunit; +using static Microsoft.Extensions.Internal.CacheFactory; + namespace Microsoft.Extensions.Caching.Memory { public class CacheEntryScopeExpirationTests { - private IMemoryCache CreateCache() - { - return CreateCache(new SystemClock()); - } - - private IMemoryCache CreateCache(ISystemClock clock) - { - return new MemoryCache(new MemoryCacheOptions() - { - Clock = clock, - }); - } - [Fact] public void SetPopulates_ExpirationTokens_IntoScopedLink() { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Infrastructure/CacheFactory.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Infrastructure/CacheFactory.cs new file mode 100644 index 0000000000000..e6d75f1b5a833 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Infrastructure/CacheFactory.cs @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + + +using System; +using Microsoft.Extensions.Caching.Memory; + +namespace Microsoft.Extensions.Internal +{ + internal static class CacheFactory + { + internal static IMemoryCache CreateCache() + { + return CreateCache(new SystemClock()); + } + + internal static IMemoryCache CreateCache(ISystemClock clock) + { + return new MemoryCache(new MemoryCacheOptions() + { + Clock = clock, + }); + } + + internal static IMemoryCache CreateCache(ISystemClock clock, TimeSpan expirationScanFrequency) + { + var options = new MemoryCacheOptions() + { + Clock = clock, + ExpirationScanFrequency = expirationScanFrequency, + }; + + return new MemoryCache(options); + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs index 4a26ad05a9fd3..72c39c294ef9e 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs @@ -6,18 +6,12 @@ using Microsoft.Extensions.Internal; using Xunit; +using static Microsoft.Extensions.Internal.CacheFactory; + namespace Microsoft.Extensions.Caching.Memory { public class TimeExpirationTests { - private IMemoryCache CreateCache(ISystemClock clock) - { - return new MemoryCache(new MemoryCacheOptions() - { - Clock = clock, - }); - } - [Fact] public void AbsoluteExpirationInThePastDoesntAddEntry() { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs index 3da2515de8789..7bb1f156100f3 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs @@ -9,23 +9,12 @@ using Microsoft.Extensions.Primitives; using Xunit; +using static Microsoft.Extensions.Internal.CacheFactory; + namespace Microsoft.Extensions.Caching.Memory { public class TokenExpirationTests { - private IMemoryCache CreateCache() - { - return CreateCache(new SystemClock()); - } - - private IMemoryCache CreateCache(ISystemClock clock) - { - return new MemoryCache(new MemoryCacheOptions() - { - Clock = clock, - }); - } - [Fact] public void SetWithTokenRegistersForNotification() { From 1ca3e9a0de23d42c08e2f85e7f7beaf7f67b40c5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 16:30:08 +0100 Subject: [PATCH 3/7] update tests: background activity is not required anymore --- .../tests/TimeExpirationTests.cs | 2 -- .../tests/TokenExpirationTests.cs | 1 - 2 files changed, 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs index 72c39c294ef9e..38dfb25bb779f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs @@ -73,8 +73,6 @@ public void AbsoluteExpirationExpiresInBackground() Assert.Same(value, result); clock.Add(TimeSpan.FromMinutes(2)); - var ignored = cache.Get("otherKey"); // Background expiration checks are triggered by misc cache activity. - Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(30)), "Callback"); found = cache.TryGetValue(key, out result); diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs index 7bb1f156100f3..c8020a1cbca83 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs @@ -121,7 +121,6 @@ public void ExpiredLazyTokenRemovesItemInBackground() clock.Add(TimeSpan.FromMinutes(2)); expirationToken.HasChanged = true; - var ignored = cache.Get("otherKey"); // Background expiration checks are triggered by misc cache activity. Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(30)), "Callback"); found = cache.TryGetValue(key, out value); From e498ae8bc8734aa7b0160abcf573ccaaf93c771e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 16:31:10 +0100 Subject: [PATCH 4/7] update tests: don't use the default expirationScanFrequency as these two tests would need to wait for a minute to get result (CI would not like it) --- .../tests/TimeExpirationTests.cs | 3 ++- .../tests/TokenExpirationTests.cs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs index 38dfb25bb779f..2e4a2e172fb3c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs @@ -52,7 +52,8 @@ public void AbsoluteExpirationExpires() public void AbsoluteExpirationExpiresInBackground() { var clock = new TestClock(); - var cache = CreateCache(clock); + TimeSpan expirationScanFrequency = TimeSpan.FromMilliseconds(100); + var cache = CreateCache(clock, expirationScanFrequency); var key = "myKey"; var value = new object(); var callbackInvoked = new ManualResetEvent(false); diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs index c8020a1cbca83..fa862f52cff72 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs @@ -103,7 +103,8 @@ public void ExpiredLazyTokenRemovesItemOnNextAccess() public void ExpiredLazyTokenRemovesItemInBackground() { var clock = new TestClock(); - var cache = CreateCache(clock); + TimeSpan expirationScanFrequency = TimeSpan.FromMilliseconds(100); + var cache = CreateCache(clock, expirationScanFrequency); string key = "myKey"; var value = new object(); var callbackInvoked = new ManualResetEvent(false); From 1ca7ab44276f72d1e28aa6af9838bc431625be52 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 16:59:55 +0100 Subject: [PATCH 5/7] introduce CanExpire flag --- .../src/CacheEntry.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 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 47e3c5c8fd417..b0974465adfb0 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -33,6 +33,7 @@ internal CacheEntry(object key, MemoryCache memoryCache) Key = key ?? throw new ArgumentNullException(nameof(key)); _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); _scope = CacheEntryHelper.EnterScope(this); + _state = (int)State.CanExpire; } /// @@ -133,6 +134,8 @@ public object Value internal EvictionReason EvictionReason { get; private set; } + internal bool CanExpire { get => ((State)_state).HasFlag(State.CanExpire); set => Set(State.CanExpire, value); } + private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } @@ -155,6 +158,9 @@ public void Dispose() // so don't use this entry. if (IsValueSet) { + // set the CanExpire flag before adding the entry to the cache + CanExpire = AbsoluteExpiration.HasValue || SlidingExpiration.HasValue || AbsoluteExpirationRelativeToNow.HasValue || _expirationTokens != null; + _cache.SetEntry(this); if (CanPropagateOptions()) @@ -166,7 +172,7 @@ public void Dispose() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal bool CheckExpired(in DateTimeOffset now) => IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); + internal bool CheckExpired(in DateTimeOffset now) => IsExpired || (CanExpire && (CheckForExpiredTime(now) || CheckForExpiredTokens())); internal void SetExpired(EvictionReason reason) { @@ -338,6 +344,7 @@ internal void PropagateOptions(CacheEntry parent) { parent.AddExpirationToken(expirationToken); } + parent.CanExpire = true; } } } @@ -347,6 +354,7 @@ internal void PropagateOptions(CacheEntry parent) if (!parent.AbsoluteExpiration.HasValue || AbsoluteExpiration < parent.AbsoluteExpiration) { parent.AbsoluteExpiration = AbsoluteExpiration; + parent.CanExpire = true; } } } @@ -369,6 +377,7 @@ private enum State IsValueSet = 1 << 0, IsExpired = 1 << 1, IsDisposed = 1 << 2, + CanExpire = 1 << 3, } } } From 38cff0e0a8d3d7ef30a0b1e7e308aa0c7f8c6d55 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 17:02:05 +0100 Subject: [PATCH 6/7] don't ask for current time if the entry that has been found can not be expired (TechEmpower case) --- .../src/MemoryCache.cs | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index db40c34028e79..aa75cf7892783 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -219,35 +219,34 @@ public bool TryGetValue(object key, out object result) ValidateCacheKey(key); CheckDisposed(); - DateTimeOffset utcNow = _options.Clock.UtcNow; + if (!_entries.TryGetValue(key, out CacheEntry entry)) + { + result = null; + return false; + } - if (_entries.TryGetValue(key, out CacheEntry entry)) + if (entry.CanExpire) { - // 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) + DateTimeOffset utcNow = _options.Clock.UtcNow; + if (entry.EvictionReason != EvictionReason.Replaced && entry.CheckExpired(utcNow)) { - entry.LastAccessed = utcNow; - result = entry.Value; - - if (entry.CanPropagateOptions()) - { - // 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); - } + RemoveEntry(entry); - return true; + result = null; + return false; } - else + + entry.LastAccessed = utcNow; + if (entry.CanPropagateOptions()) { - // TODO: For efficiency queue this up for batch removal - RemoveEntry(entry); + // 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); } } - result = null; - return false; + result = entry.Value; + return true; } /// From d2397e1d37c8927256e152ca3051dcbd8efa3ae7 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 4 Dec 2020 20:50:38 +0100 Subject: [PATCH 7/7] based on profiling apply or revert some micro optimizations --- .../src/CacheEntry.cs | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index b0974465adfb0..75062e973b3d9 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -134,11 +134,21 @@ public object Value internal EvictionReason EvictionReason { get; private set; } - internal bool CanExpire { get => ((State)_state).HasFlag(State.CanExpire); set => Set(State.CanExpire, value); } + internal bool CanExpire + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ((State)_state).HasFlag(State.CanExpire); + set => Set(State.CanExpire, value); + } private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } - private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } + private bool IsExpired + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => ((State)_state).HasFlag(State.IsExpired); + set => Set(State.IsExpired, value); + } private bool IsValueSet { get => ((State)_state).HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); } @@ -172,7 +182,7 @@ public void Dispose() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal bool CheckExpired(in DateTimeOffset now) => IsExpired || (CanExpire && (CheckForExpiredTime(now) || CheckForExpiredTokens())); + internal bool CheckExpired(DateTimeOffset now) => IsExpired || (CanExpire && (CheckForExpiredTime(now) || CheckForExpiredTokens())); internal void SetExpired(EvictionReason reason) { @@ -184,32 +194,22 @@ internal void SetExpired(EvictionReason reason) DetachTokens(); } - private bool CheckForExpiredTime(in DateTimeOffset now) + private bool CheckForExpiredTime(DateTimeOffset now) { - if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue) + if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value <= now) { - return false; + SetExpired(EvictionReason.Expired); + return true; } - return FullCheck(now); - - bool FullCheck(in DateTimeOffset offset) + if (_slidingExpiration.HasValue + && (now - LastAccessed) >= _slidingExpiration) { - if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value <= offset) - { - SetExpired(EvictionReason.Expired); - return true; - } - - if (_slidingExpiration.HasValue - && (offset - LastAccessed) >= _slidingExpiration) - { - SetExpired(EvictionReason.Expired); - return true; - } - - return false; + SetExpired(EvictionReason.Expired); + return true; } + + return false; } private bool CheckForExpiredTokens() @@ -323,6 +323,7 @@ private static void InvokeCallbacks(CacheEntry entry) } // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) + [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent)