From f5d5e3ae2fcb6ff360f16c545c3a31b9e2a2cba8 Mon Sep 17 00:00:00 2001
From: Stephen Toub <stoub@microsoft.com>
Date: Wed, 14 Sep 2022 23:32:10 -0400
Subject: [PATCH 1/5] Fix Dictionary perf regression for non-string ref type
 keys

Several releases ago, some performance improvements were made to Dictionary that were particularly valuable for value type keys, enabling equality comparisons to be devirtualized and possibly inlined. For non-string reference type keys, however, this can end up being a regression, as every dictionary access then needs to access `EqualityComparer<TKey>.Default`, which can be more expensive with shared generics.  The access is hoisted out of a loop, but at least one per call is still needed.

This fixes the regression by ensuring we always store a comparer in the dictionary's constructor if TKey is a reference type.  The same fix is applied to `HashSet<T>`. This also means we can delete some now unreachable code.
---
 ...Generic.Tests.ConcurrentAccessDetection.cs |   2 +-
 .../System/Collections/Generic/Dictionary.cs  | 279 ++++++------------
 .../src/System/Collections/Generic/HashSet.cs | 138 +++------
 .../IInternalStringEqualityComparer.cs        |   8 +-
 .../NonRandomizedStringEqualityComparer.cs    |   4 +-
 5 files changed, 140 insertions(+), 291 deletions(-)

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<DummyRefType> customComparer = null;
 
             Dictionary<DummyRefType, DummyRefType> dic = comparerType == null ?
-                new Dictionary<DummyRefType, DummyRefType>() :
+                new Dictionary<DummyRefType, DummyRefType>((customComparer = EqualityComparer<DummyRefType>.Default)) :
                 new Dictionary<DummyRefType, DummyRefType>((customComparer = (IEqualityComparer<DummyRefType>)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..4790a285880812 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,7 +52,19 @@ public Dictionary(int capacity, IEqualityComparer<TKey>? comparer)
                 Initialize(capacity);
             }
 
-            if (comparer is not null && comparer != EqualityComparer<TKey>.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<TKey>.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<TKey>.Default.Equals on every use, enabling the JIT to
+            // devirtualize and possibly inline the operation.
+            if (!typeof(TKey).IsValueType)
+            {
+                _comparer = comparer ?? EqualityComparer<TKey>.Default;
+            }
+            else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily
+                     comparer != EqualityComparer<TKey>.Default)
             {
                 _comparer = comparer;
             }
@@ -60,13 +72,10 @@ public Dictionary(int capacity, IEqualityComparer<TKey>? comparer)
             // Special-case EqualityComparer<string>.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))
+            if (typeof(TKey) == typeof(string) &&
+                NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer<string> stringComparer)
             {
-                IEqualityComparer<string>? stringComparer = NonRandomizedStringEqualityComparer.GetStringComparer(_comparer);
-                if (stringComparer is not null)
-                {
-                    _comparer = (IEqualityComparer<TKey>?)stringComparer;
-                }
+                _comparer = (IEqualityComparer<TKey>)stringComparer;
             }
         }
 
