Microsoft.Extensions.Caching.Memory: use Marvin for keys on down-level TFMs

Note that Microsoft.Extensions.Caching.Memory is deployed OOB via NuGet, and multi-targets including support for netfx and ns2.0

Marvin is the string hashing used in modern .NET (https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Marvin.cs); this change extends this behaviour to `string` keys when used with `MemoryCache` - noting that in many scenarios we *expect* the key to be a `string` (even though other types are allowed)

To do this we:

- ~~add a custom key equality comparer for use with `ConcurrentDictionary<object, CacheEntry>` (we do this on all TFMs, replacing the need for the dynamic lookup via `EqualityComparer<TKey>.Default`)~~
- split the internal dictionary into 2 - one for `string`, one for everything else
- in down-level TFMs only, provide a custom `string` key comparer with `MarvinHash` enabled (local snapshot since not available)

This gives us Marvin everywhere, and Marvin+rehash on netcore TFMs (rehash requires `TKey` === `string`)

Author: Marc Gravell

src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

@@ -2,9 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Collections;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Internal;
@@ -23,7 +25,8 @@ public class MemoryCache : IMemoryCache
         internal readonly ILogger _logger;
 
         private readonly MemoryCacheOptions _options;
-        private readonly ConcurrentDictionary<object, CacheEntry> _entries;
+        private readonly ConcurrentDictionary<string, CacheEntry> _stringKeyEntries;
+        private readonly ConcurrentDictionary<object, CacheEntry> _nonStringKeyEntries;
 
         private long _cacheSize;
         private bool _disposed;
@@ -56,7 +59,8 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor, ILoggerFactory
             _options = optionsAccessor.Value;
             _logger = loggerFactory.CreateLogger<MemoryCache>();
 
-            _entries = new ConcurrentDictionary<object, CacheEntry>();
+            _stringKeyEntries = new ConcurrentDictionary<string, CacheEntry>(StringKeyComparer.Instance);
+            _nonStringKeyEntries = new ConcurrentDictionary<object, CacheEntry>();
 
             if (_options.Clock == null)
             {
@@ -74,12 +78,14 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor, ILoggerFactory
         /// <summary>
         /// Gets the count of the current entries for diagnostic purposes.
         /// </summary>
-        public int Count => _entries.Count;
+        public int Count => _stringKeyEntries.Count + _nonStringKeyEntries.Count;
 
         // internal for testing
         internal long Size { get => Interlocked.Read(ref _cacheSize); }
 
-        private ICollection<KeyValuePair<object, CacheEntry>> EntriesCollection => _entries;
+        private ICollection<KeyValuePair<string, CacheEntry>> StringKeyEntriesCollection => _stringKeyEntries;
+
+        private ICollection<KeyValuePair<object, CacheEntry>> NonStringKeyEntriesCollection => _nonStringKeyEntries;
 
         /// <inheritdoc />
         public ICacheEntry CreateEntry(object key)
@@ -129,7 +135,16 @@ internal void SetEntry(CacheEntry entry)
             // Initialize the last access timestamp at the time the entry is added
             entry.LastAccessed = utcNow;
 
-            if (_entries.TryGetValue(entry.Key, out CacheEntry priorEntry))
+            CacheEntry priorEntry = null;
+            string s = entry.Key as string;
+            if (s != null)
+            {
+                if (_stringKeyEntries.TryGetValue(s, out priorEntry))
+                {
+                    priorEntry.SetExpired(EvictionReason.Replaced);
+                }
+            }
+            else if (_nonStringKeyEntries.TryGetValue(entry.Key, out priorEntry))
             {
                 priorEntry.SetExpired(EvictionReason.Replaced);
             }
@@ -143,12 +158,26 @@ internal void SetEntry(CacheEntry entry)
                 if (priorEntry == null)
                 {
                     // Try to add the new entry if no previous entries exist.
-                    entryAdded = _entries.TryAdd(entry.Key, entry);
+                    if (s != null)
+                    {
+                        entryAdded = _stringKeyEntries.TryAdd(s, entry);
+                    }
+                    else
+                    {
+                        entryAdded = _nonStringKeyEntries.TryAdd(entry.Key, entry);
+                    }
                 }
                 else
                 {
                     // Try to update with the new entry if a previous entries exist.
-                    entryAdded = _entries.TryUpdate(entry.Key, entry, priorEntry);
+                    if (s != null)
+                    {
+                        entryAdded = _stringKeyEntries.TryUpdate(s, entry, priorEntry);
+                    }
+                    else
+                    {
+                        entryAdded = _nonStringKeyEntries.TryUpdate(entry.Key, entry, priorEntry);
+                    }
 
                     if (entryAdded)
                     {
@@ -163,7 +192,14 @@ internal void SetEntry(CacheEntry entry)
                         // The update will fail if the previous entry was removed after retrival.
                         // Adding the new entry will succeed only if no entry has been added since.
                         // This guarantees removing an old entry does not prevent adding a new entry.
-                        entryAdded = _entries.TryAdd(entry.Key, entry);
+                        if (s != null)
+                        {
+                            entryAdded = _stringKeyEntries.TryAdd(s, entry);
+                        }
+                        else
+                        {
+                            entryAdded = _nonStringKeyEntries.TryAdd(entry.Key, entry);
+                        }
                     }
                 }
 
@@ -223,7 +259,18 @@ public bool TryGetValue(object key, out object result)
 
             DateTimeOffset utcNow = _options.Clock.UtcNow;
 
-            if (_entries.TryGetValue(key, out CacheEntry entry))
+            bool found;
+            CacheEntry entry;
+            if (key is string s)
+            {
+                found = _stringKeyEntries.TryGetValue(s, out entry);
+            }
+            else
+            {
+                found = _nonStringKeyEntries.TryGetValue(key, out entry);
+            }
+
+            if (found)
             {
                 // 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.
@@ -262,7 +309,18 @@ public void Remove(object key)
             ValidateCacheKey(key);
 
             CheckDisposed();
-            if (_entries.TryRemove(key, out CacheEntry entry))
+            bool removed;
+            CacheEntry entry;
+            if (key is string s)
+            {
+                removed = _stringKeyEntries.TryRemove(s, out entry);
+            }
+            else
+            {
+                removed = _nonStringKeyEntries.TryRemove(key, out entry);
+            }
+
+            if (removed)
             {
                 if (_options.SizeLimit.HasValue)
                 {
@@ -278,7 +336,17 @@ public void Remove(object key)
 
         private void RemoveEntry(CacheEntry entry)
         {
-            if (EntriesCollection.Remove(new KeyValuePair<object, CacheEntry>(entry.Key, entry)))
+            bool removed;
+            if (entry.Key is string s)
+            {
+                removed = StringKeyEntriesCollection.Remove(new KeyValuePair<string, CacheEntry>(s, entry));
+            }
+            else
+            {
+                removed = NonStringKeyEntriesCollection.Remove(new KeyValuePair<object, CacheEntry>(entry.Key, entry));
+            }
+
+            if (removed)
             {
                 if (_options.SizeLimit.HasValue)
                 {
@@ -317,10 +385,8 @@ private static void ScanForExpiredItems(MemoryCache cache)
         {
             DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow;
 
-            foreach (KeyValuePair<object, CacheEntry> item in cache._entries)
+            foreach (CacheEntry entry in cache.GetCacheEntries())
             {
-                CacheEntry entry = item.Value;
-
                 if (entry.CheckExpired(now))
                 {
                     cache.RemoveEntry(entry);
@@ -388,10 +454,26 @@ private static void OvercapacityCompaction(MemoryCache cache)
         /// ?. Larger objects - estimated by object graph size, inaccurate.
         public void Compact(double percentage)
         {
-            int removalCountTarget = (int)(_entries.Count * percentage);
+            int removalCountTarget = (int)(Count * percentage);
             Compact(removalCountTarget, _ => 1);
         }
 
+        private IEnumerable<CacheEntry> GetCacheEntries()
+        {
+            // note this mimics the outgoing code in that we don't just access
+            // .Values, which has additional overheads; this is only used for rare
+            // calls - compaction, clear, etc - so the additional overhead of a
+            // generated enumerator is not alarming
+            foreach (KeyValuePair<string, CacheEntry> item in _stringKeyEntries)
+            {
+                yield return item.Value;
+            }
+            foreach (KeyValuePair<object, CacheEntry> item in _nonStringKeyEntries)
+            {
+                yield return item.Value;
+            }
+        }
+
         private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntrySize)
         {
             var entriesToRemove = new List<CacheEntry>();
@@ -403,9 +485,8 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry
 
             // Sort items by expired & priority status
             DateTimeOffset now = _options.Clock.UtcNow;
-            foreach (KeyValuePair<object, CacheEntry> item in _entries)
+            foreach (CacheEntry entry in GetCacheEntries())
             {
-                CacheEntry entry = item.Value;
                 if (entry.CheckExpired(now))
                 {
                     entriesToRemove.Add(entry);
@@ -526,5 +607,34 @@ private static void ValidateCacheKey(object key)
 
             static void Throw() => throw new ArgumentNullException(nameof(key));
         }
+
+#if NETCOREAPP
+        // on .NET Core, the inbuilt comparer has Marvin built in; no need to intercept
+        private static class StringKeyComparer
+        {
+            internal static IEqualityComparer<string> Instance => EqualityComparer<string>.Default;
+        }
+#else
+        // otherwise, we need a custom comparer that manually implements Marvin
+        private sealed class StringKeyComparer : IEqualityComparer<string>, IEqualityComparer
+        {
+            private StringKeyComparer() { }
+
+            internal static readonly IEqualityComparer<string> Instance = new StringKeyComparer();
+
+            // special-case string keys and use Marvin hashing
+            public int GetHashCode(string? s) => s is null ? 0
+                : Marvin.ComputeHash32(MemoryMarshal.AsBytes(s.AsSpan()), Marvin.DefaultSeed);
+
+            public bool Equals(string? x, string? y)
+                => string.Equals(x, y);
+
+            bool IEqualityComparer.Equals(object x, object y)
+                => object.Equals(x, y);
+
+            int IEqualityComparer.GetHashCode(object obj)
+                => obj is string s ? Marvin.ComputeHash32(MemoryMarshal.AsBytes(s.AsSpan()), Marvin.DefaultSeed) : 0;
+        }
+#endif
     }
 }

src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj

@@ -4,7 +4,9 @@
     <TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
     <EnableDefaultItems>true</EnableDefaultItems>
     <PackageDescription>In-memory cache implementation of Microsoft.Extensions.Caching.Memory.IMemoryCache.</PackageDescription>
-    <ServicingVersion>1</ServicingVersion>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>2</ServicingVersion>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
 
   <ItemGroup>
@@ -15,4 +17,8 @@
     <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\src\Microsoft.Extensions.Primitives.csproj" />
   </ItemGroup>
 
+  <ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netstandard2.1'))">
+    <Compile Include="$(CoreLibSharedDir)System\Marvin.cs" Link="Common\System\Marvin.cs" />
+  </ItemGroup>
+
 </Project>

src/libraries/System.Private.CoreLib/src/System/Marvin.cs

@@ -2,10 +2,15 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics;
-using System.Numerics;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
+
+#if SYSTEM_PRIVATE_CORELIB
 using Internal.Runtime.CompilerServices;
+using static System.Numerics.BitOperations;
+#else
+using System.Security.Cryptography;
+#endif
 
 namespace System
 {
@@ -205,7 +210,7 @@ public static int ComputeHash32(ref byte data, uint count, uint p0, uint p1)
                 else
                 {
                     partialResult |= (uint)Unsafe.ReadUnaligned<ushort>(ref data);
-                    partialResult = BitOperations.RotateLeft(partialResult, 16);
+                    partialResult = RotateLeft(partialResult, 16);
                 }
             }
 
@@ -222,16 +227,16 @@ private static void Block(ref uint rp0, ref uint rp1)
             uint p1 = rp1;
 
             p1 ^= p0;
-            p0 = BitOperations.RotateLeft(p0, 20);
+            p0 = RotateLeft(p0, 20);
 
             p0 += p1;
-            p1 = BitOperations.RotateLeft(p1, 9);
+            p1 = RotateLeft(p1, 9);
 
             p1 ^= p0;
-            p0 = BitOperations.RotateLeft(p0, 27);
+            p0 = RotateLeft(p0, 27);
 
             p0 += p1;
-            p1 = BitOperations.RotateLeft(p1, 19);
+            p1 = RotateLeft(p1, 19);
 
             rp0 = p0;
             rp1 = p1;
@@ -242,8 +247,29 @@ private static void Block(ref uint rp0, ref uint rp1)
         private static unsafe ulong GenerateSeed()
         {
             ulong seed;
+#if SYSTEM_PRIVATE_CORELIB
             Interop.GetRandomBytes((byte*)&seed, sizeof(ulong));
+#else
+            byte[] seedBytes = new byte[sizeof(ulong)];
+            using (RandomNumberGenerator rng = RandomNumberGenerator.Create())
+            {
+                rng.GetBytes(seedBytes);
+                fixed (byte* b = seedBytes)
+                {
+                    seed = *(ulong*)b;
+                }
+            }
+#endif
             return seed;
         }
+
+#if !SYSTEM_PRIVATE_CORELIB
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private static uint RotateLeft(uint value, int shift)
+        {
+            // This is expected to be optimized into a single rol (or ror with negated shift value) instruction
+            return (value << shift) | (value >> (32 - shift));
+        }
+#endif
     }
 }