avoid deserialize inside lock, involves refactoring cache item so we can reserve against a cache-item

Author: mgravell

src/Caching/Hybrid/src/HybridCacheBuilderExtensions.cs

@@ -1,12 +1,7 @@
 // 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 System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Microsoft.Extensions.Caching.Hybrid;

src/Caching/Hybrid/src/HybridCacheOptions.cs

@@ -1,13 +1,6 @@
 // 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 System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
-using Microsoft.Extensions.Options;
-
 namespace Microsoft.Extensions.Caching.Hybrid;
 
 /// <summary>

src/Caching/Hybrid/src/HybridCacheServiceExtensions.cs

@@ -2,15 +2,9 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
-using System.Collections.Generic;
-using System.Diagnostics.CodeAnalysis;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
 using Microsoft.Extensions.Caching.Hybrid.Internal;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.DependencyInjection.Extensions;
-using Microsoft.Extensions.Internal;
 
 namespace Microsoft.Extensions.Caching.Hybrid;
 

src/Caching/Hybrid/src/IHybridCacheBuilder.cs

@@ -1,11 +1,6 @@
 // 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 System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Microsoft.Extensions.Caching.Hybrid;

src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Threading;
 using Microsoft.Extensions.Caching.Memory;
 
 namespace Microsoft.Extensions.Caching.Hybrid.Internal;
@@ -10,36 +11,79 @@ partial class DefaultHybridCache
 {
     internal abstract class CacheItem
     {
+        private int _refCount = 1; // the number of pending operations against this cache item
+        // note: the ref count is the number of callers anticipating this value at any given time; initially,
+        // it is one for a simple "get the value" flow, but if another call joins with us, it'll be incremented;
+        // if either cancels, it will get decremented, with the entire flow being cancelled if it ever becomes
+        // zero
+        // this counter also drives cache lifetime, with the cache itself incrementing the count by one; in the
+        // case of mutable data, cache eviction may reduce this to zero (in cooperation with any concurrent readers,
+        // who incr/decr around their fetch), allowing safe buffer recycling
+
+        internal int RefCount => Volatile.Read(ref _refCount);
+
         internal static readonly PostEvictionDelegate _sharedOnEviction = static (key, value, reason, state) =>
         {
             if (value is CacheItem item)
             {
-                // perform per-item clean-up, i.e. buffer recycling (if defensive copies needed)
-                item.OnEviction();
+                item.Release();
             }
         };
 
-        public virtual void Release() { } // for recycling purposes
-
         public virtual bool NeedsEvictionCallback => false; // do we need to call Release when evicted?
 
-        public virtual void OnEviction() { } // only invoked if NeedsEvictionCallback reported true
+        protected virtual void OnFinalRelease() { } // any required release semantics
 
         public abstract bool TryReserveBuffer(out BufferChunk buffer);
 
         public abstract bool DebugIsImmutable { get; }
+
+        public bool Release() // returns true ONLY for the final release step
+        {
+            var newCount = Interlocked.Decrement(ref _refCount);
+            if (newCount == 0)
+            {
+                // perform per-item clean-up, i.e. buffer recycling (if defensive copies needed)
+                OnFinalRelease();
+                return true;
+            }
+            return false;
+        }
+
+        public bool TryReserve()
+        {
+            var oldValue = Volatile.Read(ref _refCount);
+            do
+            {
+                if (oldValue is 0 or -1)
+                {
+                    return false; // already burned, or about to roll around back to zero
+                }
+
+                var updated = Interlocked.CompareExchange(ref _refCount, oldValue + 1, oldValue);
+                if (updated == oldValue)
+                {
+                    return true; // we exchanged
+                }
+                oldValue = updated; // we failed, but we have an updated state
+            } while (true);
+        }
+
     }
 
     internal abstract class CacheItem<T> : CacheItem
     {
+        // attempt to get a value that was *not* previously reserved
         public abstract bool TryGetValue(out T value);
 
-        public T GetValue()
+        // get a value that *was* reserved, countermanding our reservation in the process
+        public T GetReservedValue()
         {
             if (!TryGetValue(out var value))
             {
                 Throw();
             }
+            Release();
             return value;
 
             static void Throw() => throw new ObjectDisposedException("The cache item has been recycled before the value was obtained");

src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs

@@ -11,13 +11,14 @@ partial class DefaultHybridCache
 {
     private sealed class ImmutableCacheItem<T> : CacheItem<T> // used to hold types that do not require defensive copies
     {
-        private readonly T _value;
-        public ImmutableCacheItem(T value) => _value = value;
+        private T _value = default!; // deferred until SetValue
+
+        public void SetValue(T value) => _value = value;
 
         private static ImmutableCacheItem<T>? _sharedDefault;
 
         // this is only used when the underlying store is disabled; we don't need 100% singleton; "good enough is"
-        public static ImmutableCacheItem<T> Default => _sharedDefault ??= new(default!);
+        public static ImmutableCacheItem<T> Default => _sharedDefault ??= new();
 
         public override bool TryGetValue(out T value)
         {

src/Caching/Hybrid/src/Internal/DefaultHybridCache.L2.cs

@@ -113,13 +113,16 @@ private DistributedCacheEntryOptions GetOptions(HybridCacheEntryOptions? options
 
     internal void SetL1<T>(string key, CacheItem<T> value, HybridCacheEntryOptions? options)
     {
-        // based on CacheExtensions.Set<TItem>, but with post-eviction recycling
-        using var cacheEntry = _localCache.CreateEntry(key);
-        cacheEntry.AbsoluteExpirationRelativeToNow = options?.LocalCacheExpiration ?? _defaultLocalCacheExpiration;
-        cacheEntry.Value = value;
-        if (value.NeedsEvictionCallback)
+        if (value.TryReserve()) // incr ref-count for the the cache itself; this *may* be released via the NeedsEvictionCallback path
         {
-            cacheEntry.RegisterPostEvictionCallback(CacheItem._sharedOnEviction);
+            // based on CacheExtensions.Set<TItem>, but with post-eviction recycling
+            using var cacheEntry = _localCache.CreateEntry(key);
+            cacheEntry.AbsoluteExpirationRelativeToNow = options?.LocalCacheExpiration ?? _defaultLocalCacheExpiration;
+            cacheEntry.Value = value;
+            if (value.NeedsEvictionCallback)
+            {
+                cacheEntry.RegisterPostEvictionCallback(CacheItem._sharedOnEviction);
+            }
         }
     }
 }

src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs

@@ -1,28 +1,23 @@
 // 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 System.Diagnostics;
-using System.Threading;
-
 namespace Microsoft.Extensions.Caching.Hybrid.Internal;
 
 partial class DefaultHybridCache
 {
     private sealed partial class MutableCacheItem<T> : CacheItem<T> // used to hold types that require defensive copies
     {
-        private readonly IHybridCacheSerializer<T> _serializer;
-        private readonly BufferChunk _buffer;
-        private int _refCount = 1; // buffer released when this becomes zero
+        private IHybridCacheSerializer<T> _serializer = null!; // deferred until SetValue
+        private BufferChunk _buffer;
 
-        public MutableCacheItem(ref BufferChunk buffer, IHybridCacheSerializer<T> serializer)
+        public void SetValue(ref BufferChunk buffer, IHybridCacheSerializer<T> serializer)
         {
             _serializer = serializer;
             _buffer = buffer;
             buffer = default; // we're taking over the lifetime; the caller no longer has it!
         }
 
-        public MutableCacheItem(T value, IHybridCacheSerializer<T> serializer, int maxLength)
+        public void SetValue(T value, IHybridCacheSerializer<T> serializer, int maxLength)
         {
             _serializer = serializer;
             var writer = RecyclableArrayBufferWriter<byte>.Create(maxLength);
@@ -34,35 +29,10 @@ public MutableCacheItem(T value, IHybridCacheSerializer<T> serializer, int maxLe
 
         public override bool NeedsEvictionCallback => _buffer.ReturnToPool;
 
-        public override void OnEviction() => Release();
-
-        public override void Release()
+        protected override void OnFinalRelease()
         {
-            var newCount = Interlocked.Decrement(ref _refCount);
-            if (newCount == 0)
-            {
-                DebugOnlyDecrementOutstandingBuffers();
-                _buffer.RecycleIfAppropriate();
-            }
-        }
-
-        public bool TryReserve()
-        {
-            var oldValue = Volatile.Read(ref _refCount);
-            do
-            {
-                if (oldValue is 0 or -1)
-                {
-                    return false; // already burned, or about to roll around back to zero
-                }
-
-                var updated = Interlocked.CompareExchange(ref _refCount, oldValue + 1, oldValue);
-                if (updated == oldValue)
-                {
-                    return true; // we exchanged
-                }
-                oldValue = updated; // we failed, but we have an updated state
-            } while (true);
+            DebugOnlyDecrementOutstandingBuffers();
+            _buffer.RecycleIfAppropriate();
         }
 
         public override bool TryGetValue(out T value)

src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs

@@ -61,16 +61,9 @@ public bool GetOrCreateStampedeState<TState, T>(string key, HybridCacheEntryFlag
             // and for *them* to have updated needs local-cache-write, but since the shared us/them key includes flags,
             // we can skip this if *either* flag is set)
             if ((flags & HybridCacheEntryFlags.DisableLocalCache) == 0 && _localCache.TryGetValue(key, out var untyped)
-                && untyped is CacheItem<T> typed && typed.TryGetValue(out var value))
+                && untyped is CacheItem<T> typed && typed.TryReserve())
             {
-                // set the value against the *current* stampede-state, essentially making it pre-completed; we
-                // can emulate that by using ImmutableCacheItem (if it isn't already), noting that this state
-                // will never be in the dictionary, so this value is never shared with anyone else
-                if (typed is not ImmutableCacheItem<T> immutable)
-                {
-                    immutable = new ImmutableCacheItem<T>(value);
-                }
-                stampedeState.SetResultDirect(immutable);
+                stampedeState.SetResultDirect(typed);
                 return false; // the work has ALREADY been done
             }
 

src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs

@@ -15,7 +15,7 @@ internal abstract class StampedeState
 #endif
     {
         private readonly DefaultHybridCache _cache;
-        private int _activeCallers = 1;
+        private readonly CacheItem _cacheItem;
 
         // because multiple callers can enlist, we need to track when the *last* caller cancels
         // (and keep going until then); that means we need to run with custom cancellation
@@ -26,14 +26,16 @@ internal abstract class StampedeState
         // (both in terms of width and copy-semantics)
         private readonly StampedeKey _key;
         public ref readonly StampedeKey Key => ref _key;
+        protected CacheItem CacheItem => _cacheItem;
 
         /// <summary>
         /// Create a stamped token optionally with shared cancellation support
         /// </summary>
-        protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCanceled)
+        protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CacheItem cacheItem, bool canBeCanceled)
         {
             _cache = cache;
             _key = key;
+            _cacheItem = cacheItem;
             if (canBeCanceled)
             {
                 // if the first (or any) caller can't be cancelled; we'll never get to zero; no point tracking
@@ -50,10 +52,11 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBe
         /// <summary>
         /// Create a stamped token using a fixed cancellation token
         /// </summary>
-        protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CancellationToken token)
+        protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CacheItem cacheItem, CancellationToken token)
         {
             _cache = cache;
             _key = key;
+            _cacheItem = cacheItem;
             SharedToken = token;
         }
 
@@ -71,39 +74,22 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, Cancellati
 
         public abstract void SetCanceled();
 
-        public int DebugCallerCount => Volatile.Read(ref _activeCallers);
+        public int DebugCallerCount => _cacheItem.RefCount;
 
         public abstract Type Type { get; }
 
         public void RemoveCaller()
         {
             // note that TryAddCaller has protections to avoid getting back from zero
-            if (Interlocked.Decrement(ref _activeCallers) == 0)
+            if (_cacheItem.Release())
             {
                 // we're the last to leave; turn off the lights
                 _sharedCancellation?.Cancel();
                 SetCanceled();
             }
         }
 
-        public bool TryAddCaller() // essentially just interlocked-increment, but with a leading zero check and overflow detection
-        {
-            var oldValue = Volatile.Read(ref _activeCallers);
-            do
-            {
-                if (oldValue is 0 or -1)
-                {
-                    return false; // already burned or about to roll around back to zero
-                }
-
-                var updated = Interlocked.CompareExchange(ref _activeCallers, oldValue + 1, oldValue);
-                if (updated == oldValue)
-                {
-                    return true; // we exchanged
-                }
-                oldValue = updated; // we failed, but we have an updated state
-            } while (true);
-        }
+        public bool TryAddCaller() => _cacheItem.TryReserve();
     }
 
     private void RemoveStampedeState(in StampedeKey key)