@@ -161,7 +170,8 @@ public IEqualityComparer<TKey> Comparer
             {
                 if (typeof(TKey) == typeof(string))
                 {
-                    return (IEqualityComparer<TKey>)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer<string?>?)_comparer);
+                    Debug.Assert(_comparer is not null, "The comparer should never be null for a reference type.");
+                    return (IEqualityComparer<TKey>)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer<string?>)_comparer);
                 }
                 else
                 {
@@ -364,71 +374,38 @@ internal ref TValue FindValue(TKey key)
                 IEqualityComparer<TKey>? comparer = _comparer;
                 if (comparer == null)
                 {
+                    Debug.Assert(typeof(TKey).IsValueType);
+
                     uint hashCode = (uint)key.GetHashCode();
                     int i = GetBucket(hashCode);
                     Entry[]? entries = _entries;
                     uint collisionCount = 0;
-                    if (typeof(TKey).IsValueType)
-                    {
-                        // ValueType: Devirtualize with EqualityComparer<TValue>.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<TKey>.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<TValue>.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<TValue>.Default won't devirtualize
-                        // https://github.com/dotnet/runtime/issues/10050
-                        // So cache in a local rather than get EqualityComparer per loop iteration
-                        EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.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<TKey>.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
                 {
@@ -521,85 +498,42 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
 
             if (comparer == null)
             {
-                if (typeof(TKey).IsValueType)
+                Debug.Assert(typeof(TKey).IsValueType);
+
+                // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
+                while (true)
                 {
-                    // ValueType: Devirtualize with EqualityComparer<TValue>.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<TKey>.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<TValue>.Default won't devirtualize
-                    // https://github.com/dotnet/runtime/issues/10050
-                    // So cache in a local rather than get EqualityComparer per loop iteration
-                    EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default;
-                    while (true)
+
+                    if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.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();
                     }
                 }
             }
@@ -718,67 +652,33 @@ internal static class CollectionsMarshalHelper
 
                 if (comparer == null)
                 {
-                    if (typeof(TKey).IsValueType)
+                    Debug.Assert(typeof(TKey).IsValueType);
+
+                    // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
+                    while (true)
                     {
-                        // ValueType: Devirtualize with EqualityComparer<TValue>.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<TKey>.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<TValue>.Default won't devirtualize
-                        // https://github.com/dotnet/runtime/issues/10050
-                        // So cache in a local rather than get EqualityComparer per loop iteration
-                        EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default;
-                        while (true)
+
+                        if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.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)
-                            {
-                                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();
-                            }
+                            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();
                         }
                     }
                 }
@@ -938,11 +838,6 @@ private void Resize(int newSize, bool forceNewHashCodes)
                         entries[i].hashCode = (uint)_comparer.GetHashCode(entries[i].key);
                     }
                 }
-
-                if (ReferenceEquals(_comparer, EqualityComparer<TKey>.Default))
-                {
-                    _comparer = null;
-                }
             }
 
             // Assign member variables after both arrays allocated to guard against corruption from OOM if second fails
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..d4d1ab2187cf32 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,7 +52,19 @@ public HashSet() : this((IEqualityComparer<T>?)null) { }
 
         public HashSet(IEqualityComparer<T>? comparer)
         {
-            if (comparer is not null && comparer != EqualityComparer<T>.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<TKey>.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<TKey>.Default.Equals on every use, enabling the JIT to
+            // devirtualize and possibly inline the operation.
+            if (!typeof(T).IsValueType)
+            {
+                _comparer = comparer ?? EqualityComparer<T>.Default;
+            }
+            else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily
+                     comparer != EqualityComparer<T>.Default)
             {
                 _comparer = comparer;
             }
@@ -60,13 +72,10 @@ public HashSet(IEqualityComparer<T>? comparer)
             // Special-case EqualityComparer<string>.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))
+            if (typeof(T) == typeof(string) &&
+                NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer<string> stringComparer)
             {
-                IEqualityComparer<string>? stringComparer = NonRandomizedStringEqualityComparer.GetStringComparer(_comparer);
-                if (stringComparer is not null)
-                {
-                    _comparer = (IEqualityComparer<T>?)stringComparer;
-                }
+                _comparer = (IEqualityComparer<T>)stringComparer;
             }
         }
 
@@ -214,49 +223,25 @@ private int FindItemIndex(T item)
 
                 if (comparer == null)
                 {
+                    Debug.Assert(typeof(T).IsValueType);
+
+                    // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
                     int hashCode = item != null ? item.GetHashCode() : 0;
-                    if (typeof(T).IsValueType)
+                    int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based
+                    while (i >= 0)
                     {
-                        // ValueType: Devirtualize with EqualityComparer<TValue>.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<T>.Default.Equals(entry.Value, item))
                         {
-                            ref Entry entry = ref entries[i];
-                            if (entry.HashCode == hashCode && EqualityComparer<T>.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<TValue>.Default won't devirtualize (https://github.com/dotnet/runtime/issues/10050),
-                        // so cache in a local rather than get EqualityComparer per loop iteration.
-                        EqualityComparer<T> defaultComparer = EqualityComparer<T>.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();
                         }
                     }
                 }
@@ -922,7 +907,8 @@ public IEqualityComparer<T> Comparer
             {
                 if (typeof(T) == typeof(string))
                 {
-                    return (IEqualityComparer<T>)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer<string?>?)_comparer);
+                    Debug.Assert(_comparer is not null, "The comparer should never be null for a reference type.");
+                    return (IEqualityComparer<T>)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer<string?>)_comparer);
                 }
                 else
                 {
@@ -979,14 +965,9 @@ private void Resize(int newSize, bool forceNewHashCodes)
                     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<T>.Default))
