PR nits

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

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

@@ -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/IHybridCacheBuilder.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/Internal/BufferChunk.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/DefaultHybridCache.CacheItem.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.Debug.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.ImmutableCacheItem.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.L2.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.MutableCacheItem.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.Serialization.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;
-
- }
 }