From b30abeac835fea20a8b8a59a9b90e2ece5a7555e Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 27 May 2020 05:28:07 -0400 Subject: [PATCH 1/7] Clean up style in ConcurrentDictionary No functional changes. --- .../src/System.Collections.Concurrent.csproj | 1 + .../Concurrent/ConcurrentDictionary.cs | 1174 ++++++++--------- .../src/System/ThrowHelper.cs | 30 + 3 files changed, 588 insertions(+), 617 deletions(-) create mode 100644 src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs diff --git a/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj b/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj index c049418e129ad..516f92d46f22b 100644 --- a/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj +++ b/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj @@ -7,6 +7,7 @@ $(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix + diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index 7667e21505e7c..ff364e23b9c4d 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -2,16 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -/*============================================================ -** -** Class: ConcurrentDictionary -** -** -** Purpose: A scalable dictionary for concurrent access -** -** -===========================================================*/ - using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics; @@ -21,9 +11,7 @@ namespace System.Collections.Concurrent { - /// - /// Represents a thread-safe collection of keys and values. - /// + /// Represents a thread-safe collection of keys and values. /// The type of the keys in the dictionary. /// The type of the values in the dictionary. /// @@ -34,55 +22,42 @@ namespace System.Collections.Concurrent [DebuggerDisplay("Count = {Count}")] public class ConcurrentDictionary : IDictionary, IDictionary, IReadOnlyDictionary where TKey : notnull { - /// - /// Tables that hold the internal state of the ConcurrentDictionary - /// - /// Wrapping the three tables in a single object allows us to atomically - /// replace all tables at once. - /// - private sealed class Tables - { - internal readonly Node[] _buckets; // A singly-linked list for each bucket. - internal readonly object[] _locks; // A set of locks, each guarding a section of the table. - internal readonly int[] _countPerLock; // The number of elements guarded by each lock. - - internal Tables(Node[] buckets, object[] locks, int[] countPerLock) - { - _buckets = buckets; - _locks = locks; - _countPerLock = countPerLock; - } - } - - private volatile Tables _tables; // Internal tables of the dictionary - private readonly IEqualityComparer _comparer; // Key equality comparer - private readonly bool _growLockArray; // Whether to dynamically increase the size of the striped lock - private int _budget; // The maximum number of elements per lock before a resize operation is triggered - - // The default capacity, i.e. the initial # of buckets. When choosing this value, we are making - // a trade-off between the size of a very small dictionary, and the number of resizes when - // constructing a large dictionary. Also, the capacity should not be divisible by a small prime. + /// Internal tables of the dictionary. + private volatile Tables _tables; + /// Key equality comparer. + private readonly IEqualityComparer _comparer; + /// Whether to dynamically increase the size of the striped lock. + private readonly bool _growLockArray; + /// The maximum number of elements per lock before a resize operation is triggered. + private int _budget; + + /// The default capacity, i.e. the initial # of buckets. + /// + /// When choosing this value, we are making a trade-off between the size of a very small dictionary, + /// and the number of resizes when constructing a large dictionary. Also, the capacity should not be + /// divisible by a small prime. + /// private const int DefaultCapacity = 31; - // The maximum size of the striped lock that will not be exceeded when locks are automatically - // added as the dictionary grows. However, the user is allowed to exceed this limit by passing - // a concurrency level larger than MaxLockNumber into the constructor. + /// + /// The maximum size of the striped lock that will not be exceeded when locks are automatically + /// added as the dictionary grows. + /// + /// + /// The user is allowed to exceed this limit by passing + /// a concurrency level larger than MaxLockNumber into the constructor. + /// private const int MaxLockNumber = 1024; - // Whether TValue is a type that can be written atomically (i.e., with no danger of torn reads) + /// Whether TValue is a type that can be written atomically (i.e., with no danger of torn reads). private static readonly bool s_isValueWriteAtomic = IsValueWriteAtomic(); - /// - /// Determines whether type TValue can be written atomically - /// + /// Determines whether type TValue can be written atomically. private static bool IsValueWriteAtomic() { - // // Section 12.6.6 of ECMA CLI explains which types can be read and written atomically without - // the risk of tearing. - // - // See http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-335.pdf - // + // the risk of tearing. See https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf + Type valueType = typeof(TValue); if (!valueType.IsValueType) { @@ -101,124 +76,110 @@ private static bool IsValueWriteAtomic() case TypeCode.UInt16: case TypeCode.UInt32: return true; - case TypeCode.Int64: + case TypeCode.Double: + case TypeCode.Int64: case TypeCode.UInt64: return IntPtr.Size == 8; + default: return false; } } /// - /// Initializes a new instance of the + /// Initializes a new instance of the /// class that is empty, has the default concurrency level, has the default initial capacity, and /// uses the default comparer for the key type. /// - public ConcurrentDictionary() : this(DefaultConcurrencyLevel, DefaultCapacity, true, null) { } + public ConcurrentDictionary() : this(DefaultConcurrencyLevel, DefaultCapacity, growLockArray: true, null) { } /// - /// Initializes a new instance of the + /// Initializes a new instance of the /// class that is empty, has the specified concurrency level and capacity, and uses the default /// comparer for the key type. /// /// The estimated number of threads that will update the /// concurrently. - /// The initial number of elements that the - /// can contain. - /// is - /// less than 1. - /// is less than - /// 0. - public ConcurrentDictionary(int concurrencyLevel, int capacity) : this(concurrencyLevel, capacity, false, null) { } + /// The initial number of elements that the can contain. + /// is less than 1. + /// is less than 0. + public ConcurrentDictionary(int concurrencyLevel, int capacity) : this(concurrencyLevel, capacity, growLockArray: false, null) { } /// /// Initializes a new instance of the - /// class that contains elements copied from the specified , has the default concurrency + /// class that contains elements copied from the specified , has the default concurrency /// level, has the default initial capacity, and uses the default comparer for the key type. /// /// The whose elements are copied to - /// the new - /// . - /// is a null reference - /// (Nothing in Visual Basic). - /// contains one or more - /// duplicate keys. + /// cref="IEnumerable{T}"/> whose elements are copied to the new . + /// is a null reference (Nothing in Visual Basic). + /// contains one or more duplicate keys. public ConcurrentDictionary(IEnumerable> collection) : this(collection, null) { } /// /// Initializes a new instance of the /// class that is empty, has the specified concurrency level and capacity, and uses the specified - /// . + /// . /// - /// The - /// implementation to use when comparing keys. - public ConcurrentDictionary(IEqualityComparer? comparer) : this(DefaultConcurrencyLevel, DefaultCapacity, true, comparer) { } + /// The implementation to use when comparing keys. + public ConcurrentDictionary(IEqualityComparer? comparer) : this(DefaultConcurrencyLevel, DefaultCapacity, growLockArray: true, comparer) { } /// /// Initializes a new instance of the - /// class that contains elements copied from the specified , has the default concurrency level, has the default - /// initial capacity, and uses the specified - /// . + /// class that contains elements copied from the specified , has the default concurrency + /// level, has the default initial capacity, and uses the specified . /// - /// The whose elements are copied to - /// the new - /// . - /// The - /// implementation to use when comparing keys. - /// is a null reference - /// (Nothing in Visual Basic). + /// The whose elements are copied to the new . + /// The implementation to use when comparing keys. + /// is a null reference (Nothing in Visual Basic). public ConcurrentDictionary(IEnumerable> collection, IEqualityComparer? comparer) : this(comparer) { - if (collection == null) throw new ArgumentNullException(nameof(collection)); + if (collection is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(collection)); + } InitializeFromCollection(collection); } /// /// Initializes a new instance of the - /// class that contains elements copied from the specified , + /// class that contains elements copied from the specified , /// has the specified concurrency level, has the specified initial capacity, and uses the specified - /// . + /// . /// - /// The estimated number of threads that will update the - /// concurrently. - /// The whose elements are copied to the new + /// + /// The estimated number of threads that will update the concurrently. + /// + /// The whose elements are copied to the new /// . - /// The implementation to use - /// when comparing keys. - /// - /// is a null reference (Nothing in Visual Basic). - /// - /// - /// is less than 1. - /// - /// contains one or more duplicate keys. - public ConcurrentDictionary( - int concurrencyLevel, IEnumerable> collection, IEqualityComparer? comparer) - : this(concurrencyLevel, DefaultCapacity, false, comparer) - { - if (collection == null) throw new ArgumentNullException(nameof(collection)); + /// The implementation to use when comparing keys. + /// is a null reference (Nothing in Visual Basic). + /// is less than 1. + /// contains one or more duplicate keys. + public ConcurrentDictionary(int concurrencyLevel, IEnumerable> collection, IEqualityComparer? comparer) + : this(concurrencyLevel, DefaultCapacity, growLockArray: false, comparer) + { + if (collection is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(collection)); + } InitializeFromCollection(collection); } private void InitializeFromCollection(IEnumerable> collection) { - TValue dummy; foreach (KeyValuePair pair in collection) { - if (pair.Key == null) ThrowKeyNullException(); + if (pair.Key is null) + { + ThrowHelper.ThrowKeyNullException(); + } - if (!TryAddInternal(pair.Key, _comparer.GetHashCode(pair.Key), pair.Value, false, false, out dummy)) + if (!TryAddInternal(pair.Key, _comparer.GetHashCode(pair.Key), pair.Value, updateIfExists: false, acquireLock: false, out _)) { throw new ArgumentException(SR.ConcurrentDictionary_SourceContainsDuplicateKeys); } @@ -233,21 +194,14 @@ private void InitializeFromCollection(IEnumerable> co /// /// Initializes a new instance of the /// class that is empty, has the specified concurrency level, has the specified initial capacity, and - /// uses the specified . + /// uses the specified . /// - /// The estimated number of threads that will update the - /// concurrently. - /// The initial number of elements that the - /// can contain. - /// The - /// implementation to use when comparing keys. - /// - /// is less than 1. -or- - /// is less than 0. - /// + /// The estimated number of threads that will update the concurrently. + /// The initial number of elements that the can contain. + /// The implementation to use when comparing keys. + /// is less than 1. -or- is less than 0. public ConcurrentDictionary(int concurrencyLevel, int capacity, IEqualityComparer? comparer) - : this(concurrencyLevel, capacity, false, comparer) + : this(concurrencyLevel, capacity, growLockArray: false, comparer) { } @@ -269,14 +223,14 @@ internal ConcurrentDictionary(int concurrencyLevel, int capacity, bool growLockA capacity = concurrencyLevel; } - object[] locks = new object[concurrencyLevel]; + var locks = new object[concurrencyLevel]; for (int i = 0; i < locks.Length; i++) { locks[i] = new object(); } - int[] countPerLock = new int[locks.Length]; - Node[] buckets = new Node[capacity]; + var countPerLock = new int[locks.Length]; + var buckets = new Node[capacity]; _tables = new Tables(buckets, locks, countPerLock); _comparer = comparer ?? EqualityComparer.Default; @@ -285,61 +239,61 @@ internal ConcurrentDictionary(int concurrencyLevel, int capacity, bool growLockA } /// - /// Attempts to add the specified key and value to the . + /// Attempts to add the specified key and value to the . /// /// The key of the element to add. /// The value of the element to add. The value can be a null reference (Nothing /// in Visual Basic) for reference types. - /// true if the key/value pair was added to the - /// successfully; otherwise, false. - /// is null reference - /// (Nothing in Visual Basic). - /// The - /// contains too many elements. + /// + /// true if the key/value pair was added to the successfully; otherwise, false. + /// + /// is null reference (Nothing in Visual Basic). + /// The contains too many elements. public bool TryAdd(TKey key, TValue value) { - if (key == null) ThrowKeyNullException(); - TValue dummy; - return TryAddInternal(key, _comparer.GetHashCode(key), value, false, true, out dummy); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + return TryAddInternal(key, _comparer.GetHashCode(key), value, updateIfExists: false, acquireLock: true, out _); } /// - /// Determines whether the contains the specified - /// key. + /// Determines whether the contains the specified key. /// - /// The key to locate in the . - /// true if the contains an element with - /// the specified key; otherwise, false. - /// is a null reference - /// (Nothing in Visual Basic). + /// The key to locate in the . + /// true if the contains an element with the specified key; otherwise, false. + /// is a null reference (Nothing in Visual Basic). public bool ContainsKey(TKey key) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } - TValue throwAwayValue; - return TryGetValue(key, out throwAwayValue); + return TryGetValue(key, out _); } /// - /// Attempts to remove and return the value with the specified key from the - /// . + /// Attempts to remove and return the value with the specified key from the . /// /// The key of the element to remove and return. - /// When this method returns, contains the object removed from the + /// + /// When this method returns, contains the object removed from the /// or the default value of - /// if the operation failed. + /// name="TValue"/> if the operation failed. + /// /// true if an object was removed successfully; otherwise, false. - /// is a null reference - /// (Nothing in Visual Basic). + /// is a null reference (Nothing in Visual Basic). public bool TryRemove(TKey key, [MaybeNullWhen(false)] out TValue value) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } - return TryRemoveInternal(key, out value, false, default); + return TryRemoveInternal(key, out value, matchValue: false, default); } /// Removes a key and value from the dictionary. @@ -354,14 +308,14 @@ public bool TryRemove(TKey key, [MaybeNullWhen(false)] out TValue value) /// if no comparer was provided to the dictionary when it was constructed). The value is compared using the /// default comparer for . /// - /// + /// /// The property of is a null reference. /// public bool TryRemove(KeyValuePair item) { if (item.Key is null) { - throw new ArgumentNullException(nameof(item), SR.ConcurrentDictionary_ItemKeyIsNull); + ThrowHelper.ThrowArgumentNullException(nameof(item), SR.ConcurrentDictionary_ItemKeyIsNull); } return TryRemoveInternal(item.Key, out _, matchValue: true, item.Value); @@ -376,16 +330,13 @@ public bool TryRemove(KeyValuePair item) /// The variable into which the removed value, if found, is stored. /// Whether removal of the key is conditional on its value. /// The conditional value to compare against if is true - /// private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value, bool matchValue, [AllowNull] TValue oldValue) { int hashcode = _comparer.GetHashCode(key); while (true) { Tables tables = _tables; - - int bucketNo, lockNo; - GetBucketAndLockNo(hashcode, out bucketNo, out lockNo, tables._buckets.Length, tables._locks.Length); + GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, tables._buckets.Length, tables._locks.Length); lock (tables._locks[lockNo]) { @@ -397,9 +348,9 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value } Node? prev = null; - for (Node curr = tables._buckets[bucketNo]; curr != null; curr = curr._next) + for (Node? curr = tables._buckets[bucketNo]; curr != null; curr = curr._next) { - Debug.Assert((prev == null && curr == tables._buckets[bucketNo]) || prev!._next == curr); + Debug.Assert((prev is null && curr == tables._buckets[bucketNo]) || prev!._next == curr); if (hashcode == curr._hashcode && _comparer.Equals(curr._key, key)) { @@ -408,14 +359,14 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value bool valuesMatch = EqualityComparer.Default.Equals(oldValue, curr._value); if (!valuesMatch) { - value = default(TValue)!; + value = default!; return false; } } - if (prev == null) + if (prev is null) { - Volatile.Write(ref tables._buckets[bucketNo], curr._next); + Volatile.Write(ref tables._buckets[bucketNo], curr._next); } else { @@ -430,27 +381,29 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value } } - value = default(TValue)!; + value = default; return false; } } /// - /// Attempts to get the value associated with the specified key from the . + /// Attempts to get the value associated with the specified key from the . /// /// The key of the value to get. - /// When this method returns, contains the object from - /// the - /// with the specified key or the default value of - /// , if the operation failed. - /// true if the key was found in the ; - /// otherwise, false. - /// is a null reference - /// (Nothing in Visual Basic). + /// + /// When this method returns, contains the object from + /// the with the specified key or the default value of + /// , if the operation failed. + /// + /// true if the key was found in the ; otherwise, false. + /// is a null reference (Nothing in Visual Basic). public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + return TryGetValueInternal(key, _comparer.GetHashCode(key), out value); } @@ -458,27 +411,23 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] { Debug.Assert(_comparer.GetHashCode(key) == hashcode); - // We must capture the _buckets field in a local variable. It is set to a new table on each table resize. + // We must capture the volatile _tables field into a local variable: it is set to a new table on each table resize. + // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: + // this protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. Tables tables = _tables; int bucketNo = GetBucket(hashcode, tables._buckets.Length); - // We can get away w/out a lock here. - // The Volatile.Read ensures that we have a copy of the reference to tables._buckets[bucketNo]. - // This protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. - Node n = Volatile.Read(ref tables._buckets[bucketNo]); - - while (n != null) + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) { if (hashcode == n._hashcode && _comparer.Equals(n._key, key)) { value = n._value; return true; } - n = n._next; } - value = default(TValue)!; + value = default; return false; } @@ -492,14 +441,18 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] /// name="key"/> if the comparison results in equality. /// The value that is compared to the value of the element with /// . - /// true if the value with was equal to and replaced with ; otherwise, - /// false. - /// is a null - /// reference. + /// + /// true if the value with was equal to and + /// replaced with ; otherwise, false. + /// + /// is a null reference. public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + return TryUpdateInternal(key, _comparer.GetHashCode(key), newValue, comparisonValue); } @@ -514,11 +467,11 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) /// name="key"/> if the comparison results in equality. /// The value that is compared to the value of the element with /// . - /// true if the value with was equal to and replaced with ; otherwise, - /// false. - /// is a null - /// reference. + /// + /// true if the value with was equal to and + /// replaced with ; otherwise, false. + /// + /// is a null reference. private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue comparisonValue) { Debug.Assert(_comparer.GetHashCode(key) == hashcode); @@ -527,11 +480,8 @@ private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue c while (true) { - int bucketNo; - int lockNo; - Tables tables = _tables; - GetBucketAndLockNo(hashcode, out bucketNo, out lockNo, tables._buckets.Length, tables._locks.Length); + GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, tables._buckets.Length, tables._locks.Length); lock (tables._locks[lockNo]) { @@ -544,9 +494,9 @@ private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue c // Try to find this key in the bucket Node? prev = null; - for (Node node = tables._buckets[bucketNo]; node != null; node = node._next) + for (Node? node = tables._buckets[bucketNo]; node != null; node = node._next) { - Debug.Assert((prev == null && node == tables._buckets[bucketNo]) || prev!._next == node); + Debug.Assert((prev is null && node == tables._buckets[bucketNo]) || prev!._next == node); if (hashcode == node._hashcode && _comparer.Equals(node._key, key)) { if (valueComparer.Equals(node._value, comparisonValue)) @@ -557,9 +507,9 @@ private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue c } else { - Node newNode = new Node(node._key, newValue, hashcode, node._next); + var newNode = new Node(node._key, newValue, hashcode, node._next); - if (prev == null) + if (prev is null) { Volatile.Write(ref tables._buckets[bucketNo], newNode); } @@ -578,7 +528,7 @@ private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue c prev = node; } - //didn't find the key + // didn't find the key return false; } } @@ -594,7 +544,7 @@ public void Clear() { AcquireAllLocks(ref locksAcquired); - Tables newTables = new Tables(new Node[DefaultCapacity], _tables._locks, new int[_tables._countPerLock.Length]); + var newTables = new Tables(new Node[DefaultCapacity], _tables._locks, new int[_tables._countPerLock.Length]); _tables = newTables; _budget = Math.Max(1, newTables._buckets.Length / newTables._locks.Length); } @@ -605,30 +555,32 @@ public void Clear() } /// - /// Copies the elements of the to an array of - /// type , starting at the - /// specified array index. + /// Copies the elements of the to an array of type , + /// starting at the specified array index. /// - /// The one-dimensional array of type - /// that is the destination of the elements copied from the . The array must have zero-based indexing. - /// The zero-based index in at which copying - /// begins. - /// is a null reference - /// (Nothing in Visual Basic). - /// is less than - /// 0. - /// is equal to or greater than - /// the length of the . -or- The number of elements in the source - /// is greater than the available space from to the end of the destination - /// . + /// + /// The one-dimensional array of type that is the destination of the elements copied from the . The array must have zero-based indexing. + /// + /// The zero-based index in at which copying begins. + /// is a null reference (Nothing in Visual Basic). + /// is less than 0. + /// + /// is equal to or greater than the length of the . -or- The number of + /// elements in the source is greater than the available space from to + /// the end of the destination . + /// void ICollection>.CopyTo(KeyValuePair[] array, int index) { - if (array == null) throw new ArgumentNullException(nameof(array)); - if (index < 0) throw new ArgumentOutOfRangeException(nameof(index), SR.ConcurrentDictionary_IndexIsNegative); + if (array is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(array)); + } + + if (index < 0) + { + throw new ArgumentOutOfRangeException(nameof(index), SR.ConcurrentDictionary_IndexIsNegative); + } int locksAcquired = 0; try @@ -659,19 +611,20 @@ void ICollection>.CopyTo(KeyValuePair[] /// Copies the key and value pairs stored in the to a /// new array. /// - /// A new array containing a snapshot of key and value pairs copied from the . + /// A new array containing a snapshot of key and value pairs copied from the . + /// public KeyValuePair[] ToArray() { int locksAcquired = 0; try { AcquireAllLocks(ref locksAcquired); + int count = 0; - checked + int[] countPerLock = _tables._countPerLock; + for (int i = 0; i < countPerLock.Length; i++) { - int[] countPerLock = _tables._countPerLock; - for (int i = 0; i < countPerLock.Length; i++) + checked { count += countPerLock[i]; } @@ -682,7 +635,7 @@ public KeyValuePair[] ToArray() return Array.Empty>(); } - KeyValuePair[] array = new KeyValuePair[count]; + var array = new KeyValuePair[count]; CopyToPairs(array, 0); return array; } @@ -692,35 +645,29 @@ public KeyValuePair[] ToArray() } } - /// - /// Copy dictionary contents to an array - shared implementation between ToArray and CopyTo. - /// - /// Important: the caller must hold all locks in _locks before calling CopyToPairs. - /// + /// Copy dictionary contents to an array - shared implementation between ToArray and CopyTo. + /// Important: the caller must hold all locks in _locks before calling CopyToPairs. private void CopyToPairs(KeyValuePair[] array, int index) { - Node[] buckets = _tables._buckets; + Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { - for (Node current = buckets[i]; current != null; current = current._next) + for (Node? current = buckets[i]; current != null; current = current._next) { array[index] = new KeyValuePair(current._key, current._value); - index++; //this should never flow, CopyToPairs is only called when there's no overflow risk + index++; // this should never overflow, CopyToPairs is only called when there's no overflow risk } } } - /// - /// Copy dictionary contents to an array - shared implementation between ToArray and CopyTo. - /// - /// Important: the caller must hold all locks in _locks before calling CopyToEntries. - /// + /// Copy dictionary contents to an array - shared implementation between ToArray and CopyTo. + /// Important: the caller must hold all locks in _locks before calling CopyToPairs. private void CopyToEntries(DictionaryEntry[] array, int index) { - Node[] buckets = _tables._buckets; + Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { - for (Node current = buckets[i]; current != null; current = current._next) + for (Node? current = buckets[i]; current != null; current = current._next) { array[index] = new DictionaryEntry(current._key, current._value); index++; //this should never flow, CopyToEntries is only called when there's no overflow risk @@ -728,20 +675,17 @@ private void CopyToEntries(DictionaryEntry[] array, int index) } } - /// - /// Copy dictionary contents to an array - shared implementation between ToArray and CopyTo. - /// - /// Important: the caller must hold all locks in _locks before calling CopyToObjects. - /// + /// Copy dictionary contents to an array - shared implementation between ToArray and CopyTo. + /// Important: the caller must hold all locks in _locks before calling CopyToPairs. private void CopyToObjects(object[] array, int index) { - Node[] buckets = _tables._buckets; + Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { - for (Node current = buckets[i]; current != null; current = current._next) + for (Node? current = buckets[i]; current != null; current = current._next) { array[index] = new KeyValuePair(current._key, current._value); - index++; //this should never flow, CopyToObjects is only called when there's no overflow risk + index++; // this should never overflow, CopyToObjects is only called when there's no overflow risk } } } @@ -757,18 +701,14 @@ private void CopyToObjects(object[] array, int index) /// public IEnumerator> GetEnumerator() { - Node[] buckets = _tables._buckets; - + Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { - // The Volatile.Read ensures that we have a copy of the reference to buckets[i]. - // This protects us from reading fields ('_key', '_value' and '_next') of different instances. - Node current = Volatile.Read(ref buckets[i]); - - while (current != null) + // The Volatile.Read ensures that we have a copy of the reference to buckets[i]: + // this protects us from reading fields ('_key', '_value' and '_next') of different instances. + for (Node? current = Volatile.Read(ref buckets[i]); current != null; current = current._next) { yield return new KeyValuePair(current._key, current._value); - current = current._next; } } } @@ -784,17 +724,17 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE while (true) { - int bucketNo, lockNo; - Tables tables = _tables; - GetBucketAndLockNo(hashcode, out bucketNo, out lockNo, tables._buckets.Length, tables._locks.Length); + GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, tables._buckets.Length, tables._locks.Length); bool resizeDesired = false; bool lockTaken = false; try { if (acquireLock) + { Monitor.Enter(tables._locks[lockNo], ref lockTaken); + } // If the table just got resized, we may not be holding the right lock, and must retry. // This should be a rare occurrence. @@ -805,9 +745,9 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE // Try to find this key in the bucket Node? prev = null; - for (Node node = tables._buckets[bucketNo]; node != null; node = node._next) + for (Node? node = tables._buckets[bucketNo]; node != null; node = node._next) { - Debug.Assert((prev == null && node == tables._buckets[bucketNo]) || prev!._next == node); + Debug.Assert((prev is null && node == tables._buckets[bucketNo]) || prev!._next == node); if (hashcode == node._hashcode && _comparer.Equals(node._key, key)) { // The key was found in the dictionary. If updates are allowed, update the value for that key. @@ -821,8 +761,8 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE } else { - Node newNode = new Node(node._key, value, hashcode, node._next); - if (prev == null) + var newNode = new Node(node._key, value, hashcode, node._next); + if (prev is null) { Volatile.Write(ref tables._buckets[bucketNo], newNode); } @@ -843,7 +783,7 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE } // The key was not found in the bucket. Insert the key-value pair. - Volatile.Write(ref tables._buckets[bucketNo], new Node(key, value, hashcode, tables._buckets[bucketNo])); + Volatile.Write(ref tables._buckets[bucketNo], new Node(key, value, hashcode, tables._buckets[bucketNo])); checked { tables._countPerLock[lockNo]++; @@ -862,7 +802,9 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE finally { if (lockTaken) + { Monitor.Exit(tables._locks[lockNo]); + } } // @@ -883,81 +825,44 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE } } - /// - /// Gets or sets the value associated with the specified key. - /// + /// Gets or sets the value associated with the specified key. /// The key of the value to get or set. - /// The value associated with the specified key. If the specified key is not found, a get - /// operation throws a - /// , and a set operation creates a new - /// element with the specified key. - /// is a null reference - /// (Nothing in Visual Basic). - /// The property is retrieved and - /// - /// does not exist in the collection. + /// + /// The value associated with the specified key. If the specified key is not found, a get operation throws a + /// , and a set operation creates a new element with the specified key. + /// + /// + /// is a null reference (Nothing in Visual Basic). + /// + /// + /// The property is retrieved and does not exist in the collection. + /// public TValue this[TKey key] { get { - TValue value; - if (!TryGetValue(key, out value)) + if (!TryGetValue(key, out TValue value)) { - ThrowKeyNotFoundException(key); + ThrowHelper.ThrowKeyNotFoundException(key); } return value; } set { - if (key == null) ThrowKeyNullException(); - TValue dummy; - TryAddInternal(key, _comparer.GetHashCode(key), value, true, true, out dummy); - } - } - - // These exception throwing sites have been extracted into their own NoInlining methods - // as these are uncommonly needed and when inlined are observed to prevent the inlining - // of important methods like TryGetValue and ContainsKey. - - [DoesNotReturn] - private static void ThrowKeyNotFoundException(object key) - { - throw new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); - } - - [DoesNotReturn] - private static void ThrowKeyNullException() - { - throw new ArgumentNullException("key"); - } - - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static void ThrowIfInvalidObjectValue(object? value) - { - if (value != null) - { - if (!(value is TValue)) + if (key is null) { - ThrowValueNullException(); + ThrowHelper.ThrowKeyNullException(); } - } - else if (default(TValue) != null) - { - ThrowValueNullException(); - } - } - private static void ThrowValueNullException() - { - throw new ArgumentException(SR.ConcurrentDictionary_TypeOfValueIncorrect); + TryAddInternal(key, _comparer.GetHashCode(key), value, updateIfExists: true, acquireLock: true, out _); + } } /// /// Gets the number of key/value pairs contained in the . /// - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The number of key/value pairs contained in the . @@ -989,7 +894,7 @@ public int Count /// cref="ConcurrentDictionary{TKey,TValue}"/>. Should only be used after all locks /// have been acquired. /// - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The number of key/value pairs contained in the . @@ -1016,27 +921,34 @@ private int GetCountInternal() /// /// The key of the element to add. /// The function used to generate a value for the key - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The value for the key. This will be either the existing value for the key if the /// key is already in the dictionary, or the new value for the key as returned by valueFactory /// if the key was not in the dictionary. public TValue GetOrAdd(TKey key, Func valueFactory) { - if (key == null) ThrowKeyNullException(); - if (valueFactory == null) throw new ArgumentNullException(nameof(valueFactory)); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (valueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); + } int hashcode = _comparer.GetHashCode(key); - TValue resultingValue; - if (!TryGetValueInternal(key, hashcode, out resultingValue)) + if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { - TryAddInternal(key, hashcode, valueFactory(key), false, true, out resultingValue); + TryAddInternal(key, hashcode, valueFactory(key), updateIfExists: false, acquireLock: true, out resultingValue); } + return resultingValue; } @@ -1047,27 +959,34 @@ public TValue GetOrAdd(TKey key, Func valueFactory) /// The key of the element to add. /// The function used to generate a value for the key /// An argument value to pass into . - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The value for the key. This will be either the existing value for the key if the /// key is already in the dictionary, or the new value for the key as returned by valueFactory /// if the key was not in the dictionary. public TValue GetOrAdd(TKey key, Func valueFactory, TArg factoryArgument) { - if (key == null) ThrowKeyNullException(); - if (valueFactory == null) throw new ArgumentNullException(nameof(valueFactory)); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (valueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); + } int hashcode = _comparer.GetHashCode(key); - TValue resultingValue; - if (!TryGetValueInternal(key, hashcode, out resultingValue)) + if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { - TryAddInternal(key, hashcode, valueFactory(key, factoryArgument), false, true, out resultingValue); + TryAddInternal(key, hashcode, valueFactory(key, factoryArgument), updateIfExists: false, acquireLock: true, out resultingValue); } + return resultingValue; } @@ -1077,23 +996,26 @@ public TValue GetOrAdd(TKey key, Func valueFactory, TA /// /// The key of the element to add. /// the value to be added, if the key does not already exist - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The value for the key. This will be either the existing value for the key if the /// key is already in the dictionary, or the new value if the key was not in the dictionary. public TValue GetOrAdd(TKey key, TValue value) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } int hashcode = _comparer.GetHashCode(key); - TValue resultingValue; - if (!TryGetValueInternal(key, hashcode, out resultingValue)) + if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { - TryAddInternal(key, hashcode, value, false, true, out resultingValue); + TryAddInternal(key, hashcode, value, updateIfExists: false, acquireLock: true, out resultingValue); } + return resultingValue; } @@ -1107,29 +1029,39 @@ public TValue GetOrAdd(TKey key, TValue value) /// The function used to generate a new value for an existing key /// based on the key's existing value /// An argument to pass into and . - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The new value for the key. This will be either be the result of addValueFactory (if the key was /// absent) or the result of updateValueFactory (if the key was present). public TValue AddOrUpdate( TKey key, Func addValueFactory, Func updateValueFactory, TArg factoryArgument) { - if (key == null) ThrowKeyNullException(); - if (addValueFactory == null) throw new ArgumentNullException(nameof(addValueFactory)); - if (updateValueFactory == null) throw new ArgumentNullException(nameof(updateValueFactory)); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (addValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(addValueFactory)); + } + + if (updateValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); + } int hashcode = _comparer.GetHashCode(key); while (true) { - TValue oldValue; - if (TryGetValueInternal(key, hashcode, out oldValue)) + if (TryGetValueInternal(key, hashcode, out TValue oldValue)) { // key exists, try to update TValue newValue = updateValueFactory(key, oldValue, factoryArgument); @@ -1141,8 +1073,7 @@ public TValue AddOrUpdate( else { // key doesn't exist, try to add - TValue resultingValue; - if (TryAddInternal(key, hashcode, addValueFactory(key, factoryArgument), false, true, out resultingValue)) + if (TryAddInternal(key, hashcode, addValueFactory(key, factoryArgument), updateIfExists: false, acquireLock: true, out TValue resultingValue)) { return resultingValue; } @@ -1159,28 +1090,38 @@ public TValue AddOrUpdate( /// The function used to generate a value for an absent key /// The function used to generate a new value for an existing key /// based on the key's existing value - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The new value for the key. This will be either the result of addValueFactory (if the key was /// absent) or the result of updateValueFactory (if the key was present). public TValue AddOrUpdate(TKey key, Func addValueFactory, Func updateValueFactory) { - if (key == null) ThrowKeyNullException(); - if (addValueFactory == null) throw new ArgumentNullException(nameof(addValueFactory)); - if (updateValueFactory == null) throw new ArgumentNullException(nameof(updateValueFactory)); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (addValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(addValueFactory)); + } + + if (updateValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); + } int hashcode = _comparer.GetHashCode(key); while (true) { - TValue oldValue; - if (TryGetValueInternal(key, hashcode, out oldValue)) + if (TryGetValueInternal(key, hashcode, out TValue oldValue)) { // key exists, try to update TValue newValue = updateValueFactory(key, oldValue); @@ -1192,8 +1133,7 @@ public TValue AddOrUpdate(TKey key, Func addValueFactory, Func addValueFactory, FuncThe value to be added for an absent key /// The function used to generate a new value for an existing key based on /// the key's existing value - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. /// The new value for the key. This will be either the value of addValue (if the key was /// absent) or the result of updateValueFactory (if the key was present). public TValue AddOrUpdate(TKey key, TValue addValue, Func updateValueFactory) { - if (key == null) ThrowKeyNullException(); - if (updateValueFactory == null) throw new ArgumentNullException(nameof(updateValueFactory)); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (updateValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); + } int hashcode = _comparer.GetHashCode(key); while (true) { - TValue oldValue; - if (TryGetValueInternal(key, hashcode, out oldValue)) + if (TryGetValueInternal(key, hashcode, out TValue oldValue)) { // key exists, try to update TValue newValue = updateValueFactory(key, oldValue); @@ -1240,8 +1186,7 @@ public TValue AddOrUpdate(TKey key, TValue addValue, Func else { // key doesn't exist, try to add - TValue resultingValue; - if (TryAddInternal(key, hashcode, addValue, false, true, out resultingValue)) + if (TryAddInternal(key, hashcode, addValue, updateIfExists: false, acquireLock: true, out TValue resultingValue)) { return resultingValue; } @@ -1304,15 +1249,15 @@ bool AreAllBucketsEmpty() /// /// Adds the specified key and value to the . + /// cref="IDictionary{TKey,TValue}"/>. /// /// The object to use as the key of the element to add. /// The object to use as the value of the element to add. - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. - /// + /// /// An element with the same key already exists in the . void IDictionary.Add(TKey key, TValue value) @@ -1325,102 +1270,82 @@ void IDictionary.Add(TKey key, TValue value) /// /// Removes the element with the specified key from the . + /// cref="IDictionary{TKey,TValue}"/>. /// /// The key of the element to remove. /// true if the element is successfully remove; otherwise false. This method also returns /// false if /// was not found in the original . + /// cref="IDictionary{TKey,TValue}"/>. /// - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - bool IDictionary.Remove(TKey key) - { - TValue throwAwayValue; - return TryRemove(key, out throwAwayValue); - } + bool IDictionary.Remove(TKey key) => TryRemove(key, out _); /// /// Gets a collection containing the keys in the . + /// cref="Dictionary{TKey,TValue}"/>. /// - /// An containing the keys in the - /// . - public ICollection Keys - { - get { return GetKeys(); } - } + /// An containing the keys in the + /// . + public ICollection Keys => GetKeys(); /// - /// Gets an containing the keys of - /// the . + /// Gets an containing the keys of + /// the . /// - /// An containing the keys of - /// the . - IEnumerable IReadOnlyDictionary.Keys - { - get { return GetKeys(); } - } + /// An containing the keys of + /// the . + IEnumerable IReadOnlyDictionary.Keys => GetKeys(); /// /// Gets a collection containing the values in the . + /// cref="Dictionary{TKey,TValue}"/>. /// - /// An containing the values in + /// An containing the values in /// the - /// . - public ICollection Values - { - get { return GetValues(); } - } + /// . + public ICollection Values => GetValues(); /// - /// Gets an containing the values - /// in the . + /// Gets an containing the values + /// in the . /// - /// An containing the - /// values in the . - IEnumerable IReadOnlyDictionary.Values - { - get { return GetValues(); } - } + /// An containing the + /// values in the . + IEnumerable IReadOnlyDictionary.Values => GetValues(); #endregion #region ICollection> Members /// - /// Adds the specified value to the + /// Adds the specified value to the /// with the specified key. /// - /// The + /// The /// structure representing the key and value to add to the . - /// The of . + /// The of is null. - /// The + /// The /// contains too many elements. - /// An element with the same key already exists in the - /// - void ICollection>.Add(KeyValuePair keyValuePair) - { - ((IDictionary)this).Add(keyValuePair.Key, keyValuePair.Value); - } + /// An element with the same key already exists in the + /// + void ICollection>.Add(KeyValuePair keyValuePair) => ((IDictionary)this).Add(keyValuePair.Key, keyValuePair.Value); /// - /// Determines whether the + /// Determines whether the /// contains a specific key and value. /// - /// The + /// The /// structure to locate in the . + /// cref="ICollection{TValue}"/>. /// true if the is found in the ; otherwise, false. + /// cref="ICollection{T}"/>; otherwise, false. bool ICollection>.Contains(KeyValuePair keyValuePair) { - TValue value; - if (!TryGetValue(keyValuePair.Key, out value)) + if (!TryGetValue(keyValuePair.Key, out TValue value)) { return false; } @@ -1430,25 +1355,22 @@ bool ICollection>.Contains(KeyValuePair /// /// Gets a value indicating whether the dictionary is read-only. /// - /// true if the is + /// true if the is /// read-only; otherwise, false. For , this property always returns + /// cref="Dictionary{TKey,TValue}"/>, this property always returns /// false. - bool ICollection>.IsReadOnly - { - get { return false; } - } + bool ICollection>.IsReadOnly => false; /// /// Removes a key and value from the dictionary. /// /// The + /// cref="KeyValuePair{TKey,TValue}"/> /// structure representing the key and value to remove from the . + /// cref="Dictionary{TKey,TValue}"/>. /// true if the key and value represented by is successfully /// found and removed; otherwise, false. - /// The Key property of The Key property of is a null reference (Nothing in Visual Basic). bool ICollection>.Remove(KeyValuePair keyValuePair) => TryRemove(keyValuePair); @@ -1466,10 +1388,7 @@ bool ICollection>.Remove(KeyValuePair k /// of the dictionary. The contents exposed through the enumerator may contain modifications /// made to the dictionary after was called. /// - IEnumerator IEnumerable.GetEnumerator() - { - return ((ConcurrentDictionary)this).GetEnumerator(); - } + IEnumerator IEnumerable.GetEnumerator() => ((ConcurrentDictionary)this).GetEnumerator(); #endregion @@ -1480,118 +1399,116 @@ IEnumerator IEnumerable.GetEnumerator() /// /// The object to use as the key. /// The object to use as the value. - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// The dictionary contains too many + /// The dictionary contains too many /// elements. - /// + /// /// is of a type that is not assignable to the key type of the . -or- + /// name="TKey"/> of the . -or- /// is of a type that is not assignable to , - /// the type of values in the . + /// the type of values in the . /// -or- A value with the same key already exists in the . + /// cref="Dictionary{TKey,TValue}"/>. /// void IDictionary.Add(object key, object? value) { - if (key == null) ThrowKeyNullException(); - if (!(key is TKey)) throw new ArgumentException(SR.ConcurrentDictionary_TypeOfKeyIncorrect); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (!(key is TKey)) + { + throw new ArgumentException(SR.ConcurrentDictionary_TypeOfKeyIncorrect); + } + ThrowIfInvalidObjectValue(value); ((IDictionary)this).Add((TKey)key, (TValue)value!); } /// - /// Gets whether the contains an + /// Gets whether the contains an /// element with the specified key. /// /// The key to locate in the . - /// true if the contains + /// cref="IDictionary"/>. + /// true if the contains /// an element with the specified key; otherwise, false. - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). bool IDictionary.Contains(object key) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } - return (key is TKey) && this.ContainsKey((TKey)key); + return key is TKey tkey && ContainsKey(tkey); } - /// Provides an for the - /// . - /// An for the . - IDictionaryEnumerator IDictionary.GetEnumerator() - { - return new DictionaryEnumerator(this); - } + /// Provides an for the + /// . + /// An for the . + IDictionaryEnumerator IDictionary.GetEnumerator() => new DictionaryEnumerator(this); /// /// Gets a value indicating whether the has a fixed size. + /// cref="IDictionary"/> has a fixed size. /// - /// true if the has a + /// true if the has a /// fixed size; otherwise, false. For , this property always + /// cref="ConcurrentDictionary{TKey,TValue}"/>, this property always /// returns false. - bool IDictionary.IsFixedSize - { - get { return false; } - } + bool IDictionary.IsFixedSize => false; /// /// Gets a value indicating whether the is read-only. + /// cref="IDictionary"/> is read-only. /// - /// true if the is + /// true if the is /// read-only; otherwise, false. For , this property always + /// cref="ConcurrentDictionary{TKey,TValue}"/>, this property always /// returns false. - bool IDictionary.IsReadOnly - { - get { return false; } - } + bool IDictionary.IsReadOnly => false; /// - /// Gets an containing the keys of the . + /// Gets an containing the keys of the . /// - /// An containing the keys of the . - ICollection IDictionary.Keys - { - get { return GetKeys(); } - } + /// An containing the keys of the . + ICollection IDictionary.Keys => GetKeys(); /// /// Removes the element with the specified key from the . + /// cref="IDictionary"/>. /// /// The key of the element to remove. - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). void IDictionary.Remove(object key) { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } - TValue throwAwayValue; - if (key is TKey) + if (key is TKey tkey) { - TryRemove((TKey)key, out throwAwayValue); + TryRemove(tkey, out _); } } /// - /// Gets an containing the values in the . + /// Gets an containing the values in the . /// - /// An containing the values in the . - ICollection IDictionary.Values - { - get { return GetValues(); } - } + /// An containing the values in the . + ICollection IDictionary.Values => GetValues(); /// /// Gets or sets the value associated with the specified key. @@ -1600,25 +1517,27 @@ ICollection IDictionary.Values /// The value associated with the specified key, or a null reference (Nothing in Visual Basic) /// if is not in the dictionary or is of a type that is /// not assignable to the key type of the . - /// is a null reference + /// cref="ConcurrentDictionary{TKey,TValue}"/>. + /// is a null reference /// (Nothing in Visual Basic). - /// + /// /// A value is being assigned, and is of a type that is not assignable to the /// key type of the . -or- A value is being + /// cref="ConcurrentDictionary{TKey,TValue}"/>. -or- A value is being /// assigned, and is of a type that is not assignable to the value type /// of the + /// cref="ConcurrentDictionary{TKey,TValue}"/> /// object? IDictionary.this[object key] { get { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } - TValue value; - if (key is TKey && TryGetValue((TKey)key, out value)) + if (key is TKey tkey && TryGetValue(tkey, out TValue value)) { return value; } @@ -1627,41 +1546,71 @@ ICollection IDictionary.Values } set { - if (key == null) ThrowKeyNullException(); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (!(key is TKey)) + { + throw new ArgumentException(SR.ConcurrentDictionary_TypeOfKeyIncorrect); + } - if (!(key is TKey)) throw new ArgumentException(SR.ConcurrentDictionary_TypeOfKeyIncorrect); ThrowIfInvalidObjectValue(value); ((ConcurrentDictionary)this)[(TKey)key] = (TValue)value!; } } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static void ThrowIfInvalidObjectValue(object? value) + { + if (value != null) + { + if (!(value is TValue)) + { + ThrowHelper.ThrowValueNullException(); + } + } + else if (default(TValue) != null) + { + ThrowHelper.ThrowValueNullException(); + } + } + #endregion #region ICollection Members /// - /// Copies the elements of the to an array, starting + /// Copies the elements of the to an array, starting /// at the specified array index. /// /// The one-dimensional array that is the destination of the elements copied from - /// the . The array must have zero-based + /// the . The array must have zero-based /// indexing. /// The zero-based index in at which copying /// begins. - /// is a null reference + /// is a null reference /// (Nothing in Visual Basic). - /// is less than + /// is less than /// 0. - /// is equal to or greater than + /// is equal to or greater than /// the length of the . -or- The number of elements in the source + /// cref="ICollection"/> /// is greater than the available space from to the end of the destination /// . void ICollection.CopyTo(Array array, int index) { - if (array == null) throw new ArgumentNullException(nameof(array)); - if (index < 0) throw new ArgumentOutOfRangeException(nameof(index), SR.ConcurrentDictionary_IndexIsNegative); + if (array is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(array)); + } + + if (index < 0) + { + throw new ArgumentOutOfRangeException(nameof(index), SR.ConcurrentDictionary_IndexIsNegative); + } int locksAcquired = 0; try @@ -1687,22 +1636,19 @@ void ICollection.CopyTo(Array array, int index) // - an array of DictionaryEntry structs // - an array of objects - KeyValuePair[]? pairs = array as KeyValuePair[]; - if (pairs != null) + if (array is KeyValuePair[] pairs) { CopyToPairs(pairs, index); return; } - DictionaryEntry[]? entries = array as DictionaryEntry[]; - if (entries != null) + if (array is DictionaryEntry[] entries) { CopyToEntries(entries, index); return; } - object[]? objects = array as object[]; - if (objects != null) + if (array is object[] objects) { CopyToObjects(objects, index); return; @@ -1717,30 +1663,21 @@ void ICollection.CopyTo(Array array, int index) } /// - /// Gets a value indicating whether access to the is + /// Gets a value indicating whether access to the is /// synchronized with the SyncRoot. /// - /// true if access to the is synchronized + /// true if access to the is synchronized /// (thread safe); otherwise, false. For , this property always + /// cref="ConcurrentDictionary{TKey,TValue}"/>, this property always /// returns false. - bool ICollection.IsSynchronized - { - get { return false; } - } + bool ICollection.IsSynchronized => false; /// /// Gets an object that can be used to synchronize access to the . This property is not supported. + /// cref="ICollection"/>. This property is not supported. /// - /// The SyncRoot property is not supported. - object ICollection.SyncRoot - { - get - { - throw new NotSupportedException(SR.ConcurrentCollection_SyncRoot_NotSupported); - } - } + /// The SyncRoot property is not supported. + object ICollection.SyncRoot => throw new NotSupportedException(SR.ConcurrentCollection_SyncRoot_NotSupported); #endregion @@ -1788,7 +1725,6 @@ private void GrowTable(Tables tables) return; } - // Compute the new table size. We find the smallest integer larger than twice the previous table size, and not divisible by // 2,3,5 or 7. We can consider a different table-sizing policy in the future. int newLength = 0; @@ -1848,18 +1784,17 @@ private void GrowTable(Tables tables) } } - Node[] newBuckets = new Node[newLength]; - int[] newCountPerLock = new int[newLocks.Length]; + var newBuckets = new Node[newLength]; + var newCountPerLock = new int[newLocks.Length]; // Copy all data into a new table, creating new nodes for all elements for (int i = 0; i < tables._buckets.Length; i++) { - Node current = tables._buckets[i]; + Node? current = tables._buckets[i]; while (current != null) { - Node next = current._next; - int newBucketNo, newLockNo; - GetBucketAndLockNo(current._hashcode, out newBucketNo, out newLockNo, newBuckets.Length, newLocks.Length); + Node? next = current._next; + GetBucketAndLockNo(current._hashcode, out int newBucketNo, out int newLockNo, newBuckets.Length, newLocks.Length); newBuckets[newBucketNo] = new Node(current._key, current._value, current._hashcode, newBuckets[newBucketNo]); @@ -1984,17 +1919,18 @@ private ReadOnlyCollection GetKeys() AcquireAllLocks(ref locksAcquired); int count = GetCountInternal(); - if (count < 0) throw new OutOfMemoryException(); + if (count < 0) + { + ThrowHelper.ThrowOutOfMemoryException(); + } - List keys = new List(count); - Node[] buckets = _tables._buckets; + var keys = new List(count); + Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { - Node current = buckets[i]; - while (current != null) + for (Node? current = buckets[i]; current != null; current = current._next) { keys.Add(current._key); - current = current._next; } } @@ -2017,17 +1953,18 @@ private ReadOnlyCollection GetValues() AcquireAllLocks(ref locksAcquired); int count = GetCountInternal(); - if (count < 0) throw new OutOfMemoryException(); + if (count < 0) + { + ThrowHelper.ThrowOutOfMemoryException(); + } List values = new List(count); - Node[] buckets = _tables._buckets; + Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { - Node current = buckets[i]; - while (current != null) + for (Node? current = buckets[i]; current != null; current = current._next) { values.Add(current._value); - current = current._next; } } @@ -2046,10 +1983,10 @@ private sealed class Node { internal readonly TKey _key; internal TValue _value; - internal volatile Node _next; + internal volatile Node? _next; internal readonly int _hashcode; - internal Node(TKey key, TValue value, int hashcode, Node next) + internal Node(TKey key, TValue value, int hashcode, Node? next) { _key = key; _value = value; @@ -2058,6 +1995,28 @@ internal Node(TKey key, TValue value, int hashcode, Node next) } } + /// Tables that hold the internal state of the ConcurrentDictionary + /// + /// Wrapping the three tables in a single object allows us to atomically + /// replace all tables at once. + /// + private sealed class Tables + { + /// A singly-linked list for each bucket. + internal readonly Node?[] _buckets; + /// A set of locks, each guarding a section of the table. + internal readonly object[] _locks; + /// The number of elements guarded by each lock. + internal readonly int[] _countPerLock; + + internal Tables(Node?[] buckets, object[] locks, int[] countPerLock) + { + _buckets = buckets; + _locks = locks; + _countPerLock = countPerLock; + } + } + /// /// A private class to represent enumeration over the dictionary that implements the /// IDictionaryEnumerator interface. @@ -2066,61 +2025,42 @@ private sealed class DictionaryEnumerator : IDictionaryEnumerator { private readonly IEnumerator> _enumerator; // Enumerator over the dictionary. - internal DictionaryEnumerator(ConcurrentDictionary dictionary) - { - _enumerator = dictionary.GetEnumerator(); - } + internal DictionaryEnumerator(ConcurrentDictionary dictionary) => _enumerator = dictionary.GetEnumerator(); - public DictionaryEntry Entry - { - get { return new DictionaryEntry(_enumerator.Current.Key, _enumerator.Current.Value); } - } + public DictionaryEntry Entry => new DictionaryEntry(_enumerator.Current.Key, _enumerator.Current.Value); - public object Key - { - get { return _enumerator.Current.Key; } - } + public object Key => _enumerator.Current.Key; - public object? Value - { - get { return _enumerator.Current.Value; } - } + public object? Value => _enumerator.Current.Value; - public object Current - { - get { return Entry; } - } + public object Current => Entry; - public bool MoveNext() - { - return _enumerator.MoveNext(); - } + public bool MoveNext() => _enumerator.MoveNext(); - public void Reset() - { - _enumerator.Reset(); - } + public void Reset() => _enumerator.Reset(); } } - internal sealed class IDictionaryDebugView where K : notnull + internal sealed class IDictionaryDebugView where TKey : notnull { - private readonly IDictionary _dictionary; + private readonly IDictionary _dictionary; - public IDictionaryDebugView(IDictionary dictionary) + public IDictionaryDebugView(IDictionary dictionary) { - if (dictionary == null) - throw new ArgumentNullException(nameof(dictionary)); + if (dictionary is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(dictionary)); + } _dictionary = dictionary; } [DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] - public KeyValuePair[] Items + public KeyValuePair[] Items { get { - KeyValuePair[] items = new KeyValuePair[_dictionary.Count]; + var items = new KeyValuePair[_dictionary.Count]; _dictionary.CopyTo(items, 0); return items; } diff --git a/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs b/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs new file mode 100644 index 0000000000000..21429fb27af1b --- /dev/null +++ b/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs @@ -0,0 +1,30 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; + +namespace System +{ + internal static class ThrowHelper + { + [DoesNotReturn] + internal static void ThrowKeyNotFoundException(object key) => throw new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); + + [DoesNotReturn] + internal static void ThrowKeyNullException() => ThrowArgumentNullException("key"); + + [DoesNotReturn] + internal static void ThrowArgumentNullException(string name) => throw new ArgumentNullException(name); + + [DoesNotReturn] + internal static void ThrowArgumentNullException(string name, string message) => throw new ArgumentNullException(name, message); + + [DoesNotReturn] + internal static void ThrowValueNullException() => throw new ArgumentException(SR.ConcurrentDictionary_TypeOfValueIncorrect); + + [DoesNotReturn] + internal static void ThrowOutOfMemoryException() => throw new OutOfMemoryException(); + } +} From e758a9308582fc99594894585d99c0f2fc635e84 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 27 May 2020 09:28:56 -0400 Subject: [PATCH 2/7] Optimize ConcurrentDictionary for no comparer as well as value type keys --- .../Concurrent/ConcurrentDictionary.cs | 163 ++++++++++++++---- 1 file changed, 131 insertions(+), 32 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index ff364e23b9c4d..fc9a9bc8a5366 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -25,7 +25,13 @@ public class ConcurrentDictionary : IDictionary, IDi /// Internal tables of the dictionary. private volatile Tables _tables; /// Key equality comparer. - private readonly IEqualityComparer _comparer; + private readonly IEqualityComparer? _comparer; + /// Default comparer for TKey. + /// + /// Used to avoid repeatedly accessing the shared default generic static, in particular for reference types where it's + /// currently not devirtualized: https://github.com/dotnet/runtime/issues/10050. + /// + private readonly EqualityComparer _defaultComparer; /// Whether to dynamically increase the size of the striped lock. private readonly bool _growLockArray; /// The maximum number of elements per lock before a resize operation is triggered. @@ -58,13 +64,12 @@ private static bool IsValueWriteAtomic() // Section 12.6.6 of ECMA CLI explains which types can be read and written atomically without // the risk of tearing. See https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf - Type valueType = typeof(TValue); - if (!valueType.IsValueType) + if (!typeof(TValue).IsValueType) { return true; } - switch (Type.GetTypeCode(valueType)) + switch (Type.GetTypeCode(typeof(TValue))) { case TypeCode.Boolean: case TypeCode.Byte: @@ -179,7 +184,7 @@ private void InitializeFromCollection(IEnumerable> co ThrowHelper.ThrowKeyNullException(); } - if (!TryAddInternal(pair.Key, _comparer.GetHashCode(pair.Key), pair.Value, updateIfExists: false, acquireLock: false, out _)) + if (!TryAddInternal(pair.Key, null, pair.Value, updateIfExists: false, acquireLock: false, out _)) { throw new ArgumentException(SR.ConcurrentDictionary_SourceContainsDuplicateKeys); } @@ -224,7 +229,8 @@ internal ConcurrentDictionary(int concurrencyLevel, int capacity, bool growLockA } var locks = new object[concurrencyLevel]; - for (int i = 0; i < locks.Length; i++) + locks[0] = locks; // reuse array as the first lock object just to avoid an additional allocation + for (int i = 1; i < locks.Length; i++) { locks[i] = new object(); } @@ -233,7 +239,8 @@ internal ConcurrentDictionary(int concurrencyLevel, int capacity, bool growLockA var buckets = new Node[capacity]; _tables = new Tables(buckets, locks, countPerLock); - _comparer = comparer ?? EqualityComparer.Default; + _defaultComparer = EqualityComparer.Default; + _comparer = ReferenceEquals(_defaultComparer, comparer) ? null : comparer; _growLockArray = growLockArray; _budget = buckets.Length / locks.Length; } @@ -256,7 +263,7 @@ public bool TryAdd(TKey key, TValue value) ThrowHelper.ThrowKeyNullException(); } - return TryAddInternal(key, _comparer.GetHashCode(key), value, updateIfExists: false, acquireLock: true, out _); + return TryAddInternal(key, null, value, updateIfExists: false, acquireLock: true, out _); } /// @@ -332,7 +339,7 @@ public bool TryRemove(KeyValuePair item) /// The conditional value to compare against if is true private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value, bool matchValue, [AllowNull] TValue oldValue) { - int hashcode = _comparer.GetHashCode(key); + int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); while (true) { Tables tables = _tables; @@ -352,7 +359,7 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value { Debug.Assert((prev is null && curr == tables._buckets[bucketNo]) || prev!._next == curr); - if (hashcode == curr._hashcode && _comparer.Equals(curr._key, key)) + if (hashcode == curr._hashcode && (_comparer is null ? _defaultComparer.Equals(curr._key, key) : _comparer.Equals(curr._key, key))) { if (matchValue) { @@ -404,12 +411,60 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) ThrowHelper.ThrowKeyNullException(); } - return TryGetValueInternal(key, _comparer.GetHashCode(key), out value); + // We must capture the volatile _tables field into a local variable: it is set to a new table on each table resize. + // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: + // this protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. + Tables tables = _tables; + + if (_comparer is null) + { + int hashcode = key.GetHashCode(); + int bucketNo = GetBucket(hashcode, tables._buckets.Length); + if (typeof(TKey).IsValueType) + { + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + { + if (hashcode == n._hashcode && EqualityComparer.Default.Equals(n._key, key)) + { + value = n._value; + return true; + } + } + } + else + { + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + { + if (hashcode == n._hashcode && _defaultComparer.Equals(n._key, key)) + { + value = n._value; + return true; + } + } + } + } + else + { + + int hashcode = _comparer.GetHashCode(key); + int bucketNo = GetBucket(hashcode, tables._buckets.Length); + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + { + if (hashcode == n._hashcode && _comparer.Equals(n._key, key)) + { + value = n._value; + return true; + } + } + } + + value = default; + return false; } private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] out TValue value) { - Debug.Assert(_comparer.GetHashCode(key) == hashcode); + Debug.Assert((_comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key)) == hashcode); // We must capture the volatile _tables field into a local variable: it is set to a new table on each table resize. // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: @@ -418,12 +473,40 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] int bucketNo = GetBucket(hashcode, tables._buckets.Length); - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + if (_comparer is null) { - if (hashcode == n._hashcode && _comparer.Equals(n._key, key)) + if (typeof(TKey).IsValueType) { - value = n._value; - return true; + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + { + if (hashcode == n._hashcode && EqualityComparer.Default.Equals(n._key, key)) + { + value = n._value; + return true; + } + } + } + else + { + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + { + if (hashcode == n._hashcode && _defaultComparer.Equals(n._key, key)) + { + value = n._value; + return true; + } + } + } + } + else + { + for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + { + if (hashcode == n._hashcode && _comparer.Equals(n._key, key)) + { + value = n._value; + return true; + } } } @@ -453,7 +536,7 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) ThrowHelper.ThrowKeyNullException(); } - return TryUpdateInternal(key, _comparer.GetHashCode(key), newValue, comparisonValue); + return TryUpdateInternal(key, null, newValue, comparisonValue); } /// @@ -462,7 +545,7 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) /// /// The key whose value is compared with and /// possibly replaced. - /// The hashcode computed for . + /// The hashcode computed for . /// The value that replaces the value of the element with if the comparison results in equality. /// The value that is compared to the value of the element with @@ -472,11 +555,19 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) /// replaced with ; otherwise, false. /// /// is a null reference. - private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue comparisonValue) + private bool TryUpdateInternal(TKey key, int? nullableHashcode, TValue newValue, TValue comparisonValue) { - Debug.Assert(_comparer.GetHashCode(key) == hashcode); + Debug.Assert( + nullableHashcode is null || + (_comparer is null && key.GetHashCode() == nullableHashcode) || + (_comparer != null && _comparer.GetHashCode(key) == nullableHashcode)); + + int hashcode = + nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : + _comparer is null ? key.GetHashCode() : + _comparer.GetHashCode(key); - IEqualityComparer valueComparer = EqualityComparer.Default; + EqualityComparer valueComparer = EqualityComparer.Default; while (true) { @@ -497,7 +588,7 @@ private bool TryUpdateInternal(TKey key, int hashcode, TValue newValue, TValue c for (Node? node = tables._buckets[bucketNo]; node != null; node = node._next) { Debug.Assert((prev is null && node == tables._buckets[bucketNo]) || prev!._next == node); - if (hashcode == node._hashcode && _comparer.Equals(node._key, key)) + if (hashcode == node._hashcode && (_comparer is null ? _defaultComparer.Equals(node._key, key) : _comparer.Equals(node._key, key))) { if (valueComparer.Equals(node._value, comparisonValue)) { @@ -718,9 +809,17 @@ public IEnumerator> GetEnumerator() /// If key exists, we always return false; and if updateIfExists == true we force update with value; /// If key doesn't exist, we always add value and return true; /// - private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) + private bool TryAddInternal(TKey key, int? nullableHashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) { - Debug.Assert(_comparer.GetHashCode(key) == hashcode); + Debug.Assert( + nullableHashcode is null || + (_comparer is null && key.GetHashCode() == nullableHashcode) || + (_comparer != null && _comparer.GetHashCode(key) == nullableHashcode)); + + int hashcode = + nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : + _comparer is null ? key.GetHashCode() : + _comparer.GetHashCode(key); while (true) { @@ -748,7 +847,7 @@ private bool TryAddInternal(TKey key, int hashcode, TValue value, bool updateIfE for (Node? node = tables._buckets[bucketNo]; node != null; node = node._next) { Debug.Assert((prev is null && node == tables._buckets[bucketNo]) || prev!._next == node); - if (hashcode == node._hashcode && _comparer.Equals(node._key, key)) + if (hashcode == node._hashcode && (_comparer is null ? _defaultComparer.Equals(node._key, key) : _comparer.Equals(node._key, key))) { // The key was found in the dictionary. If updates are allowed, update the value for that key. // We need to create a new node for the update, in order to support TValue types that cannot @@ -854,7 +953,7 @@ public TValue this[TKey key] ThrowHelper.ThrowKeyNullException(); } - TryAddInternal(key, _comparer.GetHashCode(key), value, updateIfExists: true, acquireLock: true, out _); + TryAddInternal(key, null, value, updateIfExists: true, acquireLock: true, out _); } } @@ -942,7 +1041,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory) ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); } - int hashcode = _comparer.GetHashCode(key); + int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -980,7 +1079,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory, TA ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); } - int hashcode = _comparer.GetHashCode(key); + int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1009,7 +1108,7 @@ public TValue GetOrAdd(TKey key, TValue value) ThrowHelper.ThrowKeyNullException(); } - int hashcode = _comparer.GetHashCode(key); + int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1057,7 +1156,7 @@ public TValue AddOrUpdate( ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); } - int hashcode = _comparer.GetHashCode(key); + int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); while (true) { @@ -1117,7 +1216,7 @@ public TValue AddOrUpdate(TKey key, Func addValueFactory, Func ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); } - int hashcode = _comparer.GetHashCode(key); + int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); while (true) { From 1901c2250ffad1166716c5101d2eb0fbde3a2085 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 27 May 2020 09:36:29 -0400 Subject: [PATCH 3/7] Special-case StringComparer.Ordinal in ConcurrentDictionary --- .../Concurrent/ConcurrentDictionary.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index fc9a9bc8a5366..8b87e47c786e3 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -192,7 +192,8 @@ private void InitializeFromCollection(IEnumerable> co if (_budget == 0) { - _budget = _tables._buckets.Length / _tables._locks.Length; + Tables tables = _tables; + _budget = tables._buckets.Length / tables._locks.Length; } } @@ -240,7 +241,12 @@ internal ConcurrentDictionary(int concurrencyLevel, int capacity, bool growLockA _tables = new Tables(buckets, locks, countPerLock); _defaultComparer = EqualityComparer.Default; - _comparer = ReferenceEquals(_defaultComparer, comparer) ? null : comparer; + if (comparer != null && + !ReferenceEquals(comparer, _defaultComparer) && // if this is the default comparer, take the optimized path + !ReferenceEquals(comparer, StringComparer.Ordinal)) // strings as keys are extremely common, so special-case StringComparer.Ordinal, which is the same as the default comparer + { + _comparer = comparer; + } _growLockArray = growLockArray; _budget = buckets.Length / locks.Length; } @@ -635,7 +641,8 @@ public void Clear() { AcquireAllLocks(ref locksAcquired); - var newTables = new Tables(new Node[DefaultCapacity], _tables._locks, new int[_tables._countPerLock.Length]); + Tables tables = _tables; + var newTables = new Tables(new Node[DefaultCapacity], tables._locks, new int[tables._countPerLock.Length]); _tables = newTables; _budget = Math.Max(1, newTables._buckets.Length / newTables._locks.Length); } @@ -2001,9 +2008,10 @@ private void ReleaseLocks(int fromInclusive, int toExclusive) { Debug.Assert(fromInclusive <= toExclusive); + Tables tables = _tables; for (int i = fromInclusive; i < toExclusive; i++) { - Monitor.Exit(_tables._locks[i]); + Monitor.Exit(tables._locks[i]); } } From 458f65f3371c4804e3ed3510fde82e15aea68e32 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 27 May 2020 09:50:10 -0400 Subject: [PATCH 4/7] Store repeatedly used fields into locals --- .../Concurrent/ConcurrentDictionary.cs | 127 ++++++++++-------- 1 file changed, 73 insertions(+), 54 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index 8b87e47c786e3..d871ee5247e7b 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -345,13 +345,16 @@ public bool TryRemove(KeyValuePair item) /// The conditional value to compare against if is true private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value, bool matchValue, [AllowNull] TValue oldValue) { - int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); + IEqualityComparer? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { Tables tables = _tables; - GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, tables._buckets.Length, tables._locks.Length); + Node?[] buckets = tables._buckets; + object[] locks = tables._locks; + GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, buckets.Length, locks.Length); - lock (tables._locks[lockNo]) + lock (locks[lockNo]) { // If the table just got resized, we may not be holding the right lock, and must retry. // This should be a rare occurrence. @@ -361,11 +364,11 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value } Node? prev = null; - for (Node? curr = tables._buckets[bucketNo]; curr != null; curr = curr._next) + for (Node? curr = buckets[bucketNo]; curr != null; curr = curr._next) { - Debug.Assert((prev is null && curr == tables._buckets[bucketNo]) || prev!._next == curr); + Debug.Assert((prev is null && curr == buckets[bucketNo]) || prev!._next == curr); - if (hashcode == curr._hashcode && (_comparer is null ? _defaultComparer.Equals(curr._key, key) : _comparer.Equals(curr._key, key))) + if (hashcode == curr._hashcode && (comparer is null ? _defaultComparer.Equals(curr._key, key) : comparer.Equals(curr._key, key))) { if (matchValue) { @@ -379,7 +382,7 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value if (prev is null) { - Volatile.Write(ref tables._buckets[bucketNo], curr._next); + Volatile.Write(ref buckets[bucketNo], curr._next); } else { @@ -421,14 +424,16 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: // this protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. Tables tables = _tables; + Node?[] buckets = tables._buckets; - if (_comparer is null) + IEqualityComparer? comparer = _comparer; + if (comparer is null) { int hashcode = key.GetHashCode(); - int bucketNo = GetBucket(hashcode, tables._buckets.Length); + int bucketNo = GetBucket(hashcode, buckets.Length); if (typeof(TKey).IsValueType) { - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) { if (hashcode == n._hashcode && EqualityComparer.Default.Equals(n._key, key)) { @@ -439,7 +444,7 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) } else { - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) { if (hashcode == n._hashcode && _defaultComparer.Equals(n._key, key)) { @@ -451,12 +456,11 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) } else { - - int hashcode = _comparer.GetHashCode(key); - int bucketNo = GetBucket(hashcode, tables._buckets.Length); - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + int hashcode = comparer.GetHashCode(key); + uint bucketNo = GetBucket(hashcode, buckets.Length); + for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) { - if (hashcode == n._hashcode && _comparer.Equals(n._key, key)) + if (hashcode == n._hashcode && comparer.Equals(n._key, key)) { value = n._value; return true; @@ -476,14 +480,15 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: // this protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. Tables tables = _tables; + Node?[] buckets = tables._buckets; + int bucketNo = GetBucket(hashcode, buckets.Length); - int bucketNo = GetBucket(hashcode, tables._buckets.Length); - - if (_comparer is null) + IEqualityComparer? comparer = _comparer; + if (comparer is null) { if (typeof(TKey).IsValueType) { - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) { if (hashcode == n._hashcode && EqualityComparer.Default.Equals(n._key, key)) { @@ -494,7 +499,7 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] } else { - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) { if (hashcode == n._hashcode && _defaultComparer.Equals(n._key, key)) { @@ -506,9 +511,9 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] } else { - for (Node? n = Volatile.Read(ref tables._buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) { - if (hashcode == n._hashcode && _comparer.Equals(n._key, key)) + if (hashcode == n._hashcode && comparer.Equals(n._key, key)) { value = n._value; return true; @@ -563,24 +568,28 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) /// is a null reference. private bool TryUpdateInternal(TKey key, int? nullableHashcode, TValue newValue, TValue comparisonValue) { + IEqualityComparer? comparer = _comparer; + Debug.Assert( nullableHashcode is null || - (_comparer is null && key.GetHashCode() == nullableHashcode) || - (_comparer != null && _comparer.GetHashCode(key) == nullableHashcode)); + (comparer is null && key.GetHashCode() == nullableHashcode) || + (comparer != null && comparer.GetHashCode(key) == nullableHashcode)); int hashcode = nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : - _comparer is null ? key.GetHashCode() : - _comparer.GetHashCode(key); + comparer is null ? key.GetHashCode() : + comparer.GetHashCode(key); EqualityComparer valueComparer = EqualityComparer.Default; while (true) { Tables tables = _tables; - GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, tables._buckets.Length, tables._locks.Length); + Node?[] buckets = tables._buckets; + object[] locks = tables._locks; + GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, buckets.Length, locks.Length); - lock (tables._locks[lockNo]) + lock (locks[lockNo]) { // If the table just got resized, we may not be holding the right lock, and must retry. // This should be a rare occurrence. @@ -591,10 +600,10 @@ nullableHashcode is null || // Try to find this key in the bucket Node? prev = null; - for (Node? node = tables._buckets[bucketNo]; node != null; node = node._next) + for (Node? node = buckets[bucketNo]; node != null; node = node._next) { - Debug.Assert((prev is null && node == tables._buckets[bucketNo]) || prev!._next == node); - if (hashcode == node._hashcode && (_comparer is null ? _defaultComparer.Equals(node._key, key) : _comparer.Equals(node._key, key))) + Debug.Assert((prev is null && node == buckets[bucketNo]) || prev!._next == node); + if (hashcode == node._hashcode && (comparer is null ? _defaultComparer.Equals(node._key, key) : comparer.Equals(node._key, key))) { if (valueComparer.Equals(node._value, comparisonValue)) { @@ -608,7 +617,7 @@ nullableHashcode is null || if (prev is null) { - Volatile.Write(ref tables._buckets[bucketNo], newNode); + Volatile.Write(ref buckets[bucketNo], newNode); } else { @@ -818,20 +827,24 @@ public IEnumerator> GetEnumerator() /// private bool TryAddInternal(TKey key, int? nullableHashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) { + IEqualityComparer? comparer = _comparer; + Debug.Assert( nullableHashcode is null || - (_comparer is null && key.GetHashCode() == nullableHashcode) || - (_comparer != null && _comparer.GetHashCode(key) == nullableHashcode)); + (comparer is null && key.GetHashCode() == nullableHashcode) || + (comparer != null && comparer.GetHashCode(key) == nullableHashcode)); int hashcode = nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : - _comparer is null ? key.GetHashCode() : - _comparer.GetHashCode(key); + comparer is null ? key.GetHashCode() : + comparer.GetHashCode(key); while (true) { Tables tables = _tables; - GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, tables._buckets.Length, tables._locks.Length); + Node?[] buckets = tables._buckets; + object[] locks = tables._locks; + GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, buckets.Length, locks.Length); bool resizeDesired = false; bool lockTaken = false; @@ -839,7 +852,7 @@ nullableHashcode is null || { if (acquireLock) { - Monitor.Enter(tables._locks[lockNo], ref lockTaken); + Monitor.Enter(locks[lockNo], ref lockTaken); } // If the table just got resized, we may not be holding the right lock, and must retry. @@ -851,10 +864,10 @@ nullableHashcode is null || // Try to find this key in the bucket Node? prev = null; - for (Node? node = tables._buckets[bucketNo]; node != null; node = node._next) + for (Node? node = buckets[bucketNo]; node != null; node = node._next) { - Debug.Assert((prev is null && node == tables._buckets[bucketNo]) || prev!._next == node); - if (hashcode == node._hashcode && (_comparer is null ? _defaultComparer.Equals(node._key, key) : _comparer.Equals(node._key, key))) + Debug.Assert((prev is null && node == buckets[bucketNo]) || prev!._next == node); + if (hashcode == node._hashcode && (comparer is null ? _defaultComparer.Equals(node._key, key) : comparer.Equals(node._key, key))) { // The key was found in the dictionary. If updates are allowed, update the value for that key. // We need to create a new node for the update, in order to support TValue types that cannot @@ -870,7 +883,7 @@ nullableHashcode is null || var newNode = new Node(node._key, value, hashcode, node._next); if (prev is null) { - Volatile.Write(ref tables._buckets[bucketNo], newNode); + Volatile.Write(ref buckets[bucketNo], newNode); } else { @@ -889,7 +902,7 @@ nullableHashcode is null || } // The key was not found in the bucket. Insert the key-value pair. - Volatile.Write(ref tables._buckets[bucketNo], new Node(key, value, hashcode, tables._buckets[bucketNo])); + Volatile.Write(ref buckets[bucketNo], new Node(key, value, hashcode, buckets[bucketNo])); checked { tables._countPerLock[lockNo]++; @@ -909,7 +922,7 @@ nullableHashcode is null || { if (lockTaken) { - Monitor.Exit(tables._locks[lockNo]); + Monitor.Exit(locks[lockNo]); } } @@ -1048,7 +1061,8 @@ public TValue GetOrAdd(TKey key, Func valueFactory) ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); } - int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); + IEqualityComparer? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1086,7 +1100,8 @@ public TValue GetOrAdd(TKey key, Func valueFactory, TA ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); } - int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); + IEqualityComparer? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1115,7 +1130,8 @@ public TValue GetOrAdd(TKey key, TValue value) ThrowHelper.ThrowKeyNullException(); } - int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); + IEqualityComparer? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1163,7 +1179,8 @@ public TValue AddOrUpdate( ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); } - int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); + IEqualityComparer? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { @@ -1223,7 +1240,8 @@ public TValue AddOrUpdate(TKey key, Func addValueFactory, Func? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { @@ -1276,7 +1294,8 @@ public TValue AddOrUpdate(TKey key, TValue addValue, Func ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); } - int hashcode = _comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key); + IEqualityComparer? comparer = _comparer; + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { @@ -1894,9 +1913,9 @@ private void GrowTable(Tables tables) var newCountPerLock = new int[newLocks.Length]; // Copy all data into a new table, creating new nodes for all elements - for (int i = 0; i < tables._buckets.Length; i++) + foreach (Node? bucket in tables._buckets) { - Node? current = tables._buckets[i]; + Node? current = bucket; while (current != null) { Node? next = current._next; @@ -2065,7 +2084,7 @@ private ReadOnlyCollection GetValues() ThrowHelper.ThrowOutOfMemoryException(); } - List values = new List(count); + var values = new List(count); Node?[] buckets = _tables._buckets; for (int i = 0; i < buckets.Length; i++) { From dd4d1c81c98031d441a88e11efc183985658c82d Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 27 May 2020 11:24:30 -0400 Subject: [PATCH 5/7] Use FastMod in ConcurrentDictionary --- .../src/Resources/Strings.resx | 3 + .../src/System.Collections.Concurrent.csproj | 2 + .../Concurrent/ConcurrentDictionary.cs | 162 ++++++++++-------- .../src/System/Collections/HashHelpers.cs | 11 +- 4 files changed, 97 insertions(+), 81 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx b/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx index 8389f524bebb2..f09a978890740 100644 --- a/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx +++ b/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx @@ -180,4 +180,7 @@ The given key '{0}' was not present in the dictionary. + + Hashtable's capacity overflowed and went negative. Check load factor, capacity and the current size of the table. + diff --git a/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj b/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj index 516f92d46f22b..a5b13e63ed356 100644 --- a/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj +++ b/src/libraries/System.Collections.Concurrent/src/System.Collections.Concurrent.csproj @@ -16,6 +16,8 @@ + diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index d871ee5247e7b..0fc31a96bea39 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -346,13 +346,12 @@ public bool TryRemove(KeyValuePair item) private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value, bool matchValue, [AllowNull] TValue oldValue) { IEqualityComparer? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); while (true) { Tables tables = _tables; - Node?[] buckets = tables._buckets; object[] locks = tables._locks; - GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, buckets.Length, locks.Length); + ref Node? bucket = ref tables.GetBucketAndLock(hashcode, out uint lockNo); lock (locks[lockNo]) { @@ -364,9 +363,9 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value } Node? prev = null; - for (Node? curr = buckets[bucketNo]; curr != null; curr = curr._next) + for (Node? curr = bucket; curr != null; curr = curr._next) { - Debug.Assert((prev is null && curr == buckets[bucketNo]) || prev!._next == curr); + Debug.Assert((prev is null && curr == bucket) || prev!._next == curr); if (hashcode == curr._hashcode && (comparer is null ? _defaultComparer.Equals(curr._key, key) : comparer.Equals(curr._key, key))) { @@ -382,7 +381,7 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value if (prev is null) { - Volatile.Write(ref buckets[bucketNo], curr._next); + Volatile.Write(ref bucket, curr._next); } else { @@ -424,16 +423,14 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: // this protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. Tables tables = _tables; - Node?[] buckets = tables._buckets; IEqualityComparer? comparer = _comparer; if (comparer is null) { - int hashcode = key.GetHashCode(); - int bucketNo = GetBucket(hashcode, buckets.Length); + uint hashcode = (uint)key.GetHashCode(); if (typeof(TKey).IsValueType) { - for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && EqualityComparer.Default.Equals(n._key, key)) { @@ -444,7 +441,7 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) } else { - for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && _defaultComparer.Equals(n._key, key)) { @@ -456,9 +453,8 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) } else { - int hashcode = comparer.GetHashCode(key); - uint bucketNo = GetBucket(hashcode, buckets.Length); - for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) + uint hashcode = (uint)comparer.GetHashCode(key); + for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && comparer.Equals(n._key, key)) { @@ -472,23 +468,21 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) return false; } - private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] out TValue value) + private bool TryGetValueInternal(TKey key, uint hashcode, [MaybeNullWhen(false)] out TValue value) { - Debug.Assert((_comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key)) == hashcode); + Debug.Assert((uint)(_comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key)) == hashcode); // We must capture the volatile _tables field into a local variable: it is set to a new table on each table resize. // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: // this protects us from reading fields ('_hashcode', '_key', '_value' and '_next') of different instances. Tables tables = _tables; - Node?[] buckets = tables._buckets; - int bucketNo = GetBucket(hashcode, buckets.Length); IEqualityComparer? comparer = _comparer; if (comparer is null) { if (typeof(TKey).IsValueType) { - for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && EqualityComparer.Default.Equals(n._key, key)) { @@ -499,7 +493,7 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] } else { - for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && _defaultComparer.Equals(n._key, key)) { @@ -511,7 +505,7 @@ private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] } else { - for (Node? n = Volatile.Read(ref buckets[bucketNo]); n != null; n = n._next) + for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && comparer.Equals(n._key, key)) { @@ -566,7 +560,7 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) /// replaced with ; otherwise, false. /// /// is a null reference. - private bool TryUpdateInternal(TKey key, int? nullableHashcode, TValue newValue, TValue comparisonValue) + private bool TryUpdateInternal(TKey key, uint? nullableHashcode, TValue newValue, TValue comparisonValue) { IEqualityComparer? comparer = _comparer; @@ -575,19 +569,18 @@ nullableHashcode is null || (comparer is null && key.GetHashCode() == nullableHashcode) || (comparer != null && comparer.GetHashCode(key) == nullableHashcode)); - int hashcode = + uint hashcode = nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : - comparer is null ? key.GetHashCode() : - comparer.GetHashCode(key); + comparer is null ? (uint)key.GetHashCode() : + (uint)comparer.GetHashCode(key); EqualityComparer valueComparer = EqualityComparer.Default; while (true) { Tables tables = _tables; - Node?[] buckets = tables._buckets; object[] locks = tables._locks; - GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, buckets.Length, locks.Length); + ref Node? bucket = ref tables.GetBucketAndLock(hashcode, out uint lockNo); lock (locks[lockNo]) { @@ -600,9 +593,9 @@ nullableHashcode is null || // Try to find this key in the bucket Node? prev = null; - for (Node? node = buckets[bucketNo]; node != null; node = node._next) + for (Node? node = bucket; node != null; node = node._next) { - Debug.Assert((prev is null && node == buckets[bucketNo]) || prev!._next == node); + Debug.Assert((prev is null && node == bucket) || prev!._next == node); if (hashcode == node._hashcode && (comparer is null ? _defaultComparer.Equals(node._key, key) : comparer.Equals(node._key, key))) { if (valueComparer.Equals(node._value, comparisonValue)) @@ -617,7 +610,7 @@ nullableHashcode is null || if (prev is null) { - Volatile.Write(ref buckets[bucketNo], newNode); + Volatile.Write(ref bucket, newNode); } else { @@ -825,7 +818,7 @@ public IEnumerator> GetEnumerator() /// If key exists, we always return false; and if updateIfExists == true we force update with value; /// If key doesn't exist, we always add value and return true; /// - private bool TryAddInternal(TKey key, int? nullableHashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) + private bool TryAddInternal(TKey key, uint? nullableHashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) { IEqualityComparer? comparer = _comparer; @@ -834,17 +827,16 @@ nullableHashcode is null || (comparer is null && key.GetHashCode() == nullableHashcode) || (comparer != null && comparer.GetHashCode(key) == nullableHashcode)); - int hashcode = + uint hashcode = nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : - comparer is null ? key.GetHashCode() : - comparer.GetHashCode(key); + comparer is null ? (uint)key.GetHashCode() : + (uint)comparer.GetHashCode(key); while (true) { Tables tables = _tables; - Node?[] buckets = tables._buckets; object[] locks = tables._locks; - GetBucketAndLockNo(hashcode, out int bucketNo, out int lockNo, buckets.Length, locks.Length); + ref Node? bucket = ref tables.GetBucketAndLock(hashcode, out uint lockNo); bool resizeDesired = false; bool lockTaken = false; @@ -864,9 +856,9 @@ nullableHashcode is null || // Try to find this key in the bucket Node? prev = null; - for (Node? node = buckets[bucketNo]; node != null; node = node._next) + for (Node? node = bucket; node != null; node = node._next) { - Debug.Assert((prev is null && node == buckets[bucketNo]) || prev!._next == node); + Debug.Assert((prev is null && node == bucket) || prev!._next == node); if (hashcode == node._hashcode && (comparer is null ? _defaultComparer.Equals(node._key, key) : comparer.Equals(node._key, key))) { // The key was found in the dictionary. If updates are allowed, update the value for that key. @@ -883,7 +875,7 @@ nullableHashcode is null || var newNode = new Node(node._key, value, hashcode, node._next); if (prev is null) { - Volatile.Write(ref buckets[bucketNo], newNode); + Volatile.Write(ref bucket, newNode); } else { @@ -902,7 +894,8 @@ nullableHashcode is null || } // The key was not found in the bucket. Insert the key-value pair. - Volatile.Write(ref buckets[bucketNo], new Node(key, value, hashcode, buckets[bucketNo])); + var resultNode = new Node(key, value, hashcode, bucket); + Volatile.Write(ref bucket, resultNode); checked { tables._countPerLock[lockNo]++; @@ -1062,7 +1055,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory) } IEqualityComparer? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1101,7 +1094,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory, TA } IEqualityComparer? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1131,7 +1124,7 @@ public TValue GetOrAdd(TKey key, TValue value) } IEqualityComparer? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1180,7 +1173,7 @@ public TValue AddOrUpdate( } IEqualityComparer? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); while (true) { @@ -1241,7 +1234,7 @@ public TValue AddOrUpdate(TKey key, Func addValueFactory, Func? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); while (true) { @@ -1295,7 +1288,7 @@ public TValue AddOrUpdate(TKey key, TValue addValue, Func } IEqualityComparer? comparer = _comparer; - int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); + uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); while (true) { @@ -1911,6 +1904,7 @@ private void GrowTable(Tables tables) var newBuckets = new Node[newLength]; var newCountPerLock = new int[newLocks.Length]; + var newTables = new Tables(newBuckets, newLocks, newCountPerLock); // Copy all data into a new table, creating new nodes for all elements foreach (Node? bucket in tables._buckets) @@ -1919,9 +1913,9 @@ private void GrowTable(Tables tables) while (current != null) { Node? next = current._next; - GetBucketAndLockNo(current._hashcode, out int newBucketNo, out int newLockNo, newBuckets.Length, newLocks.Length); + ref Node? newBucket = ref newTables.GetBucketAndLock(current._hashcode, out uint newLockNo); - newBuckets[newBucketNo] = new Node(current._key, current._value, current._hashcode, newBuckets[newBucketNo]); + newBucket = new Node(current._key, current._value, current._hashcode, newBucket); checked { @@ -1936,7 +1930,7 @@ private void GrowTable(Tables tables) _budget = Math.Max(1, newBuckets.Length / newLocks.Length); // Replace tables with the new versions - _tables = new Tables(newBuckets, newLocks, newCountPerLock); + _tables = newTables; } finally { @@ -1945,31 +1939,7 @@ private void GrowTable(Tables tables) } } - /// - /// Computes the bucket for a particular key. - /// - private static int GetBucket(int hashcode, int bucketCount) - { - int bucketNo = (hashcode & 0x7fffffff) % bucketCount; - Debug.Assert(bucketNo >= 0 && bucketNo < bucketCount); - return bucketNo; - } - - /// - /// Computes the bucket and lock number for a particular key. - /// - private static void GetBucketAndLockNo(int hashcode, out int bucketNo, out int lockNo, int bucketCount, int lockCount) - { - bucketNo = (hashcode & 0x7fffffff) % bucketCount; - lockNo = bucketNo % lockCount; - - Debug.Assert(bucketNo >= 0 && bucketNo < bucketCount); - Debug.Assert(lockNo >= 0 && lockNo < lockCount); - } - - /// - /// The number of concurrent writes for which to optimize by default. - /// + /// The number of concurrent writes for which to optimize by default. private static int DefaultConcurrencyLevel => Environment.ProcessorCount; /// @@ -2110,9 +2080,9 @@ private sealed class Node internal readonly TKey _key; internal TValue _value; internal volatile Node? _next; - internal readonly int _hashcode; + internal readonly uint _hashcode; - internal Node(TKey key, TValue value, int hashcode, Node? next) + internal Node(TKey key, TValue value, uint hashcode, Node? next) { _key = key; _value = value; @@ -2134,12 +2104,52 @@ private sealed class Tables internal readonly object[] _locks; /// The number of elements guarded by each lock. internal readonly int[] _countPerLock; + /// Pre-computed multiplier for use on 64-bit performing faster modulo operations. + internal readonly ulong _fastModBucketsMultiplier; internal Tables(Node?[] buckets, object[] locks, int[] countPerLock) { _buckets = buckets; _locks = locks; _countPerLock = countPerLock; + if (IntPtr.Size == 8) + { + _fastModBucketsMultiplier = HashHelpers.GetFastModMultiplier((uint)buckets.Length); + } + } + + /// Computes a ref to the bucket for a particular key. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal ref Node? GetBucket(uint hashcode) + { + Node?[] buckets = _buckets; + if (IntPtr.Size == 8) + { + return ref buckets[HashHelpers.FastMod(hashcode, (uint)buckets.Length, _fastModBucketsMultiplier)]; + } + else + { + return ref buckets[hashcode % (uint)buckets.Length]; + } + } + + /// Computes the bucket and lock number for a particular key. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal ref Node? GetBucketAndLock(uint hashcode, out uint lockNo) + { + Node?[] buckets = _buckets; + if (IntPtr.Size == 8) + { + uint bucketNo = HashHelpers.FastMod(hashcode, (uint)buckets.Length, _fastModBucketsMultiplier); + lockNo = bucketNo % (uint)_locks.Length; + return ref buckets[bucketNo]; + } + else + { + uint bucketNo = hashcode % (uint)_buckets.Length; + lockNo = bucketNo % (uint)_locks.Length; + return ref buckets[bucketNo]; + } } } 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 9ae197ee1bcf9..e233291ef8f82 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs @@ -89,11 +89,13 @@ public static int ExpandPrime(int oldSize) return GetPrime(newSize); } -#if TARGET_64BIT - // Returns approximate reciprocal of the divisor: ceil(2**64 / divisor) - public static ulong GetFastModMultiplier(uint divisor) - => ulong.MaxValue / divisor + 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) => + ulong.MaxValue / divisor + 1; + /// Performs a mod operation using the multiplier pre-computed with . + /// This should only be used on 64-bit. [MethodImpl(MethodImplOptions.AggressiveInlining)] public static uint FastMod(uint value, uint divisor, ulong multiplier) { @@ -108,6 +110,5 @@ public static uint FastMod(uint value, uint divisor, ulong multiplier) Debug.Assert(highbits == value % divisor); return highbits; } -#endif } } From 32c038312763bb11635f459ebe976c0de05b86b4 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 27 May 2020 11:44:34 -0400 Subject: [PATCH 6/7] Allow atomic writes of {U}IntPtr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From ECMA 335: "The native size types (native int, native unsigned int, and &) are always naturally aligned (4 bytes or 8 bytes, depending on the architecture)" and "A conforming CLI shall guarantee that read and write access to properly aligned memory locations no larger than the native word size (the size of type native int) is atomic (see §I.12.6.2) when all the write accesses to a location are the same size. " --- .../src/System/Collections/Concurrent/ConcurrentDictionary.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index 0fc31a96bea39..c9ab4d050cab3 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -64,7 +64,9 @@ private static bool IsValueWriteAtomic() // Section 12.6.6 of ECMA CLI explains which types can be read and written atomically without // the risk of tearing. See https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf - if (!typeof(TValue).IsValueType) + if (!typeof(TValue).IsValueType || + typeof(TValue) == typeof(IntPtr) || + typeof(TValue) == typeof(UIntPtr)) { return true; } From 45bd55d7af482b8a17e5b48f8643c9ba65e34cee Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 28 May 2020 10:20:02 -0400 Subject: [PATCH 7/7] Address PR feedback --- .../Concurrent/ConcurrentDictionary.cs | 74 ++++++++++--------- .../src/System/ThrowHelper.cs | 4 - 2 files changed, 38 insertions(+), 40 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index c9ab4d050cab3..a0dd490370801 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -348,7 +348,7 @@ public bool TryRemove(KeyValuePair item) private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value, bool matchValue, [AllowNull] TValue oldValue) { IEqualityComparer? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { Tables tables = _tables; @@ -429,7 +429,7 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) IEqualityComparer? comparer = _comparer; if (comparer is null) { - uint hashcode = (uint)key.GetHashCode(); + int hashcode = key.GetHashCode(); if (typeof(TKey).IsValueType) { for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) @@ -455,7 +455,7 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) } else { - uint hashcode = (uint)comparer.GetHashCode(key); + int hashcode = comparer.GetHashCode(key); for (Node? n = Volatile.Read(ref tables.GetBucket(hashcode)); n != null; n = n._next) { if (hashcode == n._hashcode && comparer.Equals(n._key, key)) @@ -470,9 +470,9 @@ public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) return false; } - private bool TryGetValueInternal(TKey key, uint hashcode, [MaybeNullWhen(false)] out TValue value) + private bool TryGetValueInternal(TKey key, int hashcode, [MaybeNullWhen(false)] out TValue value) { - Debug.Assert((uint)(_comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key)) == hashcode); + Debug.Assert((_comparer is null ? key.GetHashCode() : _comparer.GetHashCode(key)) == hashcode); // We must capture the volatile _tables field into a local variable: it is set to a new table on each table resize. // The Volatile.Read on the array element then ensures that we have a copy of the reference to tables._buckets[bucketNo]: @@ -562,19 +562,17 @@ public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) /// replaced with ; otherwise, false. /// /// is a null reference. - private bool TryUpdateInternal(TKey key, uint? nullableHashcode, TValue newValue, TValue comparisonValue) + private bool TryUpdateInternal(TKey key, int? nullableHashcode, TValue newValue, TValue comparisonValue) { IEqualityComparer? comparer = _comparer; Debug.Assert( nullableHashcode is null || - (comparer is null && key.GetHashCode() == nullableHashcode) || - (comparer != null && comparer.GetHashCode(key) == nullableHashcode)); + (comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)) == nullableHashcode); - uint hashcode = - nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : - comparer is null ? (uint)key.GetHashCode() : - (uint)comparer.GetHashCode(key); + int hashcode = + nullableHashcode ?? + (comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); EqualityComparer valueComparer = EqualityComparer.Default; @@ -820,7 +818,7 @@ public IEnumerator> GetEnumerator() /// If key exists, we always return false; and if updateIfExists == true we force update with value; /// If key doesn't exist, we always add value and return true; /// - private bool TryAddInternal(TKey key, uint? nullableHashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) + private bool TryAddInternal(TKey key, int? nullableHashcode, TValue value, bool updateIfExists, bool acquireLock, out TValue resultingValue) { IEqualityComparer? comparer = _comparer; @@ -829,10 +827,9 @@ nullableHashcode is null || (comparer is null && key.GetHashCode() == nullableHashcode) || (comparer != null && comparer.GetHashCode(key) == nullableHashcode)); - uint hashcode = - nullableHashcode.HasValue ? nullableHashcode.GetValueOrDefault() : - comparer is null ? (uint)key.GetHashCode() : - (uint)comparer.GetHashCode(key); + int hashcode = + nullableHashcode ?? + (comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); while (true) { @@ -957,7 +954,7 @@ public TValue this[TKey key] { if (!TryGetValue(key, out TValue value)) { - ThrowHelper.ThrowKeyNotFoundException(key); + ThrowKeyNotFoundException(key); } return value; } @@ -972,6 +969,12 @@ public TValue this[TKey key] } } + /// Throws a KeyNotFoundException. + /// Separate from ThrowHelper to avoid boxing at call site while reusing this generic instantiation. + [DoesNotReturn] + private static void ThrowKeyNotFoundException(TKey key) => + throw new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); + /// /// Gets the number of key/value pairs contained in the . @@ -1057,7 +1060,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory) } IEqualityComparer? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1096,7 +1099,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory, TA } IEqualityComparer? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1126,7 +1129,7 @@ public TValue GetOrAdd(TKey key, TValue value) } IEqualityComparer? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); if (!TryGetValueInternal(key, hashcode, out TValue resultingValue)) { @@ -1175,7 +1178,7 @@ public TValue AddOrUpdate( } IEqualityComparer? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { @@ -1236,7 +1239,7 @@ public TValue AddOrUpdate(TKey key, Func addValueFactory, Func? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { @@ -1290,7 +1293,7 @@ public TValue AddOrUpdate(TKey key, TValue addValue, Func } IEqualityComparer? comparer = _comparer; - uint hashcode = (uint)(comparer is null ? key.GetHashCode() : comparer.GetHashCode(key)); + int hashcode = comparer is null ? key.GetHashCode() : comparer.GetHashCode(key); while (true) { @@ -2082,9 +2085,9 @@ private sealed class Node internal readonly TKey _key; internal TValue _value; internal volatile Node? _next; - internal readonly uint _hashcode; + internal readonly int _hashcode; - internal Node(TKey key, TValue value, uint hashcode, Node? next) + internal Node(TKey key, TValue value, int hashcode, Node? next) { _key = key; _value = value; @@ -2122,36 +2125,35 @@ internal Tables(Node?[] buckets, object[] locks, int[] countPerLock) /// Computes a ref to the bucket for a particular key. [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal ref Node? GetBucket(uint hashcode) + internal ref Node? GetBucket(int hashcode) { Node?[] buckets = _buckets; if (IntPtr.Size == 8) { - return ref buckets[HashHelpers.FastMod(hashcode, (uint)buckets.Length, _fastModBucketsMultiplier)]; + return ref buckets[HashHelpers.FastMod((uint)hashcode, (uint)buckets.Length, _fastModBucketsMultiplier)]; } else { - return ref buckets[hashcode % (uint)buckets.Length]; + return ref buckets[(uint)hashcode % (uint)buckets.Length]; } } /// Computes the bucket and lock number for a particular key. [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal ref Node? GetBucketAndLock(uint hashcode, out uint lockNo) + internal ref Node? GetBucketAndLock(int hashcode, out uint lockNo) { Node?[] buckets = _buckets; + uint bucketNo; if (IntPtr.Size == 8) { - uint bucketNo = HashHelpers.FastMod(hashcode, (uint)buckets.Length, _fastModBucketsMultiplier); - lockNo = bucketNo % (uint)_locks.Length; - return ref buckets[bucketNo]; + bucketNo = HashHelpers.FastMod((uint)hashcode, (uint)buckets.Length, _fastModBucketsMultiplier); } else { - uint bucketNo = hashcode % (uint)_buckets.Length; - lockNo = bucketNo % (uint)_locks.Length; - return ref buckets[bucketNo]; + bucketNo = (uint)hashcode % (uint)buckets.Length; } + lockNo = bucketNo % (uint)_locks.Length; // doesn't use FastMod, as it would require maintaining a different multiplier + return ref buckets[bucketNo]; } } diff --git a/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs b/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs index 21429fb27af1b..833d204727c1b 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs @@ -2,16 +2,12 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; namespace System { internal static class ThrowHelper { - [DoesNotReturn] - internal static void ThrowKeyNotFoundException(object key) => throw new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); - [DoesNotReturn] internal static void ThrowKeyNullException() => ThrowArgumentNullException("key");