-                {
-                    _comparer = null;
-                }
             }
 
             // Assign member variables after both arrays allocated to guard against corruption from OOM if second fails
@@ -1098,51 +1079,28 @@ private bool AddIfNotPresent(T value, out int location)
 
             if (comparer == null)
             {
+                Debug.Assert(typeof(T).IsValueType);
+
                 hashCode = value != null ? value.GetHashCode() : 0;
                 bucket = ref GetBucketRef(hashCode);
                 int i = bucket - 1; // Value in _buckets is 1-based
-                if (typeof(T).IsValueType)
-                {
-                    // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
-                    while (i >= 0)
-                    {
-                        ref Entry entry = ref entries[i];
-                        if (entry.HashCode == hashCode && EqualityComparer<T>.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<TValue>.Default intrinsic
+                while (i >= 0)
                 {
-                    // Object type: Shared Generic, EqualityComparer<TValue>.Default won't devirtualize (https://github.com/dotnet/runtime/issues/10050),
-                    // so cache in a local rather than get EqualityComparer per loop iteration.
-                    EqualityComparer<T> defaultComparer = EqualityComparer<T>.Default;
-                    while (i >= 0)
+                    ref Entry entry = ref entries[i];
+                    if (entry.HashCode == hashCode && EqualityComparer<T>.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();
                     }
                 }
             }
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<string?>
         /// Unwraps the internal equality comparer, if proxied.
         /// Otherwise returns the equality comparer itself or its default equivalent.
         /// </summary>
-        internal static IEqualityComparer<string?> GetUnderlyingEqualityComparer(IEqualityComparer<string?>? outerComparer)
+        internal static IEqualityComparer<string?> GetUnderlyingEqualityComparer(IEqualityComparer<string?> outerComparer)
         {
-            if (outerComparer is null)
-            {
-                return EqualityComparer<string?>.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..7785668a68ae76 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,12 +108,12 @@ internal override RandomizedStringEqualityComparer GetRandomizedEqualityComparer
             }
         }
 
-        public static IEqualityComparer<string>? GetStringComparer(object? comparer)
+        public static IEqualityComparer<string>? GetStringComparer(object comparer)
         {
             // Special-case EqualityComparer<string>.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<string>.Default))
             {
                 return WrappedAroundDefaultComparer;
             }

From 9f402a52c3149bd72e3e4c1f35ab747ce4ab370c Mon Sep 17 00:00:00 2001
From: Stephen Toub <stoub@microsoft.com>
Date: Thu, 15 Sep 2022 10:23:57 -0400
Subject: [PATCH 2/5] Address PR feedback

---
 .../System/Collections/Generic/Dictionary.cs  | 44 ++++++++++++-------
 .../Collections/Generic/EqualityComparer.cs   | 21 +++------
 .../src/System/Collections/Generic/HashSet.cs | 30 ++++++++-----
 .../NonRandomizedStringEqualityComparer.cs    |  9 ++--
 4 files changed, 58 insertions(+), 46 deletions(-)

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 4790a285880812..75e856ec0f3bef 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
@@ -372,10 +372,9 @@ internal ref TValue FindValue(TKey key)
             {
                 Debug.Assert(_entries != null, "expected entries to be != null");
                 IEqualityComparer<TKey>? 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)
                 {
-                    Debug.Assert(typeof(TKey).IsValueType);
-
                     uint hashCode = (uint)key.GetHashCode();
                     int i = GetBucket(hashCode);
                     Entry[]? entries = _entries;
@@ -409,6 +408,7 @@ internal ref TValue FindValue(TKey key)
                 }
                 else
                 {
+                    Debug.Assert(comparer is not null);
                     uint hashCode = (uint)comparer.GetHashCode(key);
                     int i = GetBucket(hashCode);
                     Entry[]? entries = _entries;
@@ -490,16 +490,16 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
             Debug.Assert(entries != null, "expected entries to be non-null");
 
             IEqualityComparer<TKey>? 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)
             {
-                Debug.Assert(typeof(TKey).IsValueType);
-
                 // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
                 while (true)
                 {
@@ -539,6 +539,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
             }
             else
             {
+                Debug.Assert(comparer is not null);
                 while (true)
                 {
                     // Should be a while loop https://github.com/dotnet/runtime/issues/9422
@@ -644,16 +645,16 @@ internal static class CollectionsMarshalHelper
                 Debug.Assert(entries != null, "expected entries to be non-null");
 
                 IEqualityComparer<TKey>? 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)
                 {
-                    Debug.Assert(typeof(TKey).IsValueType);
-
                     // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
                     while (true)
                     {
@@ -684,6 +685,7 @@ internal static class CollectionsMarshalHelper
                 }
                 else
                 {
+                    Debug.Assert(comparer is not null);
                     while (true)
                     {
                         // Should be a while loop https://github.com/dotnet/runtime/issues/9422
@@ -829,13 +831,13 @@ private void Resize(int newSize, bool forceNewHashCodes)
             if (!typeof(TKey).IsValueType && forceNewHashCodes)
             {
                 Debug.Assert(_comparer is NonRandomizedStringEqualityComparer);
-                _comparer = (IEqualityComparer<TKey>)((NonRandomizedStringEqualityComparer)_comparer).GetRandomizedEqualityComparer();
+                IEqualityComparer<TKey> comparer = _comparer = (IEqualityComparer<TKey>)((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);
                     }
                 }
             }
@@ -873,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<TKey>? 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;
@@ -882,7 +888,7 @@ public bool Remove(TKey key)
                 {
                     ref Entry entry = ref entries[i];
 
-                    if (entry.hashCode == hashCode && (_comparer?.Equals(entry.key, key) ?? EqualityComparer<TKey>.Default.Equals(entry.key, key)))
+                    if (entry.hashCode == hashCode && (comparer?.Equals(entry.key, key) ?? EqualityComparer<TKey>.Default.Equals(entry.key, key)))
                     {
                         if (last < 0)
                         {
@@ -941,7 +947,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<TKey>? 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;
@@ -950,7 +960,7 @@ 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<TKey>.Default.Equals(entry.key, key)))
+                    if (entry.hashCode == hashCode && (comparer?.Equals(entry.key, key) ?? EqualityComparer<TKey>.Default.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..297c5288cfe257 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
@@ -130,9 +130,8 @@ public override bool Equals(T? x, T? y)
         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<T>;
+            obj != null && GetType() == obj.GetType();
 
         // If in the future this type is made sealed, change typeof(...) to GetType().
         public override int GetHashCode() =>
@@ -212,16 +211,12 @@ public override int GetHashCode() =>
     public sealed partial class ByteEqualityComparer : EqualityComparer<byte>
     {
         [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 +248,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 d4d1ab2187cf32..39a1cc9906e14a 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
@@ -221,12 +221,11 @@ private int FindItemIndex(T item)
                 uint collisionCount = 0;
                 IEqualityComparer<T>? 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)
                 {
-                    Debug.Assert(typeof(T).IsValueType);
-
                     // ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
-                    int hashCode = item != null ? item.GetHashCode() : 0;
+                    int hashCode = item!.GetHashCode();
                     int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based
                     while (i >= 0)
                     {
@@ -247,6 +246,7 @@ private int FindItemIndex(T item)
                 }
                 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)
@@ -292,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<T>? 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
@@ -301,7 +307,7 @@ public bool Remove(T item)
                 {
                     ref Entry entry = ref entries[i];
 
-                    if (entry.HashCode == hashCode && (_comparer?.Equals(entry.Value, item) ?? EqualityComparer<T>.Default.Equals(entry.Value, item)))
+                    if (entry.HashCode == hashCode && (comparer?.Equals(entry.Value, item) ?? EqualityComparer<T>.Default.Equals(entry.Value, item)))
                     {
                         if (last < 0)
                         {
@@ -958,14 +964,14 @@ private void Resize(int newSize, bool forceNewHashCodes)
             if (!typeof(T).IsValueType && forceNewHashCodes)
             {
                 Debug.Assert(_comparer is NonRandomizedStringEqualityComparer);
-                _comparer = (IEqualityComparer<T>)((NonRandomizedStringEqualityComparer)_comparer).GetRandomizedEqualityComparer();
+                IEqualityComparer<T> comparer = _comparer = (IEqualityComparer<T>)((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;
                     }
                 }
             }
@@ -1077,11 +1083,10 @@ private bool AddIfNotPresent(T value, out int location)
             uint collisionCount = 0;
             ref int bucket = ref Unsafe.NullRef<int>();
 
-            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)
             {
-                Debug.Assert(typeof(T).IsValueType);
-
-                hashCode = value != null ? value.GetHashCode() : 0;
+                hashCode = value!.GetHashCode();
                 bucket = ref GetBucketRef(hashCode);
                 int i = bucket - 1; // Value in _buckets is 1-based
 
@@ -1106,6 +1111,7 @@ private bool AddIfNotPresent(T value, out int location)
             }
             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/NonRandomizedStringEqualityComparer.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/NonRandomizedStringEqualityComparer.cs
index 7785668a68ae76..ebc976d6b89f99 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
@@ -113,15 +113,18 @@ internal override RandomizedStringEqualityComparer GetRandomizedEqualityComparer
             // Special-case EqualityComparer<string>.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 (ReferenceEquals(comparer, EqualityComparer<string>.Default))
+
+            if (comparer.Equals(EqualityComparer<string>.Default))
             {
                 return WrappedAroundDefaultComparer;
             }
-            else if (ReferenceEquals(comparer, StringComparer.Ordinal))
+
+            if (comparer.Equals(StringComparer.Ordinal))
             {
                 return WrappedAroundStringComparerOrdinal;
             }
-            else if (ReferenceEquals(comparer, StringComparer.OrdinalIgnoreCase))
+
+            if (comparer.Equals(StringComparer.OrdinalIgnoreCase))
             {
                 return WrappedAroundStringComparerOrdinalIgnoreCase;
             }

From 985191179cd2dd6bb2c3cd86253e7d045b3b5b7a Mon Sep 17 00:00:00 2001
From: Stephen Toub <stoub@microsoft.com>
Date: Thu, 15 Sep 2022 16:20:09 -0400
Subject: [PATCH 3/5] Fix one more existing TODO

---
 .../src/System/Collections/Generic/EqualityComparer.cs         | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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 297c5288cfe257..e8f5ff1a2df579 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
@@ -133,9 +133,8 @@ public override bool Equals(T? x, T? y)
         public override bool Equals([NotNullWhen(true)] object? obj) =>
             obj != null && GetType() == obj.GetType();
 
-        // If in the future this type is made sealed, change typeof(...) to GetType().
         public override int GetHashCode() =>
-            typeof(GenericEqualityComparer<T>).GetHashCode();
+            GetType().GetHashCode();
     }
 
     [Serializable]

From 3a27103b8cb1e864b642e7f7f22fa2eec7608c19 Mon Sep 17 00:00:00 2001
From: Stephen Toub <stoub@microsoft.com>
Date: Thu, 15 Sep 2022 20:36:03 -0400
Subject: [PATCH 4/5] Revert previous PR feedback changes

---
 .../Generic/NonRandomizedStringEqualityComparer.cs          | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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 ebc976d6b89f99..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
@@ -114,17 +114,17 @@ internal override RandomizedStringEqualityComparer GetRandomizedEqualityComparer
             // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the
             // hash buckets become unbalanced.
 
-            if (comparer.Equals(EqualityComparer<string>.Default))
+            if (ReferenceEquals(comparer, EqualityComparer<string>.Default))
             {
                 return WrappedAroundDefaultComparer;
             }
 
-            if (comparer.Equals(StringComparer.Ordinal))
+            if (ReferenceEquals(comparer, StringComparer.Ordinal))
             {
                 return WrappedAroundStringComparerOrdinal;
             }
 
-            if (comparer.Equals(StringComparer.OrdinalIgnoreCase))
+            if (ReferenceEquals(comparer, StringComparer.OrdinalIgnoreCase))
             {
                 return WrappedAroundStringComparerOrdinalIgnoreCase;
             }

From b746387789781c084b4b52e1046efea38d3dd017 Mon Sep 17 00:00:00 2001
From: Stephen Toub <stoub@microsoft.com>
Date: Thu, 15 Sep 2022 20:42:27 -0400
Subject: [PATCH 5/5] Address PR feedback

---
 .../System/Collections/Generic/Dictionary.cs  | 24 ++++++++++---------
 .../Collections/Generic/EqualityComparer.cs   | 15 ++++++++----
 .../src/System/Collections/Generic/HashSet.cs | 18 +++++++-------
 3 files changed, 32 insertions(+), 25 deletions(-)

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 75e856ec0f3bef..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
@@ -62,21 +62,21 @@ public Dictionary(int capacity, IEqualityComparer<TKey>? comparer)
             if (!typeof(TKey).IsValueType)
             {
                 _comparer = comparer ?? EqualityComparer<TKey>.Default;
+
+                // Special-case EqualityComparer<string>.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<string> stringComparer)
+                {
+                    _comparer = (IEqualityComparer<TKey>)stringComparer;
+                }
             }
             else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily
                      comparer != EqualityComparer<TKey>.Default)
             {
                 _comparer = comparer;
             }
-
-            // Special-case EqualityComparer<string>.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<string> stringComparer)
-            {
-                _comparer = (IEqualityComparer<TKey>)stringComparer;
-            }
         }
 
         public Dictionary(IDictionary<TKey, TValue> dictionary) : this(dictionary, null) { }
@@ -888,7 +888,8 @@ public bool Remove(TKey key)
                 {
                     ref Entry entry = ref entries[i];
 
-                    if (entry.hashCode == hashCode && (comparer?.Equals(entry.key, key) ?? EqualityComparer<TKey>.Default.Equals(entry.key, key)))
+                    if (entry.hashCode == hashCode &&
+                        (typeof(TKey).IsValueType && comparer == null ? EqualityComparer<TKey>.Default.Equals(entry.key, key) : comparer!.Equals(entry.key, key)))
                     {
                         if (last < 0)
                         {
@@ -960,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<TKey>.Default.Equals(entry.key, key)))
+                    if (entry.hashCode == hashCode &&
+                        (typeof(TKey).IsValueType && comparer == null ? EqualityComparer<TKey>.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 e8f5ff1a2df579..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<T?, T?, bool> equals, Func<T, int> 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<T> other &&
@@ -127,7 +129,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) =>
@@ -166,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) =>
@@ -194,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) =>
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 39a1cc9906e14a..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
@@ -62,21 +62,21 @@ public HashSet(IEqualityComparer<T>? comparer)
             if (!typeof(T).IsValueType)
             {
                 _comparer = comparer ?? EqualityComparer<T>.Default;
+
+                // Special-case EqualityComparer<string>.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<string> stringComparer)
+                {
+                    _comparer = (IEqualityComparer<T>)stringComparer;
+                }
             }
             else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily
                      comparer != EqualityComparer<T>.Default)
             {
                 _comparer = comparer;
             }
-
-            // Special-case EqualityComparer<string>.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<string> stringComparer)
-            {
-                _comparer = (IEqualityComparer<T>)stringComparer;
-            }
         }
 
         public HashSet(int capacity) : this(capacity, null) { }