Merging internal commits for release/8.0 (#108679)

* Fix performance issue when deserializing large payloads in JsonObject extension data.

Bug fix to address performance issues when deserializing large payloads in `JsonObject` extension data.

Mitigates performance issues by optimizing the deserialization process for large `JsonObject` extension data properties.
- Added `LargeJsonObjectExtensionDataSerializationState` class in `System.Text.Json.Serialization.Converters` to handle large payloads efficiently.
- Updated `JsonObjectConverter` to use the new state class for large objects.
- Modified `ObjectDefaultConverter`, `ObjectWithParameterizedConstructorConverter`, and `ReadStackFrame` to complete deserialization using the new state class.
- Added tests in `JsonObjectTests` to validate the performance improvements with large payloads.

* ZipArchive: Improve performance of removing extra fields

This improves the performance of removing extra fields in the ZipArchive by optimizing the removal process.

`src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs`: Replaced multiple iterations and list operations with a single `RemoveAll` call to enhance efficiency.

* Packaging: Port fix for GetParts from .NETFramework

This ports a fix for the `GetParts` method to improve part URI handling and collision detection.

`src/libraries/System.IO.Packaging/src/System/IO/Packaging/Package.cs`: Added sorting of parts array to ensure proper order, introduced a dictionary and list to track parts and detect collisions, and implemented a new method `CopyPartDictionaryToPartList` to copy parts to `_partList`.

* Merged PR 41316: Use Marvin32 instead of xxHash32 in COSE hash codes

* 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`)

---------

Co-authored-by: Mirroring <[email protected]>
Co-authored-by: Eirik George Tsarpalis <[email protected]>
Co-authored-by: Eric St. John <[email protected]>
Co-authored-by: Jeremy Barton <[email protected]>
Co-authored-by: Marc Gravell <[email protected]>

Author: 6 people

src/libraries/Common/src/System/HashCodeRandomization.cs

@@ -1,6 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Runtime.InteropServices;
+using System.Security.Cryptography;
+
 namespace System
 {
     // Contains helpers for calculating randomized hash codes of common types.
@@ -14,39 +17,33 @@ namespace System
     // rather than a global seed for the entire AppDomain.
     internal static class HashCodeRandomization
     {
+#if !NET
+        private static readonly ulong s_seed = GenerateSeed();
+
+        private static ulong GenerateSeed()
+        {
+            using (RandomNumberGenerator rng = RandomNumberGenerator.Create())
+            {
+                byte[] rand = new byte[sizeof(ulong)];
+                rng.GetBytes(rand);
+
+                return BitConverter.ToUInt64(rand, 0);
+            }
+        }
+#endif
+
         public static int GetRandomizedOrdinalHashCode(this string value)
         {
 #if NETCOREAPP
             // In .NET Core, string hash codes are already randomized.
 
             return value.GetHashCode();
 #else
-            // Downlevel, we need to perform randomization ourselves. There's still
-            // the potential for limited collisions ("Hello!" and "Hello!\0"), but
-            // this shouldn't be a problem in practice. If we need to address it,
-            // we can mix the string length into the accumulator before running the
-            // string contents through.
-            //
-            // We'll pull out pairs of chars and write 32 bits at a time.
-
-            HashCode hashCode = default;
-            int pair = 0;
-            for (int i = 0; i < value.Length; i++)
-            {
-                int ch = value[i];
-                if ((i & 1) == 0)
-                {
-                    pair = ch << 16; // first member of pair
-                }
-                else
-                {
-                    pair |= ch; // second member of pair
-                    hashCode.Add(pair); // write pair as single unit
-                    pair = 0;
-                }
-            }
-            hashCode.Add(pair); // flush any leftover data (could be 0 or 1 chars)
-            return hashCode.ToHashCode();
+            // Downlevel, we need to perform randomization ourselves.
+
+            ReadOnlySpan<char> charSpan = value.AsSpan();
+            ReadOnlySpan<byte> byteSpan = MemoryMarshal.AsBytes(charSpan);
+            return Marvin.ComputeHash32(byteSpan, s_seed);
 #endif
         }
 

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

@@ -2,10 +2,12 @@
 // 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.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
@@ -126,7 +128,7 @@ internal void SetEntry(CacheEntry entry)
             entry.LastAccessed = utcNow;
 
             CoherentState coherentState = _coherentState; // Clear() can update the reference in the meantime
-            if (coherentState._entries.TryGetValue(entry.Key, out CacheEntry? priorEntry))
+            if (coherentState.TryGetValue(entry.Key, out CacheEntry? priorEntry))
             {
                 priorEntry.SetExpired(EvictionReason.Replaced);
             }
@@ -145,12 +147,12 @@ internal void SetEntry(CacheEntry entry)
                 if (priorEntry == null)
                 {
                     // Try to add the new entry if no previous entries exist.
-                    entryAdded = coherentState._entries.TryAdd(entry.Key, entry);
+                    entryAdded = coherentState.TryAdd(entry.Key, entry);
                 }
                 else
                 {
                     // Try to update with the new entry if a previous entries exist.
-                    entryAdded = coherentState._entries.TryUpdate(entry.Key, entry, priorEntry);
+                    entryAdded = coherentState.TryUpdate(entry.Key, entry, priorEntry);
 
                     if (entryAdded)
                     {
@@ -165,7 +167,7 @@ internal void SetEntry(CacheEntry entry)
                         // The update will fail if the previous entry was removed after retrieval.
                         // 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 = coherentState._entries.TryAdd(entry.Key, entry);
+                        entryAdded = coherentState.TryAdd(entry.Key, entry);
                     }
                 }
 
@@ -210,7 +212,7 @@ public bool TryGetValue(object key, out object? result)
             DateTime utcNow = UtcNow;
 
             CoherentState coherentState = _coherentState; // Clear() can update the reference in the meantime
-            if (coherentState._entries.TryGetValue(key, out CacheEntry? tmp))
+            if (coherentState.TryGetValue(key, out CacheEntry? tmp))
             {
                 CacheEntry entry = tmp;
                 // Check if expired due to expiration tokens, timers, etc. and if so, remove it.
@@ -269,7 +271,8 @@ public void Remove(object key)
             CheckDisposed();
 
             CoherentState coherentState = _coherentState; // Clear() can update the reference in the meantime
-            if (coherentState._entries.TryRemove(key, out CacheEntry? entry))
+
+            if (coherentState.TryRemove(key, out CacheEntry? entry))
             {
                 if (_options.HasSizeLimit)
                 {
@@ -291,10 +294,10 @@ public void Clear()
             CheckDisposed();
 
             CoherentState oldState = Interlocked.Exchange(ref _coherentState, new CoherentState());
-            foreach (KeyValuePair<object, CacheEntry> entry in oldState._entries)
+            foreach (CacheEntry entry in oldState.GetAllValues())
             {
-                entry.Value.SetExpired(EvictionReason.Removed);
-                entry.Value.InvokeEvictionCallbacks();
+                entry.SetExpired(EvictionReason.Removed);
+                entry.InvokeEvictionCallbacks();
             }
         }
 
@@ -415,10 +418,9 @@ private void ScanForExpiredItems()
             DateTime utcNow = _lastExpirationScan = UtcNow;
 
             CoherentState coherentState = _coherentState; // Clear() can update the reference in the meantime
-            foreach (KeyValuePair<object, CacheEntry> item in coherentState._entries)
-            {
-                CacheEntry entry = item.Value;
 
+            foreach (CacheEntry entry in coherentState.GetAllValues())
+            {
                 if (entry.CheckExpired(utcNow))
                 {
                     coherentState.RemoveEntry(entry, _options);
@@ -516,9 +518,8 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry
 
             // Sort items by expired & priority status
             DateTime utcNow = UtcNow;
-            foreach (KeyValuePair<object, CacheEntry> item in coherentState._entries)
+            foreach (CacheEntry entry in coherentState.GetAllValues())
             {
-                CacheEntry entry = item.Value;
                 if (entry.CheckExpired(utcNow))
                 {
                     entriesToRemove.Add(entry);
@@ -645,18 +646,59 @@ private static void ValidateCacheKey(object key)
         /// </summary>
         private sealed class CoherentState
         {
-            internal ConcurrentDictionary<object, CacheEntry> _entries = new ConcurrentDictionary<object, CacheEntry>();
+            private readonly ConcurrentDictionary<string, CacheEntry> _stringEntries = new ConcurrentDictionary<string, CacheEntry>(StringKeyComparer.Instance);
+            private readonly ConcurrentDictionary<object, CacheEntry> _nonStringEntries = new ConcurrentDictionary<object, CacheEntry>();
             internal long _cacheSize;
 
-            private ICollection<KeyValuePair<object, CacheEntry>> EntriesCollection => _entries;
+            internal bool TryGetValue(object key, [NotNullWhen(true)] out CacheEntry? entry)
+                => key is string s ? _stringEntries.TryGetValue(s, out entry) : _nonStringEntries.TryGetValue(key, out entry);
+
+            internal bool TryRemove(object key, [NotNullWhen(true)] out CacheEntry? entry)
+                => key is string s ? _stringEntries.TryRemove(s, out entry) : _nonStringEntries.TryRemove(key, out entry);
+
+            internal bool TryAdd(object key, CacheEntry entry)
+                => key is string s ? _stringEntries.TryAdd(s, entry) : _nonStringEntries.TryAdd(key, entry);
+
+            internal bool TryUpdate(object key, CacheEntry entry, CacheEntry comparison)
+                => key is string s ? _stringEntries.TryUpdate(s, entry, comparison) : _nonStringEntries.TryUpdate(key, entry, comparison);
+
+            public IEnumerable<CacheEntry> GetAllValues()
+            {
+                // 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> entry in _stringEntries)
+                {
+                    yield return entry.Value;
+                }
+                foreach (KeyValuePair<object, CacheEntry> entry in _nonStringEntries)
+                {
+                    yield return entry.Value;
+                }
+            }
+
+            private ICollection<KeyValuePair<string, CacheEntry>> StringEntriesCollection => _stringEntries;
+            private ICollection<KeyValuePair<object, CacheEntry>> NonStringEntriesCollection => _nonStringEntries;
 
-            internal int Count => _entries.Count;
+            internal int Count => _stringEntries.Count + _nonStringEntries.Count;
 
             internal long Size => Volatile.Read(ref _cacheSize);
 
             internal void RemoveEntry(CacheEntry entry, MemoryCacheOptions options)
             {
-                if (EntriesCollection.Remove(new KeyValuePair<object, CacheEntry>(entry.Key, entry)))
+                if (entry.Key is string s)
+                {
+                    if (StringEntriesCollection.Remove(new KeyValuePair<string, CacheEntry>(s, entry)))
+                    {
+                        if (options.SizeLimit.HasValue)
+                        {
+                            Interlocked.Add(ref _cacheSize, -entry.Size);
+                        }
+                        entry.InvokeEvictionCallbacks();
+                    }
+                }
+                else if (NonStringEntriesCollection.Remove(new KeyValuePair<object, CacheEntry>(entry.Key, entry)))
                 {
                     if (options.SizeLimit.HasValue)
                     {
@@ -665,6 +707,35 @@ internal void RemoveEntry(CacheEntry entry, MemoryCacheOptions options)
                     entry.InvokeEvictionCallbacks();
                 }
             }
+
+#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 ? GetHashCode(s) : 0;
+            }
+#endif
         }
     }
 }

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

@@ -7,6 +7,9 @@
     <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
     <ServicingVersion>1</ServicingVersion>
     <PackageDescription>In-memory cache implementation of Microsoft.Extensions.Caching.Memory.IMemoryCache.</PackageDescription>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
   </PropertyGroup>
 
   <ItemGroup>
@@ -23,4 +26,8 @@
     <PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" />
   </ItemGroup>
 
+  <ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netstandard2.1'))">
+    <Compile Include="$(CoreLibSharedDir)System\Marvin.cs" Link="Common\System\Marvin.cs" />
+  </ItemGroup>
+
 </Project>

src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs

@@ -794,6 +794,21 @@ public async Task GetOrCreateAsyncFromCacheWithNullKeyThrows()
             await Assert.ThrowsAsync<ArgumentNullException>(async () => await cache.GetOrCreateAsync<object>(null, null));
         }
 
+        [Fact]
+        public void MixedKeysUsage()
+        {
+            // keys are split internally into 2 separate chunks
+            var cache = CreateCache();
+            var typed = Assert.IsType<MemoryCache>(cache);
+            object key0 = 123.45M, key1 = "123.45";
+            cache.Set(key0, "string value");
+            cache.Set(key1, "decimal value");
+
+            Assert.Equal(2, typed.Count);
+            Assert.Equal("string value", cache.Get(key0));
+            Assert.Equal("decimal value", cache.Get(key1));
+        }
+
         private class TestKey
         {
             public override bool Equals(object obj) => true;

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs

@@ -269,14 +269,12 @@ public static Zip64ExtraField GetAndRemoveZip64Block(List<ZipGenericExtraField>
             zip64Field._localHeaderOffset = null;
             zip64Field._startDiskNumber = null;
 
-            List<ZipGenericExtraField> markedForDelete = new List<ZipGenericExtraField>();
             bool zip64FieldFound = false;
 
-            foreach (ZipGenericExtraField ef in extraFields)
+            extraFields.RemoveAll(ef =>
             {
                 if (ef.Tag == TagConstant)
                 {
-                    markedForDelete.Add(ef);
                     if (!zip64FieldFound)
                     {
                         if (TryGetZip64BlockFromGenericExtraField(ef, readUncompressedSize, readCompressedSize,
@@ -285,24 +283,18 @@ public static Zip64ExtraField GetAndRemoveZip64Block(List<ZipGenericExtraField>
                             zip64FieldFound = true;
                         }
                     }
+                    return true;
                 }
-            }
 
-            foreach (ZipGenericExtraField ef in markedForDelete)
-                extraFields.Remove(ef);
+                return false;
+            });
 
             return zip64Field;
         }
 
         public static void RemoveZip64Blocks(List<ZipGenericExtraField> extraFields)
         {
-            List<ZipGenericExtraField> markedForDelete = new List<ZipGenericExtraField>();
-            foreach (ZipGenericExtraField field in extraFields)
-                if (field.Tag == TagConstant)
-                    markedForDelete.Add(field);
-
-            foreach (ZipGenericExtraField field in markedForDelete)
-                extraFields.Remove(field);
+            extraFields.RemoveAll(field => field.Tag == TagConstant);
         }
 
         public void WriteBlock(Stream stream)

src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj

@@ -3,6 +3,8 @@
     <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <IsPackable>true</IsPackable>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
     <PackageDescription>Provides classes that support storage of multiple data objects in a single container.</PackageDescription>
   </PropertyGroup>