From b6f24a15691d7b604fd11a8c13ff971bcf6cd432 Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Wed, 24 Feb 2021 16:05:32 -0600 Subject: [PATCH 1/8] Removed null checks of buckets and entries in Dictionary and HashSet at the cost of some constructor performance. --- .../System/Collections/Generic/Dictionary.cs | 411 +++++++++--------- .../src/System/Collections/Generic/HashSet.cs | 375 ++++++++-------- .../src/System/Collections/HashHelpers.cs | 3 + 3 files changed, 382 insertions(+), 407 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 798b3d08c9453..247fc1dc653f3 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 @@ -22,8 +22,8 @@ public class Dictionary : IDictionary, IDictionary, private const string KeyValuePairsName = "KeyValuePairs"; // Do not rename (binary serialization) private const string ComparerName = "Comparer"; // Do not rename (binary serialization) - private int[]? _buckets; - private Entry[]? _entries; + private int[] _buckets; + private Entry[] _entries; #if TARGET_64BIT private ulong _fastModMultiplier; #endif @@ -51,8 +51,14 @@ public Dictionary(int capacity, IEqualityComparer? comparer) if (capacity > 0) { - Initialize(capacity); + Initialize(HashHelpers.GetPrime(capacity)); } + else + { + InitializeEmpty(); + } + Debug.Assert(_buckets != null); + Debug.Assert(_entries != null); if (comparer != null && comparer != EqualityComparer.Default) // first check for null to avoid forcing default comparer instantiation unnecessarily { @@ -124,8 +130,6 @@ private void AddRange(IEnumerable> collection) // This is not currently a true .AddRange as it needs to be an initalized dictionary // of the correct size, and also an empty dictionary with no current entities (and no argument checks). - Debug.Assert(source._entries is not null); - Debug.Assert(_entries is not null); Debug.Assert(_entries.Length >= source.Count); Debug.Assert(_count == 0); @@ -163,6 +167,9 @@ protected Dictionary(SerializationInfo info, StreamingContext context) // and we have a resonable estimate that GetHashCode is not going to fail. For the time being, // we'll just cache this. The graph is not valid until OnDeserialization has been called. HashHelpers.SerializationInfoTable.Add(this, info); + InitializeEmpty(); + Debug.Assert(_buckets != null); + Debug.Assert(_entries != null); } public IEqualityComparer Comparer @@ -251,9 +258,6 @@ public void Clear() int count = _count; if (count > 0) { - Debug.Assert(_buckets != null, "_buckets should be non-null"); - Debug.Assert(_entries != null, "_entries should be non-null"); - Array.Clear(_buckets, 0, _buckets.Length); _count = 0; @@ -268,12 +272,12 @@ public bool ContainsKey(TKey key) => public bool ContainsValue(TValue value) { - Entry[]? entries = _entries; + Entry[] entries = _entries; if (value == null) { for (int i = 0; i < _count; i++) { - if (entries![i].next >= -1 && entries[i].value == null) + if (entries[i].next >= -1 && entries[i].value == null) { return true; } @@ -284,7 +288,7 @@ public bool ContainsValue(TValue value) // ValueType: Devirtualize with EqualityComparer.Default intrinsic for (int i = 0; i < _count; i++) { - if (entries![i].next >= -1 && EqualityComparer.Default.Equals(entries[i].value, value)) + if (entries[i].next >= -1 && EqualityComparer.Default.Equals(entries[i].value, value)) { return true; } @@ -298,7 +302,7 @@ public bool ContainsValue(TValue value) EqualityComparer defaultComparer = EqualityComparer.Default; for (int i = 0; i < _count; i++) { - if (entries![i].next >= -1 && defaultComparer.Equals(entries[i].value, value)) + if (entries[i].next >= -1 && defaultComparer.Equals(entries[i].value, value)) { return true; } @@ -326,10 +330,10 @@ private void CopyTo(KeyValuePair[] array, int index) } int count = _count; - Entry[]? entries = _entries; + Entry[] entries = _entries; for (int i = 0; i < count; i++) { - if (entries![i].next >= -1) + if (entries[i].next >= -1) { array[index++] = new KeyValuePair(entries[i].key, entries[i].value); } @@ -350,9 +354,9 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte info.AddValue(VersionName, _version); info.AddValue(ComparerName, Comparer, typeof(IEqualityComparer)); - info.AddValue(HashSizeName, _buckets == null ? 0 : _buckets.Length); // This is the length of the bucket array + info.AddValue(HashSizeName, _entries.Length); // This is the length of the entry array - if (_buckets != null) + if (_entries.Length > 0) { var array = new KeyValuePair[Count]; CopyTo(array, 0); @@ -368,84 +372,49 @@ private ref TValue FindValue(TKey key) } ref Entry entry = ref Unsafe.NullRef(); - if (_buckets != null) + IEqualityComparer? comparer = _comparer; + if (comparer == null) { - Debug.Assert(_entries != null, "expected entries to be != null"); - IEqualityComparer? comparer = _comparer; - if (comparer == null) + uint hashCode = (uint)key.GetHashCode(); + int i = GetBucket(hashCode); + Entry[] entries = _entries; + uint collisionCount = 0; + if (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.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); + // ValueType: Devirtualize with EqualityComparer.Default intrinsic - // 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 + 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 { - uint hashCode = (uint)comparer.GetHashCode(key); - int i = GetBucket(hashCode); - Entry[]? entries = _entries; - uint collisionCount = 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; + i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. do { @@ -457,7 +426,7 @@ private ref TValue FindValue(TKey key) } entry = ref entries[i]; - if (entry.hashCode == hashCode && comparer.Equals(entry.key, key)) + if (entry.hashCode == hashCode && defaultComparer.Equals(entry.key, key)) { goto ReturnFound; } @@ -472,8 +441,37 @@ private ref TValue FindValue(TKey key) goto ConcurrentOperation; } } + else + { + uint hashCode = (uint)comparer.GetHashCode(key); + int i = GetBucket(hashCode); + Entry[] entries = _entries; + uint collisionCount = 0; + 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 && comparer.Equals(entry.key, key)) + { + goto ReturnFound; + } - goto ReturnNotFound; + 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; + } ConcurrentOperation: ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); @@ -486,9 +484,8 @@ private ref TValue FindValue(TKey key) goto Return; } - private int Initialize(int capacity) + private void Initialize(int size) { - int size = HashHelpers.GetPrime(capacity); int[] buckets = new int[size]; Entry[] entries = new Entry[size]; @@ -499,8 +496,16 @@ private int Initialize(int capacity) #endif _buckets = buckets; _entries = entries; + } - return size; + private void InitializeEmpty() + { + _freeList = -1; +#if TARGET_64BIT + _fastModMultiplier = HashHelpers.GetFastModMultiplier(0); +#endif + _buckets = HashHelpers.SizeOneIntArray; + _entries = Array.Empty(); } private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) @@ -510,14 +515,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - if (_buckets == null) - { - Initialize(0); - } - Debug.Assert(_buckets != null); - - Entry[]? entries = _entries; - Debug.Assert(entries != null, "expected entries to be non-null"); + Entry[] entries = _entries; IEqualityComparer? comparer = _comparer; uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); @@ -664,13 +662,13 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) { Resize(); bucket = ref GetBucket(hashCode); + entries = _entries; } index = count; _count = count + 1; - entries = _entries; } - ref Entry entry = ref entries![index]; + ref Entry entry = ref entries[index]; entry.hashCode = hashCode; entry.next = bucket - 1; // Value in _buckets is 1-based entry.key = key; @@ -706,7 +704,7 @@ public virtual void OnDeserialization(object? sender) if (hashsize != 0) { - Initialize(hashsize); + Initialize(HashHelpers.GetPrime(hashsize)); KeyValuePair[]? array = (KeyValuePair[]?) siInfo.GetValue(KeyValuePairsName, typeof(KeyValuePair[])); @@ -728,7 +726,7 @@ public virtual void OnDeserialization(object? sender) } else { - _buckets = null; + InitializeEmpty(); } _version = realVersion; @@ -741,7 +739,6 @@ private void Resize(int newSize, bool forceNewHashCodes) { // Value types never rehash Debug.Assert(!forceNewHashCodes || !typeof(TKey).IsValueType); - Debug.Assert(_entries != null, "_entries should be non-null"); Debug.Assert(newSize >= _entries.Length); Entry[] entries = new Entry[newSize]; @@ -797,58 +794,54 @@ public bool Remove(TKey key) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - if (_buckets != null) + uint collisionCount = 0; + uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); + ref int bucket = ref GetBucket(hashCode); + Entry[] entries = _entries; + int last = -1; + int i = bucket - 1; // Value in buckets is 1-based + while (i >= 0) { - Debug.Assert(_entries != null, "entries should be non-null"); - uint collisionCount = 0; - uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - ref int bucket = ref GetBucket(hashCode); - 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]; + 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 && (_comparer?.Equals(entry.key, key) ?? EqualityComparer.Default.Equals(entry.key, key))) + { + if (last < 0) { - 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; + bucket = entry.next + 1; // Value in buckets is 1-based + } + else + { + entries[last].next = entry.next; + } - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - { - entry.key = default!; - } + 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.value = default!; - } - - _freeList = i; - _freeCount++; - return true; + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.key = default!; } - last = i; - i = entry.next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) + if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - // 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(); + 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(); } } return false; @@ -865,60 +858,56 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - if (_buckets != null) + uint collisionCount = 0; + uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); + ref int bucket = ref GetBucket(hashCode); + Entry[] entries = _entries; + int last = -1; + int i = bucket - 1; // Value in buckets is 1-based + while (i >= 0) { - Debug.Assert(_entries != null, "entries should be non-null"); - uint collisionCount = 0; - uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - ref int bucket = ref GetBucket(hashCode); - 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]; + 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 && (_comparer?.Equals(entry.key, key) ?? EqualityComparer.Default.Equals(entry.key, key))) + { + if (last < 0) { - if (last < 0) - { - bucket = entry.next + 1; // Value in buckets is 1-based - } - else - { - entries[last].next = entry.next; - } - - 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; + bucket = entry.next + 1; // Value in buckets is 1-based + } + else + { + entries[last].next = entry.next; + } - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - { - entry.key = default!; - } + value = entry.value; - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - { - entry.value = default!; - } + Debug.Assert((StartOfFreeList - _freeList) < 0, "shouldn't underflow because max hashtable length is MaxPrimeArrayLength = 0x7FEFFFFD(2146435069) _freelist underflow threshold 2147483646"); + entry.next = StartOfFreeList - _freeList; - _freeList = i; - _freeCount++; - return true; + if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + entry.key = default!; } - last = i; - i = entry.next; - - collisionCount++; - if (collisionCount > (uint)entries.Length) + if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - // 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(); + 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(); } } @@ -980,10 +969,10 @@ void ICollection.CopyTo(Array array, int index) } else if (array is DictionaryEntry[] dictEntryArray) { - Entry[]? entries = _entries; + Entry[] entries = _entries; for (int i = 0; i < _count; i++) { - if (entries![i].next >= -1) + if (entries[i].next >= -1) { dictEntryArray[index++] = new DictionaryEntry(entries[i].key, entries[i].value); } @@ -1000,10 +989,10 @@ void ICollection.CopyTo(Array array, int index) try { int count = _count; - Entry[]? entries = _entries; + Entry[] entries = _entries; for (int i = 0; i < count; i++) { - if (entries![i].next >= -1) + if (entries[i].next >= -1) { objects[index++] = new KeyValuePair(entries[i].key, entries[i].value); } @@ -1028,7 +1017,7 @@ public int EnsureCapacity(int capacity) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); } - int currentCapacity = _entries == null ? 0 : _entries.Length; + int currentCapacity = _entries.Length; if (currentCapacity >= capacity) { return currentCapacity; @@ -1036,11 +1025,6 @@ public int EnsureCapacity(int capacity) _version++; - if (_buckets == null) - { - return Initialize(capacity); - } - int newSize = HashHelpers.GetPrime(capacity); Resize(newSize, forceNewHashCodes: false); return newSize; @@ -1075,9 +1059,8 @@ public void TrimExcess(int capacity) } int newSize = HashHelpers.GetPrime(capacity); - Entry[]? oldEntries = _entries; - int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; - if (newSize >= currentCapacity) + Entry[] oldEntries = _entries; + if (newSize >= oldEntries.Length) { return; } @@ -1086,15 +1069,11 @@ public void TrimExcess(int capacity) _version++; Initialize(newSize); - Debug.Assert(oldEntries is not null); - CopyEntries(oldEntries, oldCount); } private void CopyEntries(Entry[] entries, int count) { - Debug.Assert(_entries is not null); - Entry[] newEntries = _entries; int newCount = 0; for (int i = 0; i < count; i++) @@ -1228,7 +1207,7 @@ void IDictionary.Remove(object key) [MethodImpl(MethodImplOptions.AggressiveInlining)] private ref int GetBucket(uint hashCode) { - int[] buckets = _buckets!; + int[] buckets = _buckets; #if TARGET_64BIT return ref buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; #else @@ -1280,7 +1259,7 @@ public bool MoveNext() // dictionary.count+1 could be negative if dictionary.count is int.MaxValue while ((uint)_index < (uint)_dictionary._count) { - ref Entry entry = ref _dictionary._entries![_index++]; + ref Entry entry = ref _dictionary._entries[_index++]; if (entry.next >= -1) { @@ -1403,10 +1382,10 @@ public void CopyTo(TKey[] array, int index) } int count = _dictionary._count; - Entry[]? entries = _dictionary._entries; + Entry[] entries = _dictionary._entries; for (int i = 0; i < count; i++) { - if (entries![i].next >= -1) array[index++] = entries[i].key; + if (entries[i].next >= -1) array[index++] = entries[i].key; } } @@ -1473,12 +1452,12 @@ void ICollection.CopyTo(Array array, int index) } int count = _dictionary._count; - Entry[]? entries = _dictionary._entries; + Entry[] entries = _dictionary._entries; try { for (int i = 0; i < count; i++) { - if (entries![i].next >= -1) objects[index++] = entries[i].key; + if (entries[i].next >= -1) objects[index++] = entries[i].key; } } catch (ArrayTypeMismatchException) @@ -1518,7 +1497,7 @@ public bool MoveNext() while ((uint)_index < (uint)_dictionary._count) { - ref Entry entry = ref _dictionary._entries![_index++]; + ref Entry entry = ref _dictionary._entries[_index++]; if (entry.next >= -1) { @@ -1596,10 +1575,10 @@ public void CopyTo(TValue[] array, int index) } int count = _dictionary._count; - Entry[]? entries = _dictionary._entries; + Entry[] entries = _dictionary._entries; for (int i = 0; i < count; i++) { - if (entries![i].next >= -1) array[index++] = entries[i].value; + if (entries[i].next >= -1) array[index++] = entries[i].value; } } @@ -1665,12 +1644,12 @@ void ICollection.CopyTo(Array array, int index) } int count = _dictionary._count; - Entry[]? entries = _dictionary._entries; + Entry[] entries = _dictionary._entries; try { for (int i = 0; i < count; i++) { - if (entries![i].next >= -1) objects[index++] = entries[i].value!; + if (entries[i].next >= -1) objects[index++] = entries[i].value!; } } catch (ArrayTypeMismatchException) @@ -1710,7 +1689,7 @@ public bool MoveNext() while ((uint)_index < (uint)_dictionary._count) { - ref Entry entry = ref _dictionary._entries![_index++]; + ref Entry entry = ref _dictionary._entries[_index++]; if (entry.next >= -1) { 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 d3516d66aca91..5255ef913b39b 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 @@ -37,8 +37,8 @@ public class HashSet : ICollection, ISet, IReadOnlyCollection, IRead private const int ShrinkThreshold = 3; private const int StartOfFreeList = -3; - private int[]? _buckets; - private Entry[]? _entries; + private int[] _buckets; + private Entry[] _entries; #if TARGET_64BIT private ulong _fastModMultiplier; #endif @@ -50,83 +50,101 @@ public class HashSet : ICollection, ISet, IReadOnlyCollection, IRead #region Constructors - public HashSet() : this((IEqualityComparer?)null) { } + public HashSet() : this(0, null) { } - public HashSet(IEqualityComparer? comparer) - { - if (comparer != 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)) - { - if (_comparer is null) - { - _comparer = (IEqualityComparer)NonRandomizedStringEqualityComparer.WrappedAroundDefaultComparer; - } - else if (ReferenceEquals(_comparer, StringComparer.Ordinal)) - { - _comparer = (IEqualityComparer)NonRandomizedStringEqualityComparer.WrappedAroundStringComparerOrdinal; - } - else if (ReferenceEquals(_comparer, StringComparer.OrdinalIgnoreCase)) - { - _comparer = (IEqualityComparer)NonRandomizedStringEqualityComparer.WrappedAroundStringComparerOrdinalIgnoreCase; - } - } - } + public HashSet(IEqualityComparer? comparer) : this(0, comparer) { } public HashSet(int capacity) : this(capacity, null) { } public HashSet(IEnumerable collection) : this(collection, null) { } - public HashSet(IEnumerable collection, IEqualityComparer? comparer) : this(comparer) + public HashSet(IEnumerable collection, IEqualityComparer? comparer) { if (collection == null) { ThrowHelper.ThrowArgumentNullException(ExceptionArgument.collection); } + InitializeComparer(comparer); + if (collection is HashSet otherAsHashSet && EqualityComparersAreEqual(this, otherAsHashSet)) { ConstructFrom(otherAsHashSet); + Debug.Assert(_buckets != null); + Debug.Assert(_entries != null); } else { // To avoid excess resizes, first set size based on collection's count. The collection may // contain duplicates, so call TrimExcess if resulting HashSet is larger than the threshold. - if (collection is ICollection coll) + int count = (collection as ICollection)?.Count ?? 0; + if (count > 0) { - int count = coll.Count; - if (count > 0) - { - Initialize(count); - } + Initialize(HashHelpers.GetPrime(count)); + } + else + { + InitializeEmpty(); } + Debug.Assert(_buckets != null); + Debug.Assert(_entries != null); UnionWith(collection); - if (_count > 0 && _entries!.Length / _count > ShrinkThreshold) + if (_count > 0 && _entries.Length / _count > ShrinkThreshold) { TrimExcess(); } } } - public HashSet(int capacity, IEqualityComparer? comparer) : this(comparer) + public HashSet(int capacity, IEqualityComparer? comparer) { if (capacity < 0) { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); } + InitializeComparer(comparer); + if (capacity > 0) { - Initialize(capacity); + Initialize(HashHelpers.GetPrime(capacity)); + } + else + { + InitializeEmpty(); + } + Debug.Assert(_buckets != null); + Debug.Assert(_entries != null); + } + + // Only to be called from constructor + private void InitializeComparer(IEqualityComparer? comparer) + { + if (comparer != 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)) + { + if (_comparer is null) + { + _comparer = (IEqualityComparer)NonRandomizedStringEqualityComparer.WrappedAroundDefaultComparer; + } + else if (ReferenceEquals(_comparer, StringComparer.Ordinal)) + { + _comparer = (IEqualityComparer)NonRandomizedStringEqualityComparer.WrappedAroundStringComparerOrdinal; + } + else if (ReferenceEquals(_comparer, StringComparer.OrdinalIgnoreCase)) + { + _comparer = (IEqualityComparer)NonRandomizedStringEqualityComparer.WrappedAroundStringComparerOrdinalIgnoreCase; + } } } @@ -137,6 +155,9 @@ protected HashSet(SerializationInfo info, StreamingContext context) // fail. For the time being, we'll just cache this. The graph is not valid until // OnDeserialization has been called. HashHelpers.SerializationInfoTable.Add(this, info); + InitializeEmpty(); + Debug.Assert(_buckets != null); + Debug.Assert(_entries != null); } /// Initializes the HashSet from another HashSet with the same element type and equality comparer. @@ -144,19 +165,17 @@ private void ConstructFrom(HashSet source) { if (source.Count == 0) { - // As well as short-circuiting on the rest of the work done, - // this avoids errors from trying to access source._buckets - // or source._entries when they aren't initialized. + InitializeEmpty(); return; } - int capacity = source._buckets!.Length; + int capacity = source._entries.Length; int threshold = HashHelpers.ExpandPrime(source.Count + 1); if (threshold >= capacity) { _buckets = (int[])source._buckets.Clone(); - _entries = (Entry[])source._entries!.Clone(); + _entries = (Entry[])source._entries.Clone(); _freeList = source._freeList; _freeCount = source._freeCount; _count = source._count; @@ -166,12 +185,12 @@ private void ConstructFrom(HashSet source) } else { - Initialize(source.Count); + Initialize(HashHelpers.GetPrime(source.Count)); - Entry[]? entries = source._entries; + Entry[] entries = source._entries; for (int i = 0; i < source._count; i++) { - ref Entry entry = ref entries![i]; + ref Entry entry = ref entries[i]; if (entry.Next >= -1) { AddIfNotPresent(entry.Value, out _); @@ -194,9 +213,6 @@ public void Clear() int count = _count; if (count > 0) { - Debug.Assert(_buckets != null, "_buckets should be non-null"); - Debug.Assert(_entries != null, "_entries should be non-null"); - Array.Clear(_buckets, 0, _buckets.Length); _count = 0; _freeList = -1; @@ -213,71 +229,45 @@ public void Clear() /// Gets the index of the item in , or -1 if it's not in the set. private int FindItemIndex(T item) { - int[]? buckets = _buckets; - if (buckets != null) - { - Entry[]? entries = _entries; - Debug.Assert(entries != null, "Expected _entries to be initialized"); + Entry[] entries = _entries; - uint collisionCount = 0; - IEqualityComparer? comparer = _comparer; + uint collisionCount = 0; + IEqualityComparer? comparer = _comparer; - if (comparer == null) + if (comparer == null) + { + int hashCode = item != null ? item.GetHashCode() : 0; + if (typeof(T).IsValueType) { - int hashCode = item != null ? item.GetHashCode() : 0; - if (typeof(T).IsValueType) + // ValueType: Devirtualize with EqualityComparer.Default intrinsic + 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 { - int hashCode = item != null ? comparer.GetHashCode(item) : 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; int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based while (i >= 0) { ref Entry entry = ref entries[i]; - if (entry.HashCode == hashCode && comparer.Equals(entry.Value, item)) + if (entry.HashCode == hashCode && defaultComparer.Equals(entry.Value, item)) { return i; } @@ -292,6 +282,27 @@ private int FindItemIndex(T item) } } } + else + { + int hashCode = item != null ? comparer.GetHashCode(item) : 0; + int i = GetBucketRef(hashCode) - 1; // Value in _buckets is 1-based + while (i >= 0) + { + ref Entry entry = ref entries[i]; + if (entry.HashCode == hashCode && comparer.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 -1; } @@ -300,7 +311,7 @@ private int FindItemIndex(T item) [MethodImpl(MethodImplOptions.AggressiveInlining)] private ref int GetBucketRef(int hashCode) { - int[] buckets = _buckets!; + int[] buckets = _buckets; #if TARGET_64BIT return ref buckets[HashHelpers.FastMod((uint)hashCode, (uint)buckets.Length, _fastModMultiplier)]; #else @@ -310,56 +321,52 @@ private ref int GetBucketRef(int hashCode) public bool Remove(T item) { - if (_buckets != null) - { - Entry[]? entries = _entries; - Debug.Assert(entries != null, "entries should be non-null"); + Entry[] entries = _entries; - uint collisionCount = 0; - int last = -1; - int hashCode = item != null ? (_comparer?.GetHashCode(item) ?? item.GetHashCode()) : 0; + uint collisionCount = 0; + int last = -1; + int hashCode = item != null ? (_comparer?.GetHashCode(item) ?? item.GetHashCode()) : 0; - ref int bucket = ref GetBucketRef(hashCode); - int i = bucket - 1; // Value in buckets is 1-based + ref int bucket = ref GetBucketRef(hashCode); + int i = bucket - 1; // Value in buckets is 1-based - while (i >= 0) - { - ref Entry entry = ref entries[i]; + while (i >= 0) + { + 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) { - 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.Value = default!; - } - - _freeList = i; - _freeCount++; - return true; + bucket = entry.Next + 1; // Value in buckets is 1-based + } + else + { + entries[last].Next = entry.Next; } - last = i; - i = 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; - collisionCount++; - if (collisionCount > (uint)entries.Length) + if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - // 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(); + 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(); } } @@ -394,9 +401,9 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte info.AddValue(VersionName, _version); // need to serialize version to avoid problems with serializing while enumerating info.AddValue(ComparerName, Comparer, typeof(IEqualityComparer)); - info.AddValue(CapacityName, _buckets == null ? 0 : _buckets.Length); + info.AddValue(CapacityName, _entries.Length); - if (_buckets != null) + if (_entries.Length > 0) { var array = new T[Count]; CopyTo(array); @@ -446,7 +453,7 @@ public virtual void OnDeserialization(object? sender) } else { - _buckets = null; + InitializeEmpty(); } _version = siInfo.GetInt32(VersionName); @@ -474,14 +481,11 @@ public virtual void OnDeserialization(object? sender) /// public bool TryGetValue(T equalValue, [MaybeNullWhen(false)] out T actualValue) { - if (_buckets != null) + int index = FindItemIndex(equalValue); + if (index >= 0) { - int index = FindItemIndex(equalValue); - if (index >= 0) - { - actualValue = _entries![index].Value; - return true; - } + actualValue = _entries[index].Value; + return true; } actualValue = default; @@ -882,10 +886,10 @@ public void CopyTo(T[] array, int arrayIndex, int count) ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_ArrayPlusOffTooSmall); } - Entry[]? entries = _entries; + Entry[] entries = _entries; for (int i = 0; i < _count && count != 0; i++) { - ref Entry entry = ref entries![i]; + ref Entry entry = ref entries[i]; if (entry.Next >= -1) { array[arrayIndex++] = entry.Value; @@ -902,11 +906,11 @@ public int RemoveWhere(Predicate match) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.match); } - Entry[]? entries = _entries; + Entry[] entries = _entries; int numRemoved = 0; for (int i = 0; i < _count; i++) { - ref Entry entry = ref entries![i]; + ref Entry entry = ref entries[i]; if (entry.Next >= -1) { // Cache value in case delegate removes it @@ -949,17 +953,12 @@ public int EnsureCapacity(int capacity) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); } - int currentCapacity = _entries == null ? 0 : _entries.Length; + int currentCapacity = _entries.Length; if (currentCapacity >= capacity) { return currentCapacity; } - if (_buckets == null) - { - return Initialize(capacity); - } - int newSize = HashHelpers.GetPrime(capacity); Resize(newSize, forceNewHashCodes: false); return newSize; @@ -971,7 +970,6 @@ private void Resize(int newSize, bool forceNewHashCodes) { // Value types never rehash Debug.Assert(!forceNewHashCodes || !typeof(T).IsValueType); - Debug.Assert(_entries != null, "_entries should be non-null"); Debug.Assert(newSize >= _entries.Length); var entries = new Entry[newSize]; @@ -1027,9 +1025,8 @@ public void TrimExcess() int capacity = Count; int newSize = HashHelpers.GetPrime(capacity); - Entry[]? oldEntries = _entries; - int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; - if (newSize >= currentCapacity) + Entry[] oldEntries = _entries; + if (newSize >= oldEntries.Length) { return; } @@ -1037,14 +1034,14 @@ public void TrimExcess() int oldCount = _count; _version++; Initialize(newSize); - Entry[]? entries = _entries; + Entry[] entries = _entries; int count = 0; for (int i = 0; i < oldCount; i++) { - int hashCode = oldEntries![i].HashCode; // At this point, we know we have entries. + int hashCode = oldEntries[i].HashCode; // At this point, we know we have entries. if (oldEntries[i].Next >= -1) { - ref Entry entry = ref entries![count]; + ref Entry entry = ref entries[count]; entry = oldEntries[i]; ref int bucket = ref GetBucketRef(hashCode); entry.Next = bucket - 1; // Value in _buckets is 1-based @@ -1068,9 +1065,8 @@ public void TrimExcess() /// Initializes buckets and slots arrays. Uses suggested capacity by finding next prime /// greater than or equal to capacity. /// - private int Initialize(int capacity) + private void Initialize(int size) { - int size = HashHelpers.GetPrime(capacity); var buckets = new int[size]; var entries = new Entry[size]; @@ -1081,8 +1077,16 @@ private int Initialize(int capacity) #if TARGET_64BIT _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); #endif + } - return size; + private void InitializeEmpty() + { + _freeList = -1; +#if TARGET_64BIT + _fastModMultiplier = HashHelpers.GetFastModMultiplier(0); +#endif + _buckets = HashHelpers.SizeOneIntArray; + _entries = Array.Empty(); } /// Adds the specified element to the set if it's not already contained. @@ -1091,14 +1095,7 @@ private int Initialize(int capacity) /// true if the element is added to the object; false if the element is already present. private bool AddIfNotPresent(T value, out int location) { - if (_buckets == null) - { - Initialize(0); - } - Debug.Assert(_buckets != null); - - Entry[]? entries = _entries; - Debug.Assert(entries != null, "expected entries to be non-null"); + Entry[] entries = _entries; IEqualityComparer? comparer = _comparer; int hashCode; @@ -1185,7 +1182,7 @@ private bool AddIfNotPresent(T value, out int location) { index = _freeList; _freeCount--; - Debug.Assert((StartOfFreeList - entries![_freeList].Next) >= -1, "shouldn't overflow because `next` cannot underflow"); + Debug.Assert((StartOfFreeList - entries[_freeList].Next) >= -1, "shouldn't overflow because `next` cannot underflow"); _freeList = StartOfFreeList - entries[_freeList].Next; } else @@ -1195,14 +1192,14 @@ private bool AddIfNotPresent(T value, out int location) { Resize(); bucket = ref GetBucketRef(hashCode); + entries = _entries; } index = count; _count = count + 1; - entries = _entries; } { - ref Entry entry = ref entries![index]; + ref Entry entry = ref entries[index]; entry.HashCode = hashCode; entry.Next = bucket - 1; // Value in _buckets is 1-based entry.Value = value; @@ -1271,10 +1268,10 @@ internal bool IsSubsetOfHashSetWithSameComparer(HashSet other) /// private void IntersectWithHashSetWithSameComparer(HashSet other) { - Entry[]? entries = _entries; + Entry[] entries = _entries; for (int i = 0; i < _count; i++) { - ref Entry entry = ref entries![i]; + ref Entry entry = ref entries[i]; if (entry.Next >= -1) { T item = entry.Value; @@ -1294,8 +1291,6 @@ private void IntersectWithHashSetWithSameComparer(HashSet other) /// private unsafe void IntersectWithEnumerable(IEnumerable other) { - Debug.Assert(_buckets != null, "_buckets shouldn't be null; callers should check first"); - // Keep track of current last index; don't want to move past the end of our bit array // (could happen if another thread is modifying the collection). int originalCount = _count; @@ -1320,7 +1315,7 @@ private unsafe void IntersectWithEnumerable(IEnumerable other) // FindFirstUnmarked method. for (int i = 0; i < originalCount; i++) { - ref Entry entry = ref _entries![i]; + ref Entry entry = ref _entries[i]; if (entry.Next >= -1 && !bitHelper.IsMarked(i)) { Remove(entry.Value); @@ -1409,7 +1404,7 @@ private unsafe void SymmetricExceptWithEnumerable(IEnumerable other) { if (itemsToRemove.IsMarked(i)) { - Remove(_entries![i].Value); + Remove(_entries[i].Value); } } } @@ -1452,8 +1447,6 @@ private unsafe (int UniqueCount, int UnfoundCount) CheckUniqueAndUnfoundElements return (UniqueCount: 0, UnfoundCount: numElementsInOther); } - Debug.Assert((_buckets != null) && (_count > 0), "_buckets was null but count greater than 0"); - int originalCount = _count; int intArrayLength = BitHelper.ToIntArrayLength(originalCount); @@ -1537,7 +1530,7 @@ public bool MoveNext() // dictionary.count+1 could be negative if dictionary.count is int.MaxValue while ((uint)_index < (uint)_hashSet._count) { - ref Entry entry = ref _hashSet._entries![_index++]; + ref Entry entry = ref _hashSet._entries[_index++]; if (entry.Next >= -1) { _current = entry.Value; diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs index 8bb7d8ff95dd6..1292cb986eaea 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs @@ -15,6 +15,9 @@ internal static partial class HashHelpers public const int HashPrime = 101; + // must never be written to + public static readonly int[] SizeOneIntArray = new int[1]; + // Table of prime numbers to use as hash table sizes. // A typical resize algorithm would pick the smallest prime number in this array // that is larger than twice the previous capacity. From 8efb22d94ff585cd73be6f963bbd3be5a4bea6a7 Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Fri, 12 Mar 2021 15:13:48 -0600 Subject: [PATCH 2/8] Fix divide by zero bug --- .../src/System/Collections/Generic/Dictionary.cs | 2 +- .../src/System/Collections/Generic/HashSet.cs | 2 +- 2 files changed, 2 insertions(+), 2 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 247fc1dc653f3..dc0ac5e2a4850 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 @@ -502,7 +502,7 @@ private void InitializeEmpty() { _freeList = -1; #if TARGET_64BIT - _fastModMultiplier = HashHelpers.GetFastModMultiplier(0); + _fastModMultiplier = HashHelpers.GetFastModMultiplier(1); #endif _buckets = HashHelpers.SizeOneIntArray; _entries = Array.Empty(); 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 5255ef913b39b..975b91be12ff9 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 @@ -1083,7 +1083,7 @@ private void InitializeEmpty() { _freeList = -1; #if TARGET_64BIT - _fastModMultiplier = HashHelpers.GetFastModMultiplier(0); + _fastModMultiplier = HashHelpers.GetFastModMultiplier(1); #endif _buckets = HashHelpers.SizeOneIntArray; _entries = Array.Empty(); From 88c2fd61bc8bf1883ab7756d28413492afcc4d6a Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Mon, 15 Mar 2021 09:26:55 -0500 Subject: [PATCH 3/8] Cache fast mod multiplier for divisor of 1 and internally cache an empty array instead of using Array.Empty() since Entry is internal and to avoid the extra generic instantiation --- .../src/System/Collections/Generic/Dictionary.cs | 8 ++++++-- .../src/System/Collections/Generic/HashSet.cs | 8 ++++++-- .../src/System/Collections/HashHelpers.cs | 2 ++ 3 files changed, 14 insertions(+), 4 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 dc0ac5e2a4850..8e09715685c93 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 @@ -22,6 +22,10 @@ public class Dictionary : IDictionary, IDictionary, private const string KeyValuePairsName = "KeyValuePairs"; // Do not rename (binary serialization) private const string ComparerName = "Comparer"; // Do not rename (binary serialization) +#pragma warning disable CA1825 // avoid the extra generic instantiation for Array.Empty() + private static readonly Entry[] s_emptyArray = new Entry[0]; +#pragma warning restore CA1825 + private int[] _buckets; private Entry[] _entries; #if TARGET_64BIT @@ -502,10 +506,10 @@ private void InitializeEmpty() { _freeList = -1; #if TARGET_64BIT - _fastModMultiplier = HashHelpers.GetFastModMultiplier(1); + _fastModMultiplier = HashHelpers.FastModMultiplierDivisor1; #endif _buckets = HashHelpers.SizeOneIntArray; - _entries = Array.Empty(); + _entries = s_emptyArray; } private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) 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 975b91be12ff9..0bfea1d6b2597 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 @@ -27,6 +27,10 @@ public class HashSet : ICollection, ISet, IReadOnlyCollection, IRead /// Cutoff point for stackallocs. This corresponds to the number of ints. private const int StackAllocThreshold = 100; +#pragma warning disable CA1825 // avoid the extra generic instantiation for Array.Empty() + private static readonly Entry[] s_emptyArray = new Entry[0]; +#pragma warning restore CA1825 + /// /// When constructing a hashset from an existing collection, it may contain duplicates, /// so this is used as the max acceptable excess ratio of capacity to count. Note that @@ -1083,10 +1087,10 @@ private void InitializeEmpty() { _freeList = -1; #if TARGET_64BIT - _fastModMultiplier = HashHelpers.GetFastModMultiplier(1); + _fastModMultiplier = HashHelpers.FastModMultiplierDivisor1; #endif _buckets = HashHelpers.SizeOneIntArray; - _entries = Array.Empty(); + _entries = s_emptyArray; } /// Adds the specified element to the set if it's not already contained. diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs index 1292cb986eaea..fc464aa6694bb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs @@ -91,6 +91,8 @@ public static int ExpandPrime(int oldSize) return GetPrime(newSize); } + public static readonly ulong FastModMultiplierDivisor1 = GetFastModMultiplier(1); + /// Returns approximate reciprocal of the divisor: ceil(2**64 / divisor). /// This should only be used on 64-bit. public static ulong GetFastModMultiplier(uint divisor) => From ac22ef9c1f397e618a17cf08cf0114dbd98a8953 Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Mon, 15 Mar 2021 20:07:44 -0500 Subject: [PATCH 4/8] Fix errors. Adding the static field in Dictionary added a static constructor which made a reflection lookup ambiguous. Additionally since initially empty dictionaries now gets the hashcode of the item's key it exposed an issue with mono's DefineEnum method where getting the hashcode of the ITypeIdentifier of a null name caused a NullReferenceException. --- .../src/System/Collections/HashHelpers.cs | 2 +- .../System/Runtime/Serialization/XmlFormatGeneratorStatics.cs | 2 +- .../src/System/Reflection/Emit/ModuleBuilder.Mono.cs | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs index fc464aa6694bb..f1e21ed8d58e8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs @@ -15,7 +15,7 @@ internal static partial class HashHelpers public const int HashPrime = 101; - // must never be written to + // This field is shared between Dictionary and HashSet for initially empty collections and shouldn't be written to public static readonly int[] SizeOneIntArray = new int[1]; // Table of prime numbers to use as hash table sizes. diff --git a/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatGeneratorStatics.cs b/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatGeneratorStatics.cs index 654ab81627bce..2de7696c23e05 100644 --- a/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatGeneratorStatics.cs +++ b/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatGeneratorStatics.cs @@ -208,7 +208,7 @@ internal static ConstructorInfo HashtableCtor { if (s_hashtableCtor == null) { - s_hashtableCtor = Globals.TypeOfHashtable.GetConstructor(Globals.ScanAllMembers, Type.EmptyTypes); + s_hashtableCtor = Globals.TypeOfHashtable.GetConstructor(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public, Type.EmptyTypes); Debug.Assert(s_hashtableCtor != null); } return s_hashtableCtor; diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs index 80220b22e4ba7..603ba7304b7cf 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs @@ -342,6 +342,9 @@ public MethodInfo GetArrayMethod(Type arrayClass, string methodName, CallingConv public EnumBuilder DefineEnum(string name, TypeAttributes visibility, Type underlyingType) { + if (name == null) + throw new ArgumentNullException(nameof(name)); + ITypeIdentifier ident = TypeIdentifiers.FromInternal(name); if (name_cache.ContainsKey(ident)) throw new ArgumentException("Duplicate type name within an assembly."); From 94be5e8e6f00c3b319839157050363647c2f156f Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Mon, 15 Mar 2021 21:45:45 -0500 Subject: [PATCH 5/8] Remove tabs --- .../src/System/Reflection/Emit/ModuleBuilder.Mono.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs index 603ba7304b7cf..d7f82b45f0ad7 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs @@ -342,9 +342,9 @@ public MethodInfo GetArrayMethod(Type arrayClass, string methodName, CallingConv public EnumBuilder DefineEnum(string name, TypeAttributes visibility, Type underlyingType) { - if (name == null) + if (name == null) throw new ArgumentNullException(nameof(name)); - + ITypeIdentifier ident = TypeIdentifiers.FromInternal(name); if (name_cache.ContainsKey(ident)) throw new ArgumentException("Duplicate type name within an assembly."); From 878852f93317356c08450a2995f751aba5c5c77d Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Tue, 16 Mar 2021 08:20:14 -0500 Subject: [PATCH 6/8] Fix unit test by propagating the null arg check to EnumBuilder --- .../src/System/Reflection/Emit/ModuleBuilder.Mono.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs index d7f82b45f0ad7..e343f3ef6a489 100644 --- a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.Mono.cs @@ -342,14 +342,11 @@ public MethodInfo GetArrayMethod(Type arrayClass, string methodName, CallingConv public EnumBuilder DefineEnum(string name, TypeAttributes visibility, Type underlyingType) { - if (name == null) - throw new ArgumentNullException(nameof(name)); - ITypeIdentifier ident = TypeIdentifiers.FromInternal(name); - if (name_cache.ContainsKey(ident)) + if (name != null && name_cache.ContainsKey(ident)) throw new ArgumentException("Duplicate type name within an assembly."); - EnumBuilder eb = new EnumBuilder(this, name, visibility, underlyingType); + EnumBuilder eb = new EnumBuilder(this, name!, visibility, underlyingType); TypeBuilder res = eb.GetTypeBuilder(); AddType(res); name_cache.Add(ident, res); From 6f80ec6b7631c4352ca3bc1c55d3639856530aed Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Mon, 19 Apr 2021 12:00:36 -0500 Subject: [PATCH 7/8] Update HashHelpers.cs --- .../src/System/Collections/HashHelpers.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs index f1e21ed8d58e8..f2b5eb02ddb2d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs @@ -14,9 +14,11 @@ internal static partial class HashHelpers public const int MaxPrimeArrayLength = 0x7FEFFFFD; public const int HashPrime = 101; - + +#if SYSTEM_PRIVATE_CORELIB // This field is shared between Dictionary and HashSet for initially empty collections and shouldn't be written to public static readonly int[] SizeOneIntArray = new int[1]; +#endif // Table of prime numbers to use as hash table sizes. // A typical resize algorithm would pick the smallest prime number in this array From c750af802707782b7356e3ac7f7791d950eaf82f Mon Sep 17 00:00:00 2001 From: Tyler Brinkley Date: Mon, 19 Apr 2021 12:48:43 -0500 Subject: [PATCH 8/8] Remove trailing whitespace --- .../src/System/Collections/HashHelpers.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs index f2b5eb02ddb2d..a20aa2d099ce1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs @@ -14,7 +14,7 @@ internal static partial class HashHelpers public const int MaxPrimeArrayLength = 0x7FEFFFFD; public const int HashPrime = 101; - + #if SYSTEM_PRIVATE_CORELIB // This field is shared between Dictionary and HashSet for initially empty collections and shouldn't be written to public static readonly int[] SizeOneIntArray = new int[1];