From 85c82a94567ceff82ce00ba6f6a971afbc6048a7 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Fri, 20 Mar 2026 22:23:59 -0600 Subject: [PATCH 1/2] Split Dictionary.Remove inner loops for value-type key optimization Split both Remove(TKey) and Remove(TKey, out TValue) inner loops into separate value-type and comparer paths, matching the existing pattern in FindValue, TryInsert, and CollectionsMarshalHelper. The previous single-loop approach used an inline ternary to choose between EqualityComparer.Default.Equals and comparer.Equals, which forced the register allocator to plan for the virtual comparer call even on the value-type path. This caused 3 unnecessary stack spills and reloads per iteration for value-type keys. With split loops, the value-type path has no virtual calls, allowing the JIT to keep all loop variables in registers. Codegen analysis shows the value-type inner loop shrinks from ~133 to ~56 bytes with zero stack traffic vs 6 memory operations per iteration. Benchmark results (MannWhitney statistical test, threshold=3ns): - Guid Remove/TryRemove hit: 8-18% faster (all sizes) - Int32 Remove/TryRemove hit: 4-15% faster (most sizes) - String (ref type): neutral (as expected) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../System/Collections/Generic/Dictionary.cs | 232 +++++++++++++----- 1 file changed, 170 insertions(+), 62 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 f6b9e545c40509..a5b80830fd3a00 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 @@ -1294,6 +1294,7 @@ public bool Remove(TKey key) // The overload Remove(TKey key, out TValue value) is a copy of this method with one additional // statement to copy the value for entry being removed into the output parameter. // Code has been intentionally duplicated for performance reasons. + // If you make any changes here, make sure to keep that version in sync as well. if (key == null) { @@ -1313,49 +1314,101 @@ public bool Remove(TKey key) Entry[]? entries = _entries; int last = -1; int i = bucket - 1; // Value in buckets is 1-based - while (i >= 0) - { - ref Entry entry = ref entries[i]; - if (entry.hashCode == hashCode && - (typeof(TKey).IsValueType && comparer == null ? EqualityComparer.Default.Equals(entry.key, key) : comparer!.Equals(entry.key, key))) + if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) + { + while (i >= 0) { - if (last < 0) - { - bucket = entry.next + 1; // Value in buckets is 1-based - } - else + ref Entry entry = ref entries[i]; + + if (entry.hashCode == hashCode && EqualityComparer.Default.Equals(entry.key, key)) { - entries[last].next = entry.next; + if (last < 0) + { + bucket = entry.next + 1; // Value in buckets is 1-based + } + else + { + entries[last].next = entry.next; + } + + Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); + entry.next = StartOfFreeList - _freeList; + + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.key = default!; + } + + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.value = default!; + } + + _freeList = i; + _freeCount++; + return true; } - Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); - entry.next = StartOfFreeList - _freeList; + last = i; + i = entry.next; - if (RuntimeHelpers.IsReferenceOrContainsReferences()) + collisionCount++; + if (collisionCount > (uint)entries.Length) { - entry.key = default!; + // 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 (i >= 0) + { + ref Entry entry = ref entries[i]; - if (RuntimeHelpers.IsReferenceOrContainsReferences()) + if (entry.hashCode == hashCode && comparer.Equals(entry.key, key)) { - entry.value = default!; - } + if (last < 0) + { + bucket = entry.next + 1; // Value in buckets is 1-based + } + else + { + entries[last].next = entry.next; + } - _freeList = i; - _freeCount++; - return true; - } + Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); + entry.next = StartOfFreeList - _freeList; - last = i; - i = entry.next; + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.key = default!; + } - 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(); + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.value = default!; + } + + _freeList = i; + _freeCount++; + return true; + } + + last = i; + i = entry.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(); + } } } } @@ -1367,6 +1420,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) // This overload is a copy of the overload Remove(TKey key) with one additional // statement to copy the value for entry being removed into the output parameter. // Code has been intentionally duplicated for performance reasons. + // If you make any changes here, make sure to keep the other overload in sync as well. if (key == null) { @@ -1386,51 +1440,105 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) Entry[]? entries = _entries; int last = -1; int i = bucket - 1; // Value in buckets is 1-based - while (i >= 0) - { - ref Entry entry = ref entries[i]; - if (entry.hashCode == hashCode && - (typeof(TKey).IsValueType && comparer == null ? EqualityComparer.Default.Equals(entry.key, key) : comparer!.Equals(entry.key, key))) + if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types + comparer == null) + { + while (i >= 0) { - if (last < 0) - { - bucket = entry.next + 1; // Value in buckets is 1-based - } - else + ref Entry entry = ref entries[i]; + + if (entry.hashCode == hashCode && EqualityComparer.Default.Equals(entry.key, key)) { - entries[last].next = entry.next; - } + if (last < 0) + { + bucket = entry.next + 1; // Value in buckets is 1-based + } + else + { + entries[last].next = entry.next; + } - value = entry.value; + value = entry.value; - Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); - entry.next = StartOfFreeList - _freeList; + Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); + entry.next = StartOfFreeList - _freeList; - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - { - entry.key = default!; + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.key = default!; + } + + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.value = default!; + } + + _freeList = i; + _freeCount++; + return true; } - if (RuntimeHelpers.IsReferenceOrContainsReferences()) + last = i; + i = entry.next; + + collisionCount++; + if (collisionCount > (uint)entries.Length) { - entry.value = default!; + // 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(); } - - _freeList = i; - _freeCount++; - return true; } + } + else + { + Debug.Assert(comparer is not null); + while (i >= 0) + { + ref Entry entry = ref entries[i]; + + if (entry.hashCode == hashCode && comparer.Equals(entry.key, key)) + { + if (last < 0) + { + bucket = entry.next + 1; // Value in buckets is 1-based + } + else + { + entries[last].next = entry.next; + } - last = i; - i = entry.next; + value = entry.value; - 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(); + Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); + entry.next = StartOfFreeList - _freeList; + + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.key = default!; + } + + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.value = default!; + } + + _freeList = i; + _freeCount++; + return true; + } + + last = i; + i = entry.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(); + } } } } From f24405a798f729319436cede625ac15fe120ab61 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Sun, 29 Mar 2026 14:02:11 -0600 Subject: [PATCH 2/2] Use (uint)i < (uint)entries.Length pattern consistently in Remove methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/System/Collections/Generic/Dictionary.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 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 a5b80830fd3a00..9fa26ab0332e36 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 @@ -885,7 +885,7 @@ public bool Remove(TAlternateKey key, [MaybeNullWhen(false)] out TKey actualKey, Entry[]? entries = dictionary._entries; int last = -1; int i = bucket - 1; // Value in buckets is 1-based - while (i >= 0) + while ((uint)i < (uint)entries.Length) { ref Entry entry = ref entries[i]; @@ -1318,7 +1318,7 @@ public bool Remove(TKey key) if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types comparer == null) { - while (i >= 0) + while ((uint)i < (uint)entries.Length) { ref Entry entry = ref entries[i]; @@ -1366,7 +1366,7 @@ public bool Remove(TKey key) else { Debug.Assert(comparer is not null); - while (i >= 0) + while ((uint)i < (uint)entries.Length) { ref Entry entry = ref entries[i]; @@ -1444,7 +1444,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) if (typeof(TKey).IsValueType && // comparer can only be null for value types; enable JIT to eliminate entire if block for ref types comparer == null) { - while (i >= 0) + while ((uint)i < (uint)entries.Length) { ref Entry entry = ref entries[i]; @@ -1494,7 +1494,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) else { Debug.Assert(comparer is not null); - while (i >= 0) + while ((uint)i < (uint)entries.Length) { ref Entry entry = ref entries[i];