From be46d16f6ff2daa6632136f7f100c5de89de6809 Mon Sep 17 00:00:00 2001 From: Eirik George Tsarpalis Date: Thu, 29 Aug 2024 21:02:12 +0000 Subject: [PATCH 1/5] 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. --- ...icrosoft.Extensions.DependencyModel.csproj | 4 +- .../src/System.Text.Json.csproj | 5 +- .../Converters/Node/JsonObjectConverter.cs | 14 +++-- ...onObjectExtensionDataSerializationState.cs | 53 +++++++++++++++++++ .../Object/ObjectDefaultConverter.cs | 6 +++ ...ctWithParameterizedConstructorConverter.cs | 3 ++ .../Text/Json/Serialization/ReadStackFrame.cs | 3 ++ .../JsonNode/JsonObjectTests.cs | 30 +++++++++++ 8 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/LargeJsonObjectExtensionDataSerializationState.cs diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj b/src/libraries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj index e135c23a6bb90f..d18ee472a5e828 100644 --- a/src/libraries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj @@ -3,8 +3,8 @@ $(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum) true true - false - 1 + true + 2 Provides abstractions for reading `.deps` files. When a .NET application is compiled, the SDK generates a JSON manifest file (`<ApplicationName>.deps.json`) that contains information about application dependencies. You can use `Microsoft.Extensions.DependencyModel` to read information from this manifest at run time. This is useful when you want to dynamically compile code (for example, using Roslyn Emit API) referencing the same dependencies as your main application. By default, the dependency manifest contains information about the application's target framework and runtime dependencies. Set the PreserveCompilationContext project property to `true` to additionally include information about reference assemblies used during compilation. diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index 7e73fd3b12a50b..c17d1aa4ab1d23 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -8,8 +8,8 @@ CS8969 true true - false - 4 + true + 5 Provides high-performance and low-allocating types that serialize objects to JavaScript Object Notation (JSON) text and deserialize JSON text to objects, with UTF-8 support built-in. Also provides types to read and write JSON text encoded as UTF-8, and to create an in-memory document object model (DOM), that is read-only, for random access of the JSON elements within a structured view of the data. The System.Text.Json library is built-in as part of the shared framework in .NET Runtime. The package can be installed when you need to use it in other target frameworks. @@ -122,6 +122,7 @@ The System.Text.Json library is built-in as part of the shared framework in .NET + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonObjectConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonObjectConverter.cs index 4455e92916eb0b..5e2d754b4a5e3c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonObjectConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonObjectConverter.cs @@ -27,10 +27,18 @@ internal override void ReadElementAndSetProperty( Debug.Assert(obj is JsonObject); JsonObject jObject = (JsonObject)obj; - Debug.Assert(value == null || value is JsonNode); - JsonNode? jNodeValue = value; + if (jObject.Count < LargeJsonObjectExtensionDataSerializationState.LargeObjectThreshold) + { + jObject[propertyName] = value; + } + else + { + LargeJsonObjectExtensionDataSerializationState deserializationState = + state.Current.LargeJsonObjectExtensionDataSerializationState ??= new(jObject); - jObject[propertyName] = jNodeValue; + Debug.Assert(ReferenceEquals(deserializationState.Destination, jObject)); + deserializationState.AddProperty(propertyName, value); + } } public override void Write(Utf8JsonWriter writer, JsonObject? value, JsonSerializerOptions options) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/LargeJsonObjectExtensionDataSerializationState.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/LargeJsonObjectExtensionDataSerializationState.cs new file mode 100644 index 00000000000000..a9f0e7abd4a881 --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/LargeJsonObjectExtensionDataSerializationState.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Text.Json.Nodes; + +namespace System.Text.Json.Serialization.Converters +{ + /// + /// Implements a mitigation for deserializing large JsonObject extension data properties. + /// Extension data properties use replace semantics when duplicate keys are encountered, + /// which is an O(n) operation for JsonObject resulting in O(n^2) total deserialization time. + /// This class mitigates the performance issue by storing the deserialized properties in a + /// temporary dictionary (which has O(1) updates) and copies them to the destination object + /// at the end of deserialization. + /// + internal sealed class LargeJsonObjectExtensionDataSerializationState + { + public const int LargeObjectThreshold = 25; + private readonly Dictionary _tempDictionary; + public JsonObject Destination { get; } + + public LargeJsonObjectExtensionDataSerializationState(JsonObject destination) + { + StringComparer comparer = destination.Options?.PropertyNameCaseInsensitive ?? false + ? StringComparer.OrdinalIgnoreCase + : StringComparer.Ordinal; + + Destination = destination; + _tempDictionary = new(comparer); + } + + /// + /// Stores a deserialized property to the temporary dictionary, using replace semantics. + /// + public void AddProperty(string key, JsonNode? value) + { + _tempDictionary[key] = value; + } + + /// + /// Copies the properties from the temporary dictionary to the destination JsonObject. + /// + public void Complete() + { + // Because we're only appending values to _tempDictionary, this should preserve JSON ordering. + foreach (KeyValuePair kvp in _tempDictionary) + { + Destination[kvp.Key] = kvp.Value; + } + } + } +} diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index d232055a576bbb..d02364834dc8aa 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -254,6 +254,9 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, jsonTypeInfo.UpdateSortedPropertyCache(ref state.Current); } + // Complete any JsonObject extension data deserializations. + state.Current.LargeJsonObjectExtensionDataSerializationState?.Complete(); + return true; } @@ -299,6 +302,9 @@ internal static void PopulatePropertiesFastPath(object obj, JsonTypeInfo jsonTyp { jsonTypeInfo.UpdateSortedPropertyCache(ref state.Current); } + + // Complete any JsonObject extension data deserializations. + state.Current.LargeJsonObjectExtensionDataSerializationState?.Complete(); } internal sealed override bool OnTryWrite( diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index 7c5d6aae1c4051..b8ab2b7b9b44f3 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -267,6 +267,9 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo state.Current.JsonTypeInfo.UpdateSortedParameterCache(ref state.Current); } + // Complete any JsonObject extension data deserializations. + state.Current.LargeJsonObjectExtensionDataSerializationState?.Complete(); + return true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 15a470d145e345..e5b23a9eb7b306 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text.Json.Serialization; +using System.Text.Json.Serialization.Converters; using System.Text.Json.Serialization.Metadata; namespace System.Text.Json @@ -40,6 +41,8 @@ internal struct ReadStackFrame public JsonTypeInfo JsonTypeInfo; public StackFrameObjectState ObjectState; // State tracking the current object. + public LargeJsonObjectExtensionDataSerializationState? LargeJsonObjectExtensionDataSerializationState; + // Current object can contain metadata public bool CanContainMetadata; public MetadataPropertyName LatestMetadataPropertyName; diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs index 377d043f002943..46ea11ca680392 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs @@ -5,7 +5,9 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Text.Json.Serialization; using System.Text.Json.Serialization.Tests; +using System.Text.Json.Tests; using System.Threading.Tasks; using Xunit; @@ -1189,5 +1191,33 @@ public static void ReplaceWith() Assert.Null(jValue.Parent); Assert.Equal("{\"value\":5}", jObject.ToJsonString()); } + + [Theory] + [InlineData(10_000)] + [InlineData(50_000)] + [InlineData(100_000)] + public static void JsonObject_ExtensionData_ManyDuplicatePayloads(int size) + { + // Generate the payload + StringBuilder builder = new StringBuilder(); + builder.Append("{"); + for (int i = 0; i < size; i++) + { + builder.Append($"\"{i}\": 0,"); + builder.Append($"\"{i}\": 0,"); + } + builder.Length--; // strip trailing comma + builder.Append("}"); + + string jsonPayload = builder.ToString(); + ClassWithObjectExtensionData result = JsonSerializer.Deserialize(jsonPayload); + Assert.Equal(size, result.ExtensionData.Count); + } + + class ClassWithObjectExtensionData + { + [JsonExtensionData] + public JsonObject ExtensionData { get; set; } + } } } From c2891d32a0408ce62771120dd7c03bc3dde1fca7 Mon Sep 17 00:00:00 2001 From: "Eric St. John" Date: Mon, 9 Sep 2024 16:56:10 +0000 Subject: [PATCH 2/5] 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. --- .../src/System/IO/Compression/ZipBlocks.cs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index 8bcb5495efe461..96b19871cd734f 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -269,14 +269,12 @@ public static Zip64ExtraField GetAndRemoveZip64Block(List zip64Field._localHeaderOffset = null; zip64Field._startDiskNumber = null; - List markedForDelete = new List(); 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 zip64FieldFound = true; } } + return true; } - } - foreach (ZipGenericExtraField ef in markedForDelete) - extraFields.Remove(ef); + return false; + }); return zip64Field; } public static void RemoveZip64Blocks(List extraFields) { - List markedForDelete = new List(); - 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) From 132fb31922b8c4d5fb11ac6294042d7bb1a4b1d8 Mon Sep 17 00:00:00 2001 From: "Eric St. John" Date: Mon, 9 Sep 2024 16:59:40 +0000 Subject: [PATCH 3/5] 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`. --- .../src/System.IO.Packaging.csproj | 2 + .../src/System/IO/Packaging/Package.cs | 62 ++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj b/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj index fd5fa77c1cea4c..6d38af53ab005b 100644 --- a/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj +++ b/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj @@ -3,6 +3,8 @@ $(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum) true true + true + 1 Provides classes that support storage of multiple data objects in a single container. diff --git a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/Package.cs b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/Package.cs index 80a40418602798..a9f2c85b4f22cc 100644 --- a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/Package.cs +++ b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/Package.cs @@ -401,32 +401,63 @@ public PackagePartCollection GetParts() PackUriHelper.ValidatedPartUri partUri; + var uriComparer = Comparer.Default; + + //Sorting the parts array which takes O(n log n) time. + Array.Sort(parts, Comparer.Create((partA, partB) => uriComparer.Compare((PackUriHelper.ValidatedPartUri)partA.Uri, (PackUriHelper.ValidatedPartUri)partB.Uri))); + //We need this dictionary to detect any collisions that might be present in the //list of parts that was given to us from the underlying physical layer, as more than one //partnames can be mapped to the same normalized part. //Note: We cannot use the _partList member variable, as that gets updated incrementally and so its //not possible to find the collisions using that list. //PackUriHelper.ValidatedPartUri implements the IComparable interface. - Dictionary seenPartUris = new Dictionary(parts.Length); + Dictionary> partDictionary = new Dictionary>(parts.Length); + List partIndex = new List(parts.Length); for (int i = 0; i < parts.Length; i++) { partUri = (PackUriHelper.ValidatedPartUri)parts[i].Uri; - if (seenPartUris.ContainsKey(partUri)) + string normalizedPartName = partUri.NormalizedPartUriString; + + if (partDictionary.ContainsKey(normalizedPartName)) + { throw new FileFormatException(SR.BadPackageFormat); + } else { - // Add the part to the list of URIs that we have already seen - seenPartUris.Add(partUri, parts[i]); + //since we will arive to this line of code after the parts are already sorted + string? precedingPartName = null; + + if (partIndex.Count > 0) + { + precedingPartName = (partIndex[partIndex.Count - 1]); + } + + // Add the part to the dictionary + partDictionary.Add(normalizedPartName, new KeyValuePair(partUri, parts[i])); - if (!_partList.ContainsKey(partUri)) + if (precedingPartName != null + && normalizedPartName.StartsWith(precedingPartName, StringComparison.Ordinal) + && normalizedPartName.Length > precedingPartName.Length + && normalizedPartName[precedingPartName.Length] == PackUriHelper.ForwardSlashChar) { - // Add the part to the _partList if there is no prefix collision - AddIfNoPrefixCollisionDetected(partUri, parts[i]); + //Removing the invalid entry from the _partList. + partDictionary.Remove(normalizedPartName); + + throw new InvalidOperationException(SR.PartNamePrefixExists); } + + //adding entry to partIndex to keep track of last element being added. + //since parts are already sorted, last element in partIndex list will point to preceeding element to the current. + partIndex.Add(partUri.NormalizedPartUriString); } } + + //copying parts from partdictionary to partlist + CopyPartDicitonaryToPartList(partDictionary, partIndex); + _partCollection = new PackagePartCollection(_partList); } return _partCollection; @@ -1173,6 +1204,23 @@ private PackageRelationshipCollection GetRelationshipsHelper(string? filterStrin return new PackageRelationshipCollection(_relationships, filterString); } + private void CopyPartDicitonaryToPartList(Dictionary> partDictionary, List partIndex) + { + //Clearing _partList before copying in new data. Reassigning the variable, assuming the previous object to be garbage collected. + //ideally addition to sortedlist takes O(n) but since we have sorted data and also we defined the size, it will take O(log n) per addition + //total time complexity for this function will be O(n log n) + _partList = new SortedList(partDictionary.Count); + + //Since partIndex is created from a sorted parts array we are sure that partIndex + //will have items in same order + foreach (var id in partIndex) + { + //retrieving object from partDictionary hashtable + var keyValue = partDictionary[id]; + _partList.Add(keyValue.Key, keyValue.Value); + } + } + #endregion Private Methods #region Private Members From 7dc1560dfb4ff946388512ef439fcb44f294b32a Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 9 Sep 2024 17:17:09 +0000 Subject: [PATCH 4/5] Merged PR 41316: Use Marvin32 instead of xxHash32 in COSE hash codes --- .../src/System/HashCodeRandomization.cs | 49 +++++++++---------- .../src/System/Marvin.cs | 21 +++++--- .../System.Security.Cryptography.Cose.csproj | 11 +++-- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/libraries/Common/src/System/HashCodeRandomization.cs b/src/libraries/Common/src/System/HashCodeRandomization.cs index 16cd6cb577c052..a275390df05cbf 100644 --- a/src/libraries/Common/src/System/HashCodeRandomization.cs +++ b/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,6 +17,21 @@ 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 @@ -21,32 +39,11 @@ public static int GetRandomizedOrdinalHashCode(this string value) 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 charSpan = value.AsSpan(); + ReadOnlySpan byteSpan = MemoryMarshal.AsBytes(charSpan); + return Marvin.ComputeHash32(byteSpan, s_seed); #endif } diff --git a/src/libraries/System.Private.CoreLib/src/System/Marvin.cs b/src/libraries/System.Private.CoreLib/src/System/Marvin.cs index 098ebb5260dc9d..37ac05610d9283 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Marvin.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Marvin.cs @@ -2,7 +2,6 @@ // 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; @@ -204,7 +203,7 @@ public static int ComputeHash32(ref byte data, uint count, uint p0, uint p1) else { partialResult |= (uint)Unsafe.ReadUnaligned(ref data); - partialResult = BitOperations.RotateLeft(partialResult, 16); + partialResult = RotateLeft(partialResult, 16); } } @@ -221,21 +220,26 @@ 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; } +#if SYSTEM_PRIVATE_CORELIB + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static uint RotateLeft(uint value, int offset) => + System.Numerics.BitOperations.RotateLeft(value, offset); + public static ulong DefaultSeed { get; } = GenerateSeed(); private static unsafe ulong GenerateSeed() @@ -244,5 +248,10 @@ private static unsafe ulong GenerateSeed() Interop.GetRandomBytes((byte*)&seed, sizeof(ulong)); return seed; } +#else + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static uint RotateLeft(uint value, int offset) => + (value << offset) | (value >> (32 - offset)); +#endif } } diff --git a/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj b/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj index 3ed0bec5488ed5..371d72ab6bc270 100644 --- a/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj +++ b/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj @@ -4,6 +4,8 @@ $(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum) true true + 1 + true Provides support for CBOR Object Signing and Encryption (COSE). @@ -33,13 +35,12 @@ - - + + - - - + + From 378b27e4d8832ccee8c25176092e946ce220a7a1 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 10 Sep 2024 01:39:01 +0000 Subject: [PATCH 5/5] 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` (we do this on all TFMs, replacing the need for the dynamic lookup via `EqualityComparer.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`) --- .../src/MemoryCache.cs | 107 +++++++++++++++--- ...Microsoft.Extensions.Caching.Memory.csproj | 7 ++ .../tests/MemoryCacheSetAndRemoveTests.cs | 15 +++ .../src/System/Marvin.cs | 33 ++++-- 4 files changed, 136 insertions(+), 26 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 24212fbc65c684..ec7888610c944c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/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 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 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 computeEntry // Sort items by expired & priority status DateTime utcNow = UtcNow; - foreach (KeyValuePair 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) /// private sealed class CoherentState { - internal ConcurrentDictionary _entries = new ConcurrentDictionary(); + private readonly ConcurrentDictionary _stringEntries = new ConcurrentDictionary(StringKeyComparer.Instance); + private readonly ConcurrentDictionary _nonStringEntries = new ConcurrentDictionary(); internal long _cacheSize; - private ICollection> 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 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 entry in _stringEntries) + { + yield return entry.Value; + } + foreach (KeyValuePair entry in _nonStringEntries) + { + yield return entry.Value; + } + } + + private ICollection> StringEntriesCollection => _stringEntries; + private ICollection> 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(entry.Key, entry))) + if (entry.Key is string s) + { + if (StringEntriesCollection.Remove(new KeyValuePair(s, entry))) + { + if (options.SizeLimit.HasValue) + { + Interlocked.Add(ref _cacheSize, -entry.Size); + } + entry.InvokeEvictionCallbacks(); + } + } + else if (NonStringEntriesCollection.Remove(new KeyValuePair(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 Instance => EqualityComparer.Default; + } +#else + // otherwise, we need a custom comparer that manually implements Marvin + private sealed class StringKeyComparer : IEqualityComparer, IEqualityComparer + { + private StringKeyComparer() { } + + internal static readonly IEqualityComparer 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 } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj b/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj index 469e6e572536ef..bc5dba097854a7 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj @@ -5,6 +5,9 @@ true true In-memory cache implementation of Microsoft.Extensions.Caching.Memory.IMemoryCache. + true + true + 1 @@ -21,4 +24,8 @@ + + + + diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs index f1002232e4c18f..71738d4e0c3492 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs @@ -794,6 +794,21 @@ public async Task GetOrCreateAsyncFromCacheWithNullKeyThrows() await Assert.ThrowsAsync(async () => await cache.GetOrCreateAsync(null, null)); } + [Fact] + public void MixedKeysUsage() + { + // keys are split internally into 2 separate chunks + var cache = CreateCache(); + var typed = Assert.IsType(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; diff --git a/src/libraries/System.Private.CoreLib/src/System/Marvin.cs b/src/libraries/System.Private.CoreLib/src/System/Marvin.cs index 37ac05610d9283..189240a33f4c5f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Marvin.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Marvin.cs @@ -5,6 +5,12 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +#if SYSTEM_PRIVATE_CORELIB +using static System.Numerics.BitOperations; +#else +using System.Security.Cryptography; +#endif + namespace System { internal static partial class Marvin @@ -235,23 +241,34 @@ private static void Block(ref uint rp0, ref uint rp1) rp1 = p1; } -#if SYSTEM_PRIVATE_CORELIB - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static uint RotateLeft(uint value, int offset) => - System.Numerics.BitOperations.RotateLeft(value, offset); - public static ulong DefaultSeed { get; } = GenerateSeed(); 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; } -#else + +#if !SYSTEM_PRIVATE_CORELIB [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static uint RotateLeft(uint value, int offset) => - (value << offset) | (value >> (32 - offset)); + 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 } }