PR nits

re .dic changes: adds "memoize[s|d]", "mibi", "canceled", and a few similar; happy to revert if disagreeable

Author: mgravell

eng/spellchecking_exclusions.dic

@@ -8,6 +8,8 @@ namespace Microsoft.Extensions.Caching.Hybrid;
 /// </summary>
 public class HybridCacheOptions
 {
+    private const int ShiftBytesToMibiBytes = 20;
+
     /// <summary>
     /// Gets or sets the default global options to be applied to <see cref="HybridCache"/> operations; if options are
     /// specified at the individual call level, the non-null values are merged (with the per-call
@@ -39,6 +41,4 @@ public class HybridCacheOptions
     /// tags do not contain data that should not be visible in metrics systems.
     /// </summary>
     public bool ReportTagMetrics { get; set; }
-
-    private const int ShiftBytesToMibiBytes = 20;
 }

src/Libraries/Microsoft.Extensions.Caching.Hybrid/HybridCacheOptions.cs

@@ -15,13 +15,3 @@ public interface IHybridCacheBuilder
     /// </summary>
     IServiceCollection Services { get; }
 }
-
-internal sealed class HybridCacheBuilder : IHybridCacheBuilder
-{
-    public HybridCacheBuilder(IServiceCollection services)
-    {
-        Services = services;
-    }
-
-    public IServiceCollection Services { get; }
-}

src/Libraries/Microsoft.Extensions.Caching.Hybrid/IHybridCacheBuilder.cs

@@ -8,19 +8,21 @@
 
 namespace Microsoft.Extensions.Caching.Hybrid.Internal;
 
-// used to convey buffer status; like ArraySegment<byte>, but Offset is always
-// zero, and we use the most significant bit (MSB, the sign flag) of the length
-// to track whether or not to recycle this value
+// Used to convey buffer status; like ArraySegment<byte>, but Offset is always
+// zero, and we use the most significant bit of the length (usually the sign flag,
+// but we do not need to support negative length) to track whether or not
+// to recycle this value.
 internal readonly struct BufferChunk
 {
-    private const int MSB = (1 << 31);
+    private const int FlagReturnToPool = (1 << 31);
 
     private readonly int _lengthAndPoolFlag;
+
     public byte[]? Array { get; } // null for default
 
-    public int Length => _lengthAndPoolFlag & ~MSB;
+    public int Length => _lengthAndPoolFlag & ~FlagReturnToPool;
 
-    public bool ReturnToPool => (_lengthAndPoolFlag & MSB) != 0;
+    public bool ReturnToPool => (_lengthAndPoolFlag & FlagReturnToPool) != 0;
 
     public byte[] ToArray()
     {
@@ -33,6 +35,13 @@ public byte[] ToArray()
         var copy = new byte[length];
         Buffer.BlockCopy(Array!, 0, copy, 0, length);
         return copy;
+
+        // Note on nullability of Array; the usage here is that a non-null array
+        // is always provided during construction, so the only null scenario is for default(BufferChunk).
+        // Since the constructor explicitly accesses array.Length, any null array passed to the constructor
+        // will cause an exception, even in release (the Debug.Assert only covers debug) - although in
+        // reality we do not expect this to ever occur (internal type, usage checked, etc). In the case of
+        // default(BufferChunk), we know that Length will be zero, which means we will hit the [] case.
     }
 
     public BufferChunk(byte[] array)
@@ -55,7 +64,7 @@ public BufferChunk(byte[] array, int length, bool returnToPool)
         Debug.Assert(array is not null, "expected valid array input");
         Debug.Assert(length >= 0, "expected valid length");
         Array = array;
-        _lengthAndPoolFlag = length | (returnToPool ? MSB : 0);
+        _lengthAndPoolFlag = length | (returnToPool ? FlagReturnToPool : 0);
         Debug.Assert(ReturnToPool == returnToPool, "return-to-pool not respected");
         Debug.Assert(Length == length, "length not respected");
     }
@@ -71,12 +80,13 @@ internal void RecycleIfAppropriate()
         Debug.Assert(Array is null && !ReturnToPool, "expected clean slate after recycle");
     }
 
+    // get the data as a ROS; for note on null-logic of Array!, see comment in ToArray
     internal ReadOnlySequence<byte> AsSequence() => Length == 0 ? default : new ReadOnlySequence<byte>(Array!, 0, Length);
 
     internal BufferChunk DoNotReturnToPool()
     {
         var copy = this;
-        Unsafe.AsRef(in copy._lengthAndPoolFlag) &= ~MSB;
+        Unsafe.AsRef(in copy._lengthAndPoolFlag) &= ~FlagReturnToPool;
         Debug.Assert(copy.Length == Length, "same length expected");
         Debug.Assert(!copy.ReturnToPool, "do not return to pool");
         return copy;

src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/BufferChunk.cs

@@ -14,13 +14,15 @@ 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,
+        public abstract bool DebugIsImmutable { get; }
+
+        // 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
+        // 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
+        // who incr/decr around their fetch), allowing safe buffer recycling.
 
         internal int RefCount => Volatile.Read(ref _refCount);
 
@@ -36,11 +38,13 @@ internal abstract class CacheItem
 
         public abstract bool TryReserveBuffer(out BufferChunk buffer);
 
-        public abstract bool DebugIsImmutable { get; }
-
-        public bool Release() // returns true ONLY for the final release step
+        /// <summary>
+        /// Signal that the consumer is done with this item (ref-count decr).
+        /// </summary>
+        /// <returns>True if this is the final release.</returns>
+        public bool Release()
         {
-            var newCount = Interlocked.Decrement(ref _refCount);
+            int newCount = Interlocked.Decrement(ref _refCount);
             Debug.Assert(newCount >= 0, "over-release detected");
             if (newCount == 0)
             {
@@ -54,10 +58,10 @@ internal abstract class CacheItem
 
         public bool TryReserve()
         {
-            // this is basically interlocked increment, but with a check against:
+            // This is basically interlocked increment, but with a check against:
             // a) incrementing upwards from zero
             // b) overflowing *back* to zero
-            var oldValue = Volatile.Read(ref _refCount);
+            int oldValue = Volatile.Read(ref _refCount);
             do
             {
                 if (oldValue is 0 or -1)

src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.CacheItem.cs

@@ -12,6 +12,9 @@ namespace Microsoft.Extensions.Caching.Hybrid.Internal;
 
 internal partial class DefaultHybridCache
 {
+    /// <summary>
+    /// Auxiliary API for testing purposes, allowing confirmation of the internal state independent of the public API.
+    /// </summary>
     internal bool DebugTryGetCacheItem(string key, [NotNullWhen(true)] out CacheItem? value)
     {
         if (_localCache.TryGetValue(key, out var untyped) && untyped is CacheItem typed)
@@ -46,38 +49,33 @@ internal void DebugOnlyIncrementOutstandingBuffers()
 
     private partial class MutableCacheItem<T>
     {
-        [Conditional("DEBUG")]
-        internal void DebugOnlyTrackBuffer(DefaultHybridCache cache) => DebugOnlyTrackBufferCore(cache);
-
-        [SuppressMessage("Minor Code Smell", "S4136:Method overloads should be grouped together", Justification = "Conditional partial declaration/usage")]
-        [SuppressMessage("Minor Code Smell", "S3251:Implementations should be provided for \"partial\" methods", Justification = "Intentional debug-only API")]
-        [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "Uses instance state in debug build")]
-        partial void DebugOnlyDecrementOutstandingBuffers();
-
-        [SuppressMessage("Minor Code Smell", "S4136:Method overloads should be grouped together", Justification = "Conditional partial declaration/usage")]
-        [SuppressMessage("Minor Code Smell", "S3251:Implementations should be provided for \"partial\" methods", Justification = "Intentional debug-only API")]
-        [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "Uses instance state in debug build")]
-        [SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "Uses parameter in debug build")]
-        partial void DebugOnlyTrackBufferCore(DefaultHybridCache cache);
-
 #if DEBUG
-        private DefaultHybridCache? _cache; // for buffer-tracking - only enabled in DEBUG
-        partial void DebugOnlyDecrementOutstandingBuffers()
+        private DefaultHybridCache? _cache; // for buffer-tracking - only needed in DEBUG
+#endif
+
+        [Conditional("DEBUG")]
+        [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "Instance state used in debug")]
+        internal void DebugOnlyTrackBuffer(DefaultHybridCache cache)
         {
+#if DEBUG
+            _cache = cache;
             if (_buffer.ReturnToPool)
             {
-                _cache?.DebugOnlyDecrementOutstandingBuffers();
+                _cache?.DebugOnlyIncrementOutstandingBuffers();
             }
+#endif
         }
 
-        partial void DebugOnlyTrackBufferCore(DefaultHybridCache cache)
+        [Conditional("DEBUG")]
+        [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "Instance state used in debug")]
+        private void DebugOnlyDecrementOutstandingBuffers()
         {
-            _cache = cache;
+#if DEBUG
             if (_buffer.ReturnToPool)
             {
-                _cache?.DebugOnlyIncrementOutstandingBuffers();
+                _cache?.DebugOnlyDecrementOutstandingBuffers();
             }
-        }
 #endif
+        }
     }
 }

src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.Debug.cs

@@ -9,17 +9,17 @@ internal partial class DefaultHybridCache
 {
     private sealed class ImmutableCacheItem<T> : CacheItem<T> // used to hold types that do not require defensive copies
     {
-        private T _value = default!; // deferred until SetValue
+        private static ImmutableCacheItem<T>? _sharedDefault;
 
-        public void SetValue(T value) => _value = value;
+        private T _value = default!; // deferred until SetValue
 
-        private static ImmutableCacheItem<T>? _sharedDefault;
+        public override bool DebugIsImmutable => true;
 
         // get a shared instance that passes as "reserved"; doesn't need to be 100% singleton,
         // but we don't want to break the reservation rules either; if we can't reserve: create new
         public static ImmutableCacheItem<T> GetReservedShared()
         {
-            var obj = Volatile.Read(ref _sharedDefault);
+            ImmutableCacheItem<T>? obj = Volatile.Read(ref _sharedDefault);
             if (obj is null || !obj.TryReserve())
             {
                 obj = new();
@@ -30,6 +30,8 @@ public static ImmutableCacheItem<T> GetReservedShared()
             return obj;
         }
 
+        public void SetValue(T value) => _value = value;
+
         public override bool TryGetValue(out T value)
         {
             value = _value;
@@ -41,7 +43,5 @@ public override bool TryReserveBuffer(out BufferChunk buffer)
             buffer = default;
             return false; // we don't have one to reserve!
         }
-
-        public override bool DebugIsImmutable => true;
     }
 }

src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.ImmutableCacheItem.cs

@@ -32,6 +32,7 @@ internal ValueTask<BufferChunk> GetFromL2Async(string key, CancellationToken tok
                 }
 
                 return new(GetValidPayloadSegment(pendingLegacy.Result)); // already complete
+
             case CacheFeatures.BackendCache | CacheFeatures.BackendBuffers: // IBufferWriter<byte>-based
                 var writer = RecyclableArrayBufferWriter<byte>.Create(MaximumPayloadBytes);
                 var cache = Unsafe.As<IBufferDistributedCache>(_backendCache!); // type-checked already

src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.L2.cs

@@ -10,6 +10,10 @@ private sealed partial class MutableCacheItem<T> : CacheItem<T> // used to hold
         private IHybridCacheSerializer<T> _serializer = null!; // deferred until SetValue
         private BufferChunk _buffer;
 
+        public override bool NeedsEvictionCallback => _buffer.ReturnToPool;
+
+        public override bool DebugIsImmutable => false;
+
         public void SetValue(ref BufferChunk buffer, IHybridCacheSerializer<T> serializer)
         {
             _serializer = serializer;
@@ -27,8 +31,6 @@ public void SetValue(T value, IHybridCacheSerializer<T> serializer, int maxLengt
             writer.Dispose(); // no buffers left (we just detached them), but just in case of other logic
         }
 
-        public override bool NeedsEvictionCallback => _buffer.ReturnToPool;
-
         public override bool TryGetValue(out T value)
         {
             // only if we haven't already burned
@@ -62,14 +64,9 @@ public override bool TryReserveBuffer(out BufferChunk buffer)
             return false;
         }
 
-        public override bool DebugIsImmutable => false;
-
         protected override void OnFinalRelease()
         {
-#pragma warning disable S3251 // intentional: Supply an implementation for the partial method, otherwise this call will be ignored.
             DebugOnlyDecrementOutstandingBuffers();
-#pragma warning restore S3251
-
             _buffer.RecycleIfAppropriate();
         }
     }

src/Libraries/Microsoft.Extensions.Caching.Hybrid/Internal/DefaultHybridCache.MutableCacheItem.cs

@@ -3,23 +3,18 @@
 
 using System;
 using System.Collections.Concurrent;
-using System.ComponentModel;
 using System.Reflection;
 using System.Runtime.CompilerServices;
 using Microsoft.Extensions.DependencyInjection;
 
-#if !(NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER)
-using System.Runtime.InteropServices;
-using System.Runtime.Serialization;
-#endif
-
 namespace Microsoft.Extensions.Caching.Hybrid.Internal;
+
 internal partial class DefaultHybridCache
 {
-    // per instance cache of typed serializers; each serializer is a
+    // Per instance cache of typed serializers; each serializer is a
     // IHybridCacheSerializer<T> for the corresponding Type, but we can't
     // know which here - and undesirable to add an artificial non-generic
-    // IHybridCacheSerializer base that serves no other purpose
+    // IHybridCacheSerializer base that serves no other purpose.
     private readonly ConcurrentDictionary<Type, object> _serializers = new();
 
     internal int MaximumPayloadBytes { get; }
@@ -31,8 +26,8 @@ internal IHybridCacheSerializer<T> GetSerializer<T>()
 
         static IHybridCacheSerializer<T> ResolveAndAddSerializer(DefaultHybridCache @this)
         {
-            // it isn't critical that we get only one serializer instance during start-up; what matters
-            // is that we don't get a new serializer instance *every time*
+            // It isn't critical that we get only one serializer instance during start-up; what matters
+            // is that we don't get a new serializer instance *every time*.
             var serializer = @this._services.GetService<IHybridCacheSerializer<T>>();
             if (serializer is null)
             {
@@ -56,66 +51,4 @@ static IHybridCacheSerializer<T> ResolveAndAddSerializer(DefaultHybridCache @thi
             return serializer;
         }
     }
-
-    internal static class ImmutableTypeCache<T> // lazy memoize; T doesn't change per cache instance
-    {
-        // note for blittable types: a pure struct will be a full copy every time - nothing shared to mutate
-        public static readonly bool IsImmutable = (typeof(T).IsValueType && IsBlittable<T>()) || IsTypeImmutable(typeof(T));
-    }
-
-    [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Catch-all failure")]
-    private static bool IsBlittable<T>() // minimize the generic portion
-    {
-#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER
-        return !RuntimeHelpers.IsReferenceOrContainsReferences<T>();
-#else
-        // down-level: only blittable types can be pinned
-        try
-        {
-            // get a typed, zeroed, non-null boxed instance of the appropriate type
-            // (can't use (object)default(T), as that would box to null for nullable types)
-            var obj = FormatterServices.GetUninitializedObject(Nullable.GetUnderlyingType(typeof(T)) ?? typeof(T));
-            GCHandle.Alloc(obj, GCHandleType.Pinned).Free();
-            return true;
-        }
-        catch
-        {
-            return false;
-        }
-#endif
-    }
-
-    [System.Diagnostics.CodeAnalysis.SuppressMessage("Blocker Code Smell", "S2178:Short-circuit logic should be used in boolean contexts",
-        Justification = "Non-short-circuiting intentional to remove unnecessary branch")]
-    private static bool IsTypeImmutable(Type type)
-    {
-        // check for known types
-        if (type == typeof(string))
-        {
-            return true;
-        }
-
-        if (type.IsValueType)
-        {
-            // switch from Foo? to Foo if necessary
-            if (Nullable.GetUnderlyingType(type) is { } nullable)
-            {
-                type = nullable;
-            }
-        }
-
-        if (type.IsValueType || (type.IsClass & type.IsSealed))
-        {
-            // check for [ImmutableObject(true)]; note we're looking at this as a statement about
-            // the overall nullability; for example, a type could contain a private int[] field,
-            // where the field is mutable and the list is mutable; but if the type is annotated:
-            // we're trusting that the API and use-case is such that the type is immutable
-            return type.GetCustomAttribute<ImmutableObjectAttribute>() is { Immutable: true };
-        }
-
-        // don't trust interfaces and non-sealed types; we might have any concrete
-        // type that has different behaviour
-        return false;
-
-    }
 }