From 8b168c1dafa239c63a1e88729393ce6fe703d35e Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 4 Apr 2023 11:02:07 -0400 Subject: [PATCH 1/2] Add back IComparable-based optimization to FrozenDictionary/Set We previously had an optimization in FrozenDictionary/Set that special-cased a small number of value types when the default comparer is used... if that type was IComparable, we would sort the types, which would then a) enable us to immediately reject items larger than the known max, and b) enable us to stop searching once we hit an item larger than the one for which we were searching (which then implicitly also immediately rules out items smaller than the known min). We removed that optimization in general because some prominent IComparable implementations don't always work, in particular container types like ValueTuple that implement IComparable but then it's only functional if the contained types are also comparable. This commit puts it back for an allow-list of types. --- .../src/System.Collections.Immutable.csproj | 2 + .../System/Collections/Frozen/Constants.cs | 36 +++++++++ .../Collections/Frozen/FrozenDictionary.cs | 13 +++- .../System/Collections/Frozen/FrozenSet.cs | 12 +++ ...mallValueTypeComparableFrozenDictionary.cs | 70 +++++++++++++++++ .../SmallValueTypeComparableFrozenSet.cs | 76 +++++++++++++++++++ 6 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs create mode 100644 src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs diff --git a/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj b/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj index 5613158ff7855f..64b075b1503902 100644 --- a/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj +++ b/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj @@ -29,6 +29,8 @@ The System.Collections.Immutable library is built-in as part of the shared frame + + diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs index b65f051f6b9920..995958e63db1ca 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Text; + namespace System.Collections.Frozen { /// @@ -28,5 +30,39 @@ internal static class Constants /// hashing ValueTypeDefaultComparerFrozenDictionary/Set. /// public const int MaxItemsInSmallValueTypeFrozenCollection = 10; + + /// + /// Whether the is known to implement safely and efficiently, + /// such that its comparison operations should be used in searching for types in small collections. + /// + /// + /// This does not automatically return true for any type that implements . + /// Doing so leads to problems for container types (e.g. ValueTuple{T1, T2}) where the + /// container implements to delegate to its contained items' implementation + /// but those then don't provide such support. + /// + public static bool IsKnownComparable() => + // This list covers all of the IComparable value types in Corelib that aren't containers (like ValueTuple). + typeof(T).IsPrimitive || + typeof(T) == typeof(decimal) || + typeof(T) == typeof(TimeSpan) || + typeof(T) == typeof(DateTime) || + typeof(T) == typeof(DateTimeOffset) || + typeof(T) == typeof(Guid) || +#if NETCOREAPP3_0_OR_GREATER + typeof(T) == typeof(Rune) || +#endif +#if NET5_0_OR_GREATER + typeof(T) == typeof(Half) || +#endif +#if NET6_0_OR_GREATER + typeof(T) == typeof(DateOnly) || + typeof(T) == typeof(TimeOnly) || +#endif +#if NET7_0_OR_GREATER + typeof(T) == typeof(Int128) || + typeof(T) == typeof(UInt128) || +#endif + typeof(T).IsEnum; } } 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 eca5a39bbb5919..a1e5016027f4f2 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 @@ -6,7 +6,6 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; -using System.Numerics; using System.Runtime.CompilerServices; namespace System.Collections.Frozen @@ -202,14 +201,26 @@ private static FrozenDictionary ChooseImplementationOptimizedForRe { if (source.Count <= Constants.MaxItemsInSmallValueTypeFrozenCollection) { + // If the key is a something we know we can efficiently compare, use a specialized implementation + // that will enable quickly ruling out values outside of the range of keys stored. + if (Constants.IsKnownComparable()) + { + return (FrozenDictionary)(object)new SmallValueTypeComparableFrozenDictionary(source); + } + + // Otherwise, use an implementation optimized for a small number of value types using the default comparer. return (FrozenDictionary)(object)new SmallValueTypeDefaultComparerFrozenDictionary(source); } + // Use a hash-based implementation. + + // For Int32 keys, we can reuse the key storage as the hash storage, saving on space and extra indirection. if (typeof(TKey) == typeof(int)) { return (FrozenDictionary)(object)new Int32FrozenDictionary((Dictionary)(object)source); } + // Fallback to an implementation usable with any value type and the default comparer. return new ValueTypeDefaultComparerFrozenDictionary(source); } } diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs index 69638e8acd1065..df867322e3df7e 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs @@ -133,14 +133,26 @@ private static FrozenSet ChooseImplementationOptimizedForReading(HashSet()) + { + return (FrozenSet)(object)new SmallValueTypeComparableFrozenSet(source); + } + + // Otherwise, use an implementation optimized for a small number of value types using the default comparer. return (FrozenSet)(object)new SmallValueTypeDefaultComparerFrozenSet(source); } + // Use a hash-based implementation. + + // For Int32 values, we can reuse the item storage as the hash storage, saving on space and extra indirection. if (typeof(T) == typeof(int)) { return (FrozenSet)(object)new Int32FrozenSet((HashSet)(object)source); } + // Fallback to an implementation usable with any value type and the default comparer. return new ValueTypeDefaultComparerFrozenSet(source); } } diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs new file mode 100644 index 00000000000000..8da882f7bda286 --- /dev/null +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs @@ -0,0 +1,70 @@ +// 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.Diagnostics; +using System.Linq; +using System.Runtime.CompilerServices; + +namespace System.Collections.Frozen +{ + /// Provides a frozen dictionary to use when the key is a value type, the default comparer is used, and the item count is small. + /// + /// While not constrained in this manner, the must be an . + /// This implementation is only used for a set of types that have a known-good implementation; it's not + /// used for an as we can't know for sure whether it's valid, e.g. if the TKey is a ValueTuple`2, it itself + /// is comparable, but it's items might not be such that trying to compare it will result in exception. + /// + internal sealed class SmallValueTypeComparableFrozenDictionary : FrozenDictionary + where TKey : notnull + { + private readonly TKey[] _keys; + private readonly TValue[] _values; + private readonly TKey _max; + + internal SmallValueTypeComparableFrozenDictionary(Dictionary source) : base(EqualityComparer.Default) + { + Debug.Assert(default(TKey) is IComparable); + Debug.Assert(default(TKey) is not null); + Debug.Assert(typeof(TKey).IsValueType); + + Debug.Assert(source.Count != 0); + Debug.Assert(ReferenceEquals(source.Comparer, EqualityComparer.Default)); + + _keys = source.Keys.ToArray(); + _values = source.Values.ToArray(); + + Array.Sort(_keys, _values); + _max = _keys[_keys.Length - 1]; + } + + private protected override TKey[] KeysCore => _keys; + private protected override TValue[] ValuesCore => _values; + private protected override Enumerator GetEnumeratorCore() => new Enumerator(_keys, _values); + private protected override int CountCore => _keys.Length; + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private protected override ref readonly TValue GetValueRefOrNullRefCore(TKey key) + { + if (Comparer.Default.Compare(key, _max) <= 0) + { + TKey[] keys = _keys; + for (int i = 0; i < keys.Length; i++) + { + int c = Comparer.Default.Compare(key, keys[i]); + if (c <= 0) + { + if (c == 0) + { + return ref _values[i]; + } + + break; + } + } + } + + return ref Unsafe.NullRef(); + } + } +} diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs new file mode 100644 index 00000000000000..41a103da59b67d --- /dev/null +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs @@ -0,0 +1,76 @@ +// 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.Collections.Immutable; +using System.Diagnostics; +using System.Linq; + +namespace System.Collections.Frozen +{ + /// Provides a frozen set to use when the item is a value type, the default comparer is used, and the item count is small. + /// + /// While not constrained in this manner, the must be an . + /// This implementation is only used for a set of types that have a known-good implementation; it's not + /// used for an as we can't know for sure whether it's valid, e.g. if the T is a ValueTuple`2, it itself + /// is comparable, but it's items might not be such that trying to compare it will result in exception. + /// + internal sealed class SmallValueTypeComparableFrozenSet : FrozenSetInternalBase.GSW> + { + private readonly T[] _items; + private readonly T _max; + + internal SmallValueTypeComparableFrozenSet(HashSet source) : base(EqualityComparer.Default) + { + Debug.Assert(default(T) is IComparable); + Debug.Assert(default(T) is not null); + Debug.Assert(typeof(T).IsValueType); + + Debug.Assert(source.Count != 0); + Debug.Assert(ReferenceEquals(source.Comparer, EqualityComparer.Default)); + + _items = source.ToArray(); + + Array.Sort(_items); + _max = _items[_items.Length - 1]; + } + + private protected override T[] ItemsCore => _items; + private protected override Enumerator GetEnumeratorCore() => new Enumerator(_items); + private protected override int CountCore => _items.Length; + + private protected override int FindItemIndex(T item) + { + if (Comparer.Default.Compare(item, _max) <= 0) + { + T[] items = _items; + for (int i = 0; i < items.Length; i++) + { + int c = Comparer.Default.Compare(item, items[i]); + if (c <= 0) + { + if (c == 0) + { + return i; + } + + break; + } + } + } + + return -1; + } + + internal struct GSW : IGenericSpecializedWrapper + { + private SmallValueTypeComparableFrozenSet _set; + public void Store(FrozenSet set) => _set = (SmallValueTypeComparableFrozenSet)set; + + public int Count => _set.Count; + public IEqualityComparer Comparer => _set.Comparer; + public int FindItemIndex(T item) => _set.FindItemIndex(item); + public Enumerator GetEnumerator() => _set.GetEnumerator(); + } + } +} From 14dde3f7ee4d04f527026132e557e72fc68a5f7c Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 5 Apr 2023 14:58:44 -0400 Subject: [PATCH 2/2] Address PR feedback --- .../src/System/Collections/Frozen/Constants.cs | 16 +++++++++++++++- .../SmallValueTypeComparableFrozenDictionary.cs | 2 +- .../Frozen/SmallValueTypeComparableFrozenSet.cs | 2 +- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs index 995958e63db1ca..55e654a1e3029b 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/Constants.cs @@ -43,7 +43,21 @@ internal static class Constants /// public static bool IsKnownComparable() => // This list covers all of the IComparable value types in Corelib that aren't containers (like ValueTuple). - typeof(T).IsPrimitive || + typeof(T) == typeof(bool) || + typeof(T) == typeof(sbyte) || + typeof(T) == typeof(byte) || + typeof(T) == typeof(char) || + typeof(T) == typeof(short) || + typeof(T) == typeof(ushort) || + typeof(T) == typeof(int) || + typeof(T) == typeof(uint) || + typeof(T) == typeof(long) || + typeof(T) == typeof(ulong) || + typeof(T) == typeof(nint) || + typeof(T) == typeof(nuint) || + typeof(T) == typeof(decimal) || + typeof(T) == typeof(float) || + typeof(T) == typeof(double) || typeof(T) == typeof(decimal) || typeof(T) == typeof(TimeSpan) || typeof(T) == typeof(DateTime) || diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs index 8da882f7bda286..884cb7aeca807c 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenDictionary.cs @@ -13,7 +13,7 @@ namespace System.Collections.Frozen /// While not constrained in this manner, the must be an . /// This implementation is only used for a set of types that have a known-good implementation; it's not /// used for an as we can't know for sure whether it's valid, e.g. if the TKey is a ValueTuple`2, it itself - /// is comparable, but it's items might not be such that trying to compare it will result in exception. + /// is comparable, but its items might not be such that trying to compare it will result in exception. /// internal sealed class SmallValueTypeComparableFrozenDictionary : FrozenDictionary where TKey : notnull diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs index 41a103da59b67d..e5c4bbf87edf1e 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/SmallValueTypeComparableFrozenSet.cs @@ -13,7 +13,7 @@ namespace System.Collections.Frozen /// While not constrained in this manner, the must be an . /// This implementation is only used for a set of types that have a known-good implementation; it's not /// used for an as we can't know for sure whether it's valid, e.g. if the T is a ValueTuple`2, it itself - /// is comparable, but it's items might not be such that trying to compare it will result in exception. + /// is comparable, but its items might not be such that trying to compare it will result in exception. /// internal sealed class SmallValueTypeComparableFrozenSet : FrozenSetInternalBase.GSW> {