From 6fa2ccf9b94bce3148e1839d4e07172d5091d8c6 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Nov 2020 12:50:45 +0100 Subject: [PATCH 1/6] make ValidateCacheKey and CheckDisposed inlinable by moving throws to separate methods --- .../src/MemoryCache.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index a3d22ca18e2dca..2df7948e1c1876 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -500,16 +500,20 @@ private void CheckDisposed() { if (_disposed) { - throw new ObjectDisposedException(typeof(MemoryCache).FullName); + Throw(); } + + void Throw() => throw new ObjectDisposedException(typeof(MemoryCache).FullName); } private static void ValidateCacheKey(object key) { if (key == null) { - throw new ArgumentNullException(nameof(key)); + Throw(); } + + void Throw() => throw new ArgumentNullException(nameof(key)); } } } From 9de3a9eb081f4bc60c3994f1c7c3f8427d0067e0 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Nov 2020 12:53:31 +0100 Subject: [PATCH 2/6] ensure that the check for expiration does not require a method call for the most common case --- .../src/MemoryCache.cs | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 2df7948e1c1876..c5c99401a1096b 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -5,6 +5,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Internal; @@ -230,44 +231,45 @@ private void SetEntry(CacheEntry entry) } } - StartScanForExpiredItems(utcNow); + StartScanForExpiredItemsIfNeeded(utcNow); } /// public bool TryGetValue(object key, out object result) { ValidateCacheKey(key); - CheckDisposed(); - result = null; DateTimeOffset utcNow = _options.Clock.UtcNow; - bool found = false; if (_entries.TryGetValue(key, out CacheEntry entry)) { // 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) - { - // TODO: For efficiency queue this up for batch removal - RemoveEntry(entry); - } - else + if (!(entry.CheckExpired(utcNow) && entry.EvictionReason != EvictionReason.Replaced)) { - found = true; entry.LastAccessed = utcNow; result = entry.Value; // 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); + + StartScanForExpiredItemsIfNeeded(utcNow); + + return true; + } + else + { + // TODO: For efficiency queue this up for batch removal + RemoveEntry(entry); } } - StartScanForExpiredItems(utcNow); + StartScanForExpiredItemsIfNeeded(utcNow); - return found; + result = null; + return false; } /// @@ -287,7 +289,7 @@ public void Remove(object key) entry.InvokeEvictionCallbacks(); } - StartScanForExpiredItems(); + StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); } private void RemoveEntry(CacheEntry entry) @@ -306,18 +308,23 @@ private void EntryExpired(CacheEntry entry) { // TODO: For efficiency consider processing these expirations in batches. RemoveEntry(entry); - StartScanForExpiredItems(); + 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. - private void StartScanForExpiredItems(DateTimeOffset? utcNow = null) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void StartScanForExpiredItemsIfNeeded(DateTimeOffset utcNow) { // Since fetching time is expensive, minimize it in the hot paths - DateTimeOffset now = utcNow ?? _options.Clock.UtcNow; - if (_options.ExpirationScanFrequency < now - _lastExpirationScan) + if (_options.ExpirationScanFrequency < utcNow - _lastExpirationScan) + { + ScheduleTask(utcNow); + } + + void ScheduleTask(DateTimeOffset utcNow) { - _lastExpirationScan = now; + _lastExpirationScan = utcNow; Task.Factory.StartNew(state => ScanForExpiredItems((MemoryCache)state), this, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); } From bcc85841d621e0732a549110c3dfc117633a49ec Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Nov 2020 12:54:11 +0100 Subject: [PATCH 3/6] update the last expiration scan when the Scan Task starts actual work --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index c5c99401a1096b..4a9748d7e546d1 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -332,7 +332,7 @@ void ScheduleTask(DateTimeOffset utcNow) private static void ScanForExpiredItems(MemoryCache cache) { - DateTimeOffset now = cache._options.Clock.UtcNow; + DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow; foreach (CacheEntry entry in cache._entries.Values) { if (entry.CheckExpired(now)) From 5d395aaa741bfc3c45a3713a0100a29954751285 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Nov 2020 15:53:50 +0100 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Stephen Toub --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 4a9748d7e546d1..ac8e45c2803a56 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -510,7 +510,7 @@ private void CheckDisposed() Throw(); } - void Throw() => throw new ObjectDisposedException(typeof(MemoryCache).FullName); + static void Throw() => throw new ObjectDisposedException(typeof(MemoryCache).FullName); } private static void ValidateCacheKey(object key) @@ -520,7 +520,7 @@ private static void ValidateCacheKey(object key) Throw(); } - void Throw() => throw new ArgumentNullException(nameof(key)); + static void Throw() => throw new ArgumentNullException(nameof(key)); } } } From e03d0a2486c0a86a07404f4340b49c4c941e044c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 17 Nov 2020 16:38:17 +0100 Subject: [PATCH 5/6] Update src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index ac8e45c2803a56..c3444bb1fc52ef 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -246,7 +246,7 @@ public bool TryGetValue(object key, out object result) { // 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)) + if (!entry.CheckExpired(utcNow) || entry.EvictionReason == EvictionReason.Replaced) { entry.LastAccessed = utcNow; result = entry.Value; From 4eb26ad35a0646ca2b999df55985189aec60a7da Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 18 Nov 2020 11:48:06 +0100 Subject: [PATCH 6/6] remove outdated comment --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index c3444bb1fc52ef..55dd1217e65f9c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -316,7 +316,6 @@ private void EntryExpired(CacheEntry entry) [MethodImpl(MethodImplOptions.AggressiveInlining)] private void StartScanForExpiredItemsIfNeeded(DateTimeOffset utcNow) { - // Since fetching time is expensive, minimize it in the hot paths if (_options.ExpirationScanFrequency < utcNow - _lastExpirationScan) { ScheduleTask(utcNow);