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..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]; @@ -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 ((uint)i < (uint)entries.Length) { - 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 ((uint)i < (uint)entries.Length) + { + 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 ((uint)i < (uint)entries.Length) { - 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 ((uint)i < (uint)entries.Length) + { + 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(); + } } } }