diff --git a/src/libraries/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.ConcurrentAccessDetection.cs b/src/libraries/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.ConcurrentAccessDetection.cs index 2d5985f2e30581..1bfbe361b60391 100644 --- a/src/libraries/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.ConcurrentAccessDetection.cs +++ b/src/libraries/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.ConcurrentAccessDetection.cs @@ -67,7 +67,7 @@ public async Task DictionaryConcurrentAccessDetection_ReferenceTypeKey(Type comp IEqualityComparer customComparer = null; Dictionary dic = comparerType == null ? - new Dictionary() : + new Dictionary((customComparer = EqualityComparer.Default)) : new Dictionary((customComparer = (IEqualityComparer)Activator.CreateInstance(comparerType))); var keyValueSample = new DummyRefType() { Value = 1 }; diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs index b77d3a2e43bbdb..88e63dde0aa114 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs @@ -52,22 +52,31 @@ public Dictionary(int capacity, IEqualityComparer? comparer) Initialize(capacity); } - if (comparer is not null && comparer != EqualityComparer.Default) // first check for null to avoid forcing default comparer instantiation unnecessarily + // For reference types, we always want to store a comparer instance, either + // the one provided, or if one wasn't provided, the default (accessing + // EqualityComparer.Default with shared generics on every dictionary + // access can add measurable overhead). For value types, if no comparer is + // provided, or if the default is provided, we'd prefer to use + // EqualityComparer.Default.Equals on every use, enabling the JIT to + // devirtualize and possibly inline the operation. + if (!typeof(TKey).IsValueType) { - _comparer = comparer; - } + _comparer = comparer ?? EqualityComparer.Default; - // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. - // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the - // hash buckets become unbalanced. - if (typeof(TKey) == typeof(string)) - { - IEqualityComparer? stringComparer = NonRandomizedStringEqualityComparer.GetStringComparer(_comparer); - if (stringComparer is not null) + // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. + // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the + // hash buckets become unbalanced. + if (typeof(TKey) == typeof(string) && + NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer stringComparer) { - _comparer = (IEqualityComparer?)stringComparer; + _comparer = (IEqualityComparer)stringComparer; } } + else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily + comparer != EqualityComparer.Default) + { + _comparer = comparer; + } } public Dictionary(IDictionary dictionary) : this(dictionary, null) { } @@ -161,7 +170,8 @@ public IEqualityComparer Comparer { if (typeof(TKey) == typeof(string)) { - return (IEqualityComparer)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer?)_comparer); + Debug.Assert(_comparer is not null, "The comparer should never be null for a reference type."); + return (IEqualityComparer)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer)_comparer); } else { @@ -362,76 +372,43 @@ internal ref TValue FindValue(TKey key) { Debug.Assert(_entries != null, "expected entries to be != null"); IEqualityComparer? comparer = _comparer; - if (comparer == null) + if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) { uint hashCode = (uint)key.GetHashCode(); int i = GetBucket(hashCode); Entry[]? entries = _entries; uint collisionCount = 0; - if (typeof(TKey).IsValueType) - { - // ValueType: Devirtualize with EqualityComparer.Default intrinsic - - i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. - do - { - // Should be a while loop https://github.com/dotnet/runtime/issues/9422 - // Test in if to drop range check for following array access - if ((uint)i >= (uint)entries.Length) - { - goto ReturnNotFound; - } - entry = ref entries[i]; - if (entry.hashCode == hashCode && EqualityComparer.Default.Equals(entry.key, key)) - { - goto ReturnFound; - } - - i = entry.next; - - collisionCount++; - } while (collisionCount <= (uint)entries.Length); - - // The chain of entries forms a loop; which means a concurrent update has happened. - // Break out of the loop and throw, rather than looping forever. - goto ConcurrentOperation; - } - else + // ValueType: Devirtualize with EqualityComparer.Default intrinsic + i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. + do { - // Object type: Shared Generic, EqualityComparer.Default won't devirtualize - // https://github.com/dotnet/runtime/issues/10050 - // So cache in a local rather than get EqualityComparer per loop iteration - EqualityComparer defaultComparer = EqualityComparer.Default; - - i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. - do + // Should be a while loop https://github.com/dotnet/runtime/issues/9422 + // Test in if to drop range check for following array access + if ((uint)i >= (uint)entries.Length) { - // Should be a while loop https://github.com/dotnet/runtime/issues/9422 - // Test in if to drop range check for following array access - if ((uint)i >= (uint)entries.Length) - { - goto ReturnNotFound; - } + goto ReturnNotFound; + } - entry = ref entries[i]; - if (entry.hashCode == hashCode && defaultComparer.Equals(entry.key, key)) - { - goto ReturnFound; - } + entry = ref entries[i]; + if (entry.hashCode == hashCode && EqualityComparer.Default.Equals(entry.key, key)) + { + goto ReturnFound; + } - i = entry.next; + i = entry.next; - collisionCount++; - } while (collisionCount <= (uint)entries.Length); + collisionCount++; + } while (collisionCount <= (uint)entries.Length); - // The chain of entries forms a loop; which means a concurrent update has happened. - // Break out of the loop and throw, rather than looping forever. - goto ConcurrentOperation; - } + // The chain of entries forms a loop; which means a concurrent update has happened. + // Break out of the loop and throw, rather than looping forever. + goto ConcurrentOperation; } else { + Debug.Assert(comparer is not null); uint hashCode = (uint)comparer.GetHashCode(key); int i = GetBucket(hashCode); Entry[]? entries = _entries; @@ -513,98 +490,56 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) Debug.Assert(entries != null, "expected entries to be non-null"); IEqualityComparer? comparer = _comparer; - uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); + Debug.Assert(comparer is not null || typeof(TKey).IsValueType); + uint hashCode = (uint)((typeof(TKey).IsValueType && comparer == null) ? key.GetHashCode() : comparer!.GetHashCode(key)); uint collisionCount = 0; ref int bucket = ref GetBucket(hashCode); int i = bucket - 1; // Value in _buckets is 1-based - if (comparer == null) + if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) { - if (typeof(TKey).IsValueType) + // ValueType: Devirtualize with EqualityComparer.Default intrinsic + while (true) { - // ValueType: Devirtualize with EqualityComparer.Default intrinsic - while (true) + // Should be a while loop https://github.com/dotnet/runtime/issues/9422 + // Test uint in if rather than loop condition to drop range check for following array access + if ((uint)i >= (uint)entries.Length) { - // Should be a while loop https://github.com/dotnet/runtime/issues/9422 - // Test uint in if rather than loop condition to drop range check for following array access - if ((uint)i >= (uint)entries.Length) - { - break; - } - - if (entries[i].hashCode == hashCode && EqualityComparer.Default.Equals(entries[i].key, key)) - { - if (behavior == InsertionBehavior.OverwriteExisting) - { - entries[i].value = value; - return true; - } - - if (behavior == InsertionBehavior.ThrowOnExisting) - { - ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key); - } - - return false; - } - - i = entries[i].next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop; which means a concurrent update has happened. - // Break out of the loop and throw, rather than looping forever. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + break; } - } - else - { - // Object type: Shared Generic, EqualityComparer.Default won't devirtualize - // https://github.com/dotnet/runtime/issues/10050 - // So cache in a local rather than get EqualityComparer per loop iteration - EqualityComparer defaultComparer = EqualityComparer.Default; - while (true) + + if (entries[i].hashCode == hashCode && EqualityComparer.Default.Equals(entries[i].key, key)) { - // Should be a while loop https://github.com/dotnet/runtime/issues/9422 - // Test uint in if rather than loop condition to drop range check for following array access - if ((uint)i >= (uint)entries.Length) + if (behavior == InsertionBehavior.OverwriteExisting) { - break; + entries[i].value = value; + return true; } - if (entries[i].hashCode == hashCode && defaultComparer.Equals(entries[i].key, key)) + if (behavior == InsertionBehavior.ThrowOnExisting) { - if (behavior == InsertionBehavior.OverwriteExisting) - { - entries[i].value = value; - return true; - } - - if (behavior == InsertionBehavior.ThrowOnExisting) - { - ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key); - } - - return false; + ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key); } - i = entries[i].next; + return false; + } - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop; which means a concurrent update has happened. - // Break out of the loop and throw, rather than looping forever. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + i = entries[i].next; + + collisionCount++; + if (collisionCount > (uint)entries.Length) + { + // The chain of entries forms a loop; which means a concurrent update has happened. + // Break out of the loop and throw, rather than looping forever. + ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); } } } else { + Debug.Assert(comparer is not null); while (true) { // Should be a while loop https://github.com/dotnet/runtime/issues/9422 @@ -710,80 +645,47 @@ internal static class CollectionsMarshalHelper Debug.Assert(entries != null, "expected entries to be non-null"); IEqualityComparer? comparer = dictionary._comparer; - uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); + Debug.Assert(comparer is not null || typeof(TKey).IsValueType); + uint hashCode = (uint)((typeof(TKey).IsValueType && comparer == null) ? key.GetHashCode() : comparer!.GetHashCode(key)); uint collisionCount = 0; ref int bucket = ref dictionary.GetBucket(hashCode); int i = bucket - 1; // Value in _buckets is 1-based - if (comparer == null) + if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) { - if (typeof(TKey).IsValueType) + // ValueType: Devirtualize with EqualityComparer.Default intrinsic + while (true) { - // ValueType: Devirtualize with EqualityComparer.Default intrinsic - while (true) + // Should be a while loop https://github.com/dotnet/runtime/issues/9422 + // Test uint in if rather than loop condition to drop range check for following array access + if ((uint)i >= (uint)entries.Length) { - // Should be a while loop https://github.com/dotnet/runtime/issues/9422 - // Test uint in if rather than loop condition to drop range check for following array access - if ((uint)i >= (uint)entries.Length) - { - break; - } - - if (entries[i].hashCode == hashCode && EqualityComparer.Default.Equals(entries[i].key, key)) - { - exists = true; - - return ref entries[i].value!; - } - - i = entries[i].next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop; which means a concurrent update has happened. - // Break out of the loop and throw, rather than looping forever. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + break; } - } - else - { - // Object type: Shared Generic, EqualityComparer.Default won't devirtualize - // https://github.com/dotnet/runtime/issues/10050 - // So cache in a local rather than get EqualityComparer per loop iteration - EqualityComparer defaultComparer = EqualityComparer.Default; - while (true) + + if (entries[i].hashCode == hashCode && EqualityComparer.Default.Equals(entries[i].key, key)) + { + exists = true; + + return ref entries[i].value!; + } + + i = entries[i].next; + + collisionCount++; + if (collisionCount > (uint)entries.Length) { - // Should be a while loop https://github.com/dotnet/runtime/issues/9422 - // Test uint in if rather than loop condition to drop range check for following array access - if ((uint)i >= (uint)entries.Length) - { - break; - } - - if (entries[i].hashCode == hashCode && defaultComparer.Equals(entries[i].key, key)) - { - exists = true; - - return ref entries[i].value!; - } - - i = entries[i].next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop; which means a concurrent update has happened. - // Break out of the loop and throw, rather than looping forever. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + // The chain of entries forms a loop; which means a concurrent update has happened. + // Break out of the loop and throw, rather than looping forever. + ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); } } } else { + Debug.Assert(comparer is not null); while (true) { // Should be a while loop https://github.com/dotnet/runtime/issues/9422 @@ -929,20 +831,15 @@ private void Resize(int newSize, bool forceNewHashCodes) if (!typeof(TKey).IsValueType && forceNewHashCodes) { Debug.Assert(_comparer is NonRandomizedStringEqualityComparer); - _comparer = (IEqualityComparer)((NonRandomizedStringEqualityComparer)_comparer).GetRandomizedEqualityComparer(); + IEqualityComparer comparer = _comparer = (IEqualityComparer)((NonRandomizedStringEqualityComparer)_comparer).GetRandomizedEqualityComparer(); for (int i = 0; i < count; i++) { if (entries[i].next >= -1) { - entries[i].hashCode = (uint)_comparer.GetHashCode(entries[i].key); + entries[i].hashCode = (uint)comparer.GetHashCode(entries[i].key); } } - - if (ReferenceEquals(_comparer, EqualityComparer.Default)) - { - _comparer = null; - } } // Assign member variables after both arrays allocated to guard against corruption from OOM if second fails @@ -978,7 +875,11 @@ public bool Remove(TKey key) { Debug.Assert(_entries != null, "entries should be non-null"); uint collisionCount = 0; - uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); + + IEqualityComparer? comparer = _comparer; + Debug.Assert(typeof(TKey).IsValueType || comparer is not null); + uint hashCode = (uint)(typeof(TKey).IsValueType && comparer == null ? key.GetHashCode() : comparer!.GetHashCode(key)); + ref int bucket = ref GetBucket(hashCode); Entry[]? entries = _entries; int last = -1; @@ -987,7 +888,8 @@ public bool Remove(TKey key) { ref Entry entry = ref entries[i]; - if (entry.hashCode == hashCode && (_comparer?.Equals(entry.key, key) ?? EqualityComparer.Default.Equals(entry.key, key))) + if (entry.hashCode == hashCode && + (typeof(TKey).IsValueType && comparer == null ? EqualityComparer.Default.Equals(entry.key, key) : comparer!.Equals(entry.key, key))) { if (last < 0) { @@ -1046,7 +948,11 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) { Debug.Assert(_entries != null, "entries should be non-null"); uint collisionCount = 0; - uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); + + IEqualityComparer? comparer = _comparer; + Debug.Assert(typeof(TKey).IsValueType || comparer is not null); + uint hashCode = (uint)(typeof(TKey).IsValueType && comparer == null ? key.GetHashCode() : comparer!.GetHashCode(key)); + ref int bucket = ref GetBucket(hashCode); Entry[]? entries = _entries; int last = -1; @@ -1055,7 +961,8 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) { ref Entry entry = ref entries[i]; - if (entry.hashCode == hashCode && (_comparer?.Equals(entry.key, key) ?? EqualityComparer.Default.Equals(entry.key, key))) + if (entry.hashCode == hashCode && + (typeof(TKey).IsValueType && comparer == null ? EqualityComparer.Default.Equals(entry.key, key) : comparer!.Equals(entry.key, key))) { if (last < 0) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs index a4f392142bc27b..fbee608bbf4809 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs @@ -94,9 +94,11 @@ public DelegateEqualityComparer(Func equals, Func getHashC _getHashCode = getHashCode; } - public override bool Equals(T? x, T? y) => _equals(x, y); + public override bool Equals(T? x, T? y) => + _equals(x, y); - public override int GetHashCode([DisallowNull] T obj) => _getHashCode(obj); + public override int GetHashCode([DisallowNull] T obj) => + _getHashCode(obj); public override bool Equals(object? obj) => obj is DelegateEqualityComparer other && @@ -127,16 +129,15 @@ public override bool Equals(T? x, T? y) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public override int GetHashCode([DisallowNull] T obj) => obj?.GetHashCode() ?? 0; + public override int GetHashCode([DisallowNull] T obj) => + obj?.GetHashCode() ?? 0; // Equals method for the comparer itself. - // If in the future this type is made sealed, change the is check to obj != null && GetType() == obj.GetType(). public override bool Equals([NotNullWhen(true)] object? obj) => - obj is GenericEqualityComparer; + obj != null && GetType() == obj.GetType(); - // If in the future this type is made sealed, change typeof(...) to GetType(). public override int GetHashCode() => - typeof(GenericEqualityComparer).GetHashCode(); + GetType().GetHashCode(); } [Serializable] @@ -168,7 +169,8 @@ public override bool Equals(T? x, T? y) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public override int GetHashCode(T? obj) => obj.GetHashCode(); + public override int GetHashCode(T? obj) => + obj.GetHashCode(); // Equals method for the comparer itself. public override bool Equals([NotNullWhen(true)] object? obj) => @@ -196,7 +198,8 @@ public override bool Equals(T? x, T? y) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public override int GetHashCode([DisallowNull] T obj) => obj?.GetHashCode() ?? 0; + public override int GetHashCode([DisallowNull] T obj) => + obj?.GetHashCode() ?? 0; // Equals method for the comparer itself. public override bool Equals([NotNullWhen(true)] object? obj) => @@ -212,16 +215,12 @@ public override int GetHashCode() => public sealed partial class ByteEqualityComparer : EqualityComparer { [MethodImpl(MethodImplOptions.AggressiveInlining)] - public override bool Equals(byte x, byte y) - { - return x == y; - } + public override bool Equals(byte x, byte y) => + x == y; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public override int GetHashCode(byte b) - { - return b.GetHashCode(); - } + public override int GetHashCode(byte b) => + b.GetHashCode(); // Equals method for the comparer itself. public override bool Equals([NotNullWhen(true)] object? obj) => @@ -253,10 +252,8 @@ public void GetObjectData(SerializationInfo info, StreamingContext context) // public override bool Equals(T x, T y) is runtime-specific [MethodImpl(MethodImplOptions.AggressiveInlining)] - public override int GetHashCode(T obj) - { - return obj.GetHashCode(); - } + public override int GetHashCode(T obj) => + obj.GetHashCode(); // Equals method for the comparer itself. public override bool Equals([NotNullWhen(true)] object? obj) => diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs index 47b72f57deacae..f842455c827dd6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs @@ -52,22 +52,31 @@ public HashSet() : this((IEqualityComparer?)null) { } public HashSet(IEqualityComparer? comparer) { - if (comparer is not null && comparer != EqualityComparer.Default) // first check for null to avoid forcing default comparer instantiation unnecessarily - { - _comparer = comparer; - } - - // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. - // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the - // hash buckets become unbalanced. - if (typeof(T) == typeof(string)) - { - IEqualityComparer? stringComparer = NonRandomizedStringEqualityComparer.GetStringComparer(_comparer); - if (stringComparer is not null) + // For reference types, we always want to store a comparer instance, either + // the one provided, or if one wasn't provided, the default (accessing + // EqualityComparer.Default with shared generics on every dictionary + // access can add measurable overhead). For value types, if no comparer is + // provided, or if the default is provided, we'd prefer to use + // EqualityComparer.Default.Equals on every use, enabling the JIT to + // devirtualize and possibly inline the operation. + if (!typeof(T).IsValueType) + { + _comparer = comparer ?? EqualityComparer.Default; + + // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. + // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the + // hash buckets become unbalanced. + if (typeof(T) == typeof(string) && + NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer stringComparer) { - _comparer = (IEqualityComparer?)stringComparer; + _comparer = (IEqualityComparer)stringComparer; } } + else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily + comparer != EqualityComparer.Default) + { + _comparer = comparer; + } } public HashSet(int capacity) : this(capacity, null) { } @@ -212,56 +221,32 @@ private int FindItemIndex(T item) uint collisionCount = 0; IEqualityComparer? comparer = _comparer; - if (comparer == null) + if (typeof(T).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) { - int hashCode = item != null ? item.GetHashCode() : 0; - if (typeof(T).IsValueType) + // ValueType: Devirtualize with EqualityComparer.Default intrinsic + int hashCode = item!.GetHashCode(); + int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based + while (i >= 0) { - // ValueType: Devirtualize with EqualityComparer.Default intrinsic - int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based - while (i >= 0) + ref Entry entry = ref entries[i]; + if (entry.HashCode == hashCode && EqualityComparer.Default.Equals(entry.Value, item)) { - ref Entry entry = ref entries[i]; - if (entry.HashCode == hashCode && EqualityComparer.Default.Equals(entry.Value, item)) - { - return i; - } - i = entry.Next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop, which means a concurrent update has happened. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + return i; } - } - else - { - // Object type: Shared Generic, EqualityComparer.Default won't devirtualize (https://github.com/dotnet/runtime/issues/10050), - // so cache in a local rather than get EqualityComparer per loop iteration. - EqualityComparer defaultComparer = EqualityComparer.Default; - int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based - while (i >= 0) + i = entry.Next; + + collisionCount++; + if (collisionCount > (uint)entries.Length) { - ref Entry entry = ref entries[i]; - if (entry.HashCode == hashCode && defaultComparer.Equals(entry.Value, item)) - { - return i; - } - i = entry.Next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop, which means a concurrent update has happened. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + // The chain of entries forms a loop, which means a concurrent update has happened. + ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); } } } else { + Debug.Assert(comparer is not null); int hashCode = item != null ? comparer.GetHashCode(item) : 0; int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based while (i >= 0) @@ -307,7 +292,13 @@ public bool Remove(T item) uint collisionCount = 0; int last = -1; - int hashCode = item != null ? (_comparer?.GetHashCode(item) ?? item.GetHashCode()) : 0; + + IEqualityComparer? comparer = _comparer; + Debug.Assert(typeof(T).IsValueType || comparer is not null); + int hashCode = + typeof(T).IsValueType && comparer == null ? item!.GetHashCode() : + item is not null ? comparer!.GetHashCode(item) : + 0; ref int bucket = ref GetBucketRef(hashCode); int i = bucket - 1; // Value in buckets is 1-based @@ -316,7 +307,7 @@ public bool Remove(T item) { ref Entry entry = ref entries[i]; - if (entry.HashCode == hashCode && (_comparer?.Equals(entry.Value, item) ?? EqualityComparer.Default.Equals(entry.Value, item))) + if (entry.HashCode == hashCode && (comparer?.Equals(entry.Value, item) ?? EqualityComparer.Default.Equals(entry.Value, item))) { if (last < 0) { @@ -922,7 +913,8 @@ public IEqualityComparer Comparer { if (typeof(T) == typeof(string)) { - return (IEqualityComparer)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer?)_comparer); + Debug.Assert(_comparer is not null, "The comparer should never be null for a reference type."); + return (IEqualityComparer)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer)_comparer); } else { @@ -972,21 +964,16 @@ private void Resize(int newSize, bool forceNewHashCodes) if (!typeof(T).IsValueType && forceNewHashCodes) { Debug.Assert(_comparer is NonRandomizedStringEqualityComparer); - _comparer = (IEqualityComparer)((NonRandomizedStringEqualityComparer)_comparer).GetRandomizedEqualityComparer(); + IEqualityComparer comparer = _comparer = (IEqualityComparer)((NonRandomizedStringEqualityComparer)_comparer).GetRandomizedEqualityComparer(); for (int i = 0; i < count; i++) { ref Entry entry = ref entries[i]; if (entry.Next >= -1) { - entry.HashCode = entry.Value != null ? _comparer!.GetHashCode(entry.Value) : 0; + entry.HashCode = entry.Value != null ? comparer.GetHashCode(entry.Value) : 0; } } - - if (ReferenceEquals(_comparer, EqualityComparer.Default)) - { - _comparer = null; - } } // Assign member variables after both arrays allocated to guard against corruption from OOM if second fails @@ -1096,58 +1083,35 @@ private bool AddIfNotPresent(T value, out int location) uint collisionCount = 0; ref int bucket = ref Unsafe.NullRef(); - if (comparer == null) + if (typeof(T).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) { - hashCode = value != null ? value.GetHashCode() : 0; + hashCode = value!.GetHashCode(); bucket = ref GetBucketRef(hashCode); int i = bucket - 1; // Value in _buckets is 1-based - if (typeof(T).IsValueType) - { - // ValueType: Devirtualize with EqualityComparer.Default intrinsic - while (i >= 0) - { - ref Entry entry = ref entries[i]; - if (entry.HashCode == hashCode && EqualityComparer.Default.Equals(entry.Value, value)) - { - location = i; - return false; - } - i = entry.Next; - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop, which means a concurrent update has happened. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } - } - } - else + // ValueType: Devirtualize with EqualityComparer.Default intrinsic + while (i >= 0) { - // Object type: Shared Generic, EqualityComparer.Default won't devirtualize (https://github.com/dotnet/runtime/issues/10050), - // so cache in a local rather than get EqualityComparer per loop iteration. - EqualityComparer defaultComparer = EqualityComparer.Default; - while (i >= 0) + ref Entry entry = ref entries[i]; + if (entry.HashCode == hashCode && EqualityComparer.Default.Equals(entry.Value, value)) { - ref Entry entry = ref entries[i]; - if (entry.HashCode == hashCode && defaultComparer.Equals(entry.Value, value)) - { - location = i; - return false; - } - i = entry.Next; + location = i; + return false; + } + i = entry.Next; - collisionCount++; - if (collisionCount > (uint)entries.Length) - { - // The chain of entries forms a loop, which means a concurrent update has happened. - ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); - } + collisionCount++; + if (collisionCount > (uint)entries.Length) + { + // The chain of entries forms a loop, which means a concurrent update has happened. + ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); } } } else { + Debug.Assert(comparer is not null); hashCode = value != null ? comparer.GetHashCode(value) : 0; bucket = ref GetBucketRef(hashCode); int i = bucket - 1; // Value in _buckets is 1-based diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IInternalStringEqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IInternalStringEqualityComparer.cs index 28c2ab9a07c9d8..a084a81becfc02 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IInternalStringEqualityComparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IInternalStringEqualityComparer.cs @@ -17,13 +17,9 @@ internal interface IInternalStringEqualityComparer : IEqualityComparer /// Unwraps the internal equality comparer, if proxied. /// Otherwise returns the equality comparer itself or its default equivalent. /// - internal static IEqualityComparer GetUnderlyingEqualityComparer(IEqualityComparer? outerComparer) + internal static IEqualityComparer GetUnderlyingEqualityComparer(IEqualityComparer outerComparer) { - if (outerComparer is null) - { - return EqualityComparer.Default; - } - else if (outerComparer is IInternalStringEqualityComparer internalComparer) + if (outerComparer is IInternalStringEqualityComparer internalComparer) { return internalComparer.GetUnderlyingEqualityComparer(); } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs index 1724cdcd1daf25..25846a6b2abb2c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs @@ -108,20 +108,23 @@ internal override RandomizedStringEqualityComparer GetRandomizedEqualityComparer } } - public static IEqualityComparer? GetStringComparer(object? comparer) + public static IEqualityComparer? GetStringComparer(object comparer) { // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the // hash buckets become unbalanced. - if (comparer is null) + + if (ReferenceEquals(comparer, EqualityComparer.Default)) { return WrappedAroundDefaultComparer; } - else if (ReferenceEquals(comparer, StringComparer.Ordinal)) + + if (ReferenceEquals(comparer, StringComparer.Ordinal)) { return WrappedAroundStringComparerOrdinal; } - else if (ReferenceEquals(comparer, StringComparer.OrdinalIgnoreCase)) + + if (ReferenceEquals(comparer, StringComparer.OrdinalIgnoreCase)) { return WrappedAroundStringComparerOrdinalIgnoreCase; }