From c3dbecfcf2b37240db3356f990bcc9b31c970bf6 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 7 Feb 2025 22:47:54 -0500 Subject: [PATCH 1/2] Address feedback on dense FrozenDictionary optimization Allow it to apply to chars and reduce cost of construction a bit. --- .../Collections/Frozen/FrozenDictionary.cs | 4 +- .../Integer/DenseIntegralFrozenDictionary.cs | 120 ++++++------------ .../Frozen/FrozenFromKnownValuesTests.cs | 31 +++-- 3 files changed, 64 insertions(+), 91 deletions(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs index 410f28e156efa1..e785a77b153278 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs @@ -123,9 +123,9 @@ private static FrozenDictionary CreateFromDictionary if (typeof(TKey).IsValueType && ReferenceEquals(comparer, EqualityComparer.Default)) { #if NET - if (DenseIntegralFrozenDictionary.TryCreate(source, out FrozenDictionary? result)) + if (DenseIntegralFrozenDictionary.CreateIfValid(source) is { } denseResult) { - return result; + return denseResult; } #endif diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs index 926225aadb1391..df30d777e43295 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs @@ -17,63 +17,46 @@ namespace System.Collections.Frozen internal sealed class DenseIntegralFrozenDictionary { /// - /// Maximum allowed ratio of the number of key/value pairs to the range between the minimum and maximum keys. + /// Maximum allowed factor by which the spread between the min and max of keys in the dictionary may exceed the count. /// /// - /// - /// This is dialable. The closer the value gets to 0, the more likely this implementation will be used, - /// and the more memory will be consumed to store the values. The value of 0.1 means that up to 90% of the + /// This is dialable. The larger this value, the more likely this implementation will be used, + /// and the more memory will be consumed to store the values. The value of 10 means that up to 90% of the /// slots in the values array may be unused. - /// - /// - /// As an example, DaysOfWeek's min is 0, its max is 6, and it has 7 values, such that 7 / (6 - 0 + 1) = 1.0; thus - /// with a threshold of 0.1, DaysOfWeek will use this implementation. But SocketError's min is -1, its max is 11004, and - /// it has 47 values, such that 47 / (11004 - (-1) + 1) = 0.004; thus, SocketError will not use this implementation. - /// /// - private const double CountToLengthRatio = 0.1; + private const int LengthToCountFactor = 10; - public static bool TryCreate(Dictionary source, [NotNullWhen(true)] out FrozenDictionary? result) + public static FrozenDictionary? CreateIfValid(Dictionary source) where TKey : notnull { // Int32 and integer types that fit within Int32. This is to minimize difficulty later validating that - // inputs are in range of int: we can always cast everything to Int32 without loss of information. - - if (typeof(TKey) == typeof(byte) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(byte))) - return TryCreate(source, out result); - - if (typeof(TKey) == typeof(sbyte) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(sbyte))) - return TryCreate(source, out result); - - if (typeof(TKey) == typeof(ushort) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(ushort))) - return TryCreate(source, out result); - - if (typeof(TKey) == typeof(short) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(short))) - return TryCreate(source, out result); - - if (typeof(TKey) == typeof(int) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(int))) - return TryCreate(source, out result); - - result = null; - return false; + // inputs are in range of int: we can always cast everything here to Int32 without loss of information. + return + typeof(TKey) == typeof(byte) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(byte)) ? CreateIfValid(source) : + typeof(TKey) == typeof(sbyte) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(sbyte)) ? CreateIfValid(source) : + typeof(TKey) == typeof(ushort) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(ushort)) ? CreateIfValid(source) : + typeof(TKey) == typeof(short) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(short)) ? CreateIfValid(source) : + typeof(TKey) == typeof(char) ? CreateIfValid(source) : + typeof(TKey) == typeof(int) || (typeof(TKey).IsEnum && typeof(TKey).GetEnumUnderlyingType() == typeof(int)) ? CreateIfValid(source) : + null; } - private static bool TryCreate(Dictionary source, [NotNullWhen(true)] out FrozenDictionary? result) + private static FrozenDictionary? CreateIfValid(Dictionary source) where TKey : notnull where TKeyUnderlying : unmanaged, IBinaryInteger { - // Start enumerating the dictionary to ensure it has at least one element. + int count = source.Count; + Dictionary.Enumerator e = source.GetEnumerator(); if (e.MoveNext()) { - // Get that element and treat it as the min and max. Then continue enumerating the remainder - // of the dictionary to count the number of elements and track the full min and max. - int count = 1; + // Get the first element and treat it as the min and max. Then continue enumerating the remainder + // of the dictionary to track the full min and max. Along the way, bail if at any point the length + // exceeds the allowed limit based on the known count. int min = int.CreateTruncating((TKeyUnderlying)(object)e.Current.Key); int max = min; while (e.MoveNext()) { - count++; int key = int.CreateTruncating((TKeyUnderlying)(object)e.Current.Key); if (key < min) { @@ -85,51 +68,26 @@ private static bool TryCreate(Dictionary 0); - if (length <= int.MaxValue && - (double)count / length >= CountToLengthRatio) + if (length <= maxAllowedLength) { - // Create arrays of the keys and values, sorted ascending by key. var keys = new TKey[count]; var values = new TValue[keys.Length]; - int i = 0; - foreach (KeyValuePair entry in source) - { - keys[i] = entry.Key; - values[i] = entry.Value; - i++; - } - if (i != keys.Length) - { - throw new InvalidOperationException(SR.CollectionModifiedDuringEnumeration); - } - - // Sort the values so that we can more easily check for contiguity but also so that - // the keys/values returned from various properties/enumeration are in a predictable order. - Array.Sort(keys, values); - - // Determine whether all of the keys are contiguous starting at 0. - bool isFull = true; - for (i = 0; i < keys.Length; i++) - { - if (int.CreateTruncating((TKeyUnderlying)(object)keys[i]) != i) - { - isFull = false; - break; - } - } - - if (isFull) + if (min == 0 && length == count) { // All of the keys are contiguous starting at 0, so we can use an implementation that // just stores all the values in an array indexed by key. This both provides faster access // and allows the single values array to be used for lookups and for ValuesCore. - result = new WithFullValues(keys, values); + foreach (KeyValuePair entry in source) + { + int index = int.CreateTruncating((TKeyUnderlying)(object)entry.Key); + keys[index] = entry.Key; + values[index] = entry.Value; + } + + return new WithFullValues(keys, values); } else { @@ -137,20 +95,22 @@ private static bool TryCreate(Dictionary[length]; - for (i = 0; i < keys.Length; i++) + int i = 0; + foreach (KeyValuePair entry in source) { - optionalValues[int.CreateTruncating((TKeyUnderlying)(object)keys[i]) - min] = new(values[i], hasValue: true); + keys[i] = entry.Key; + values[i] = entry.Value; + i++; + + optionalValues[int.CreateTruncating((TKeyUnderlying)(object)entry.Key) - min] = new(entry.Value, hasValue: true); } - result = new WithOptionalValues(keys, values, optionalValues, min); + return new WithOptionalValues(keys, values, optionalValues, min); } - - return true; } } - result = null; - return false; + return null; } /// Implementation used when all keys are contiguous starting at 0. diff --git a/src/libraries/System.Collections.Immutable/tests/Frozen/FrozenFromKnownValuesTests.cs b/src/libraries/System.Collections.Immutable/tests/Frozen/FrozenFromKnownValuesTests.cs index 5776a7373bb5fd..dc4e2b9e89dbee 100644 --- a/src/libraries/System.Collections.Immutable/tests/Frozen/FrozenFromKnownValuesTests.cs +++ b/src/libraries/System.Collections.Immutable/tests/Frozen/FrozenFromKnownValuesTests.cs @@ -11,17 +11,28 @@ namespace System.Collections.Frozen.Tests { public class FrozenFromKnownValuesTests { - public static IEnumerable Int32StringData() => - from keys in new int[][] + public static IEnumerable Int32StringData() + { + IEnumerable[] ints = + [ + [0], + [0, 1], + [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10], + [0, 2, 4, 6, 8, 10], + [-1, 0, 2, 4, 6, 8, 10], + Enumerable.Range(42, 100), + Enumerable.Range(-42, 100), + Enumerable.Range(0, 20).Select(i => i * 11), + ]; + + for (int i = 0; i < ints.Length; i++) { - new int[] { 0 }, - new int[] { 0, 1 }, - new int[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }, - new int[] { 0, 2, 4, 6, 8, 10 }, - new int[] { -1, 0, 2, 4, 6, 8, 10 }, - Enumerable.Range(42, 100).ToArray(), + ints[i] = ints[i].OrderBy(_ => Guid.NewGuid()); } - select new object[] { keys.ToDictionary(i => i, i => i.ToString()) }; + + return from keys in ints + select new object[] { keys.ToDictionary(i => i, i => i.ToString()) }; + } public static IEnumerable StringStringData() => from comparer in new[] { StringComparer.Ordinal, StringComparer.OrdinalIgnoreCase } @@ -168,9 +179,11 @@ public void FrozenDictionary_Int32String(Dictionary source) FrozenDictionaryWorker(source.ToDictionary(i => (short)i.Key, i => i.Value)); FrozenDictionaryWorker(source.ToDictionary(i => i.Key, i => i.Value)); FrozenDictionaryWorker(source.ToDictionary(i => (long)i.Key, i => i.Value)); + FrozenDictionaryWorker(source.ToDictionary(i => (DayOfWeek)i.Key, i => i.Value)); FrozenDictionaryWorker(source.ToDictionary(i => (byte)i.Key, i => i.Value)); FrozenDictionaryWorker(source.ToDictionary(i => (ushort)i.Key, i => i.Value)); + FrozenDictionaryWorker(source.ToDictionary(i => (char)i.Key, i => i.Value)); FrozenDictionaryWorker(source.ToDictionary(i => (uint)i.Key, i => i.Value)); FrozenDictionaryWorker(source.ToDictionary(i => (ulong)i.Key, i => i.Value)); From 8b68dda5635737b8ab45527c3ddf4c36ab311a1e Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Sat, 8 Feb 2025 07:38:20 -0500 Subject: [PATCH 2/2] Update src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs Co-authored-by: skyoxZ --- .../Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs index df30d777e43295..6de9d99554260b 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Integer/DenseIntegralFrozenDictionary.cs @@ -68,7 +68,7 @@ internal sealed class DenseIntegralFrozenDictionary } } - int maxAllowedLength = (int)Math.Min((long)count * LengthToCountFactor, int.MaxValue); + long maxAllowedLength = Math.Min((long)count * LengthToCountFactor, Array.MaxLength); long length = (long)max - min + 1; if (length <= maxAllowedLength) {