From 3bdf19435e2178b420fb71127f35da648a1f93e9 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 19 Oct 2019 05:10:06 +0100 Subject: [PATCH 01/12] Avoid mod operator when fast alternative available --- THIRD-PARTY-NOTICES.TXT | 17 +++++++++++ .../System/Collections/Generic/Dictionary.cs | 16 +++++----- .../shared/System/Collections/HashHelpers.cs | 29 +++++++++++++++++++ 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/THIRD-PARTY-NOTICES.TXT b/THIRD-PARTY-NOTICES.TXT index 9e40842ff171..f55a71c70bee 100644 --- a/THIRD-PARTY-NOTICES.TXT +++ b/THIRD-PARTY-NOTICES.TXT @@ -341,3 +341,20 @@ License notice for Xorshift (Wikipedia) https://en.wikipedia.org/wiki/Xorshift License: https://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attribution-ShareAlike_3.0_Unported_License + +License for fastrange (https://github.com/lemire/fastrange) +-------------------------------------- + + Copyright 2016 Daniel Lemire + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 5eca70b93e84..67b273748a83 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -339,7 +339,7 @@ private ref TValue FindValue(TKey key) if (comparer == null) { uint hashCode = (uint)key.GetHashCode(); - int i = buckets[hashCode % (uint)buckets.Length]; + int i = buckets[HashHelpers.FastRange(hashCode, (uint)buckets.Length)]; Entry[]? entries = _entries; uint collisionCount = 0; if (default(TKey)! != null) // TODO-NULLABLE: default(T) == null warning (https://github.com/dotnet/roslyn/issues/34757) @@ -407,7 +407,7 @@ private ref TValue FindValue(TKey key) else { uint hashCode = (uint)comparer.GetHashCode(key); - int i = buckets[hashCode % (uint)buckets.Length]; + int i = buckets[HashHelpers.FastRange(hashCode, (uint)buckets.Length)]; Entry[]? entries = _entries; uint collisionCount = 0; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. @@ -481,7 +481,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); uint collisionCount = 0; - ref int bucket = ref _buckets[hashCode % (uint)_buckets.Length]; + ref int bucket = ref _buckets[HashHelpers.FastRange(hashCode, (uint)_buckets.Length)]; // Value in _buckets is 1-based int i = bucket - 1; @@ -625,7 +625,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) if (count == entries.Length) { Resize(); - bucket = ref _buckets[hashCode % (uint)_buckets.Length]; + bucket = ref _buckets[HashHelpers.FastRange(hashCode, (uint)_buckets.Length)]; } index = count; _count = count + 1; @@ -738,7 +738,7 @@ private void Resize(int newSize, bool forceNewHashCodes) { if (entries[i].next >= -1) { - uint bucket = entries[i].hashCode % (uint)newSize; + uint bucket = HashHelpers.FastRange(entries[i].hashCode, (uint)newSize); // Value in _buckets is 1-based entries[i].next = buckets[bucket] - 1; // Value in _buckets is 1-based @@ -767,7 +767,7 @@ public bool Remove(TKey key) Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = hashCode % (uint)buckets.Length; + uint bucket = HashHelpers.FastRange(hashCode, (uint)buckets.Length); int last = -1; // Value in buckets is 1-based int i = buckets[bucket] - 1; @@ -836,7 +836,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = hashCode % (uint)buckets.Length; + uint bucket = HashHelpers.FastRange(hashCode, (uint)buckets.Length); int last = -1; // Value in buckets is 1-based int i = buckets[bucket] - 1; @@ -1031,7 +1031,7 @@ public void TrimExcess(int capacity) { ref Entry entry = ref entries![count]; entry = oldEntries[i]; - uint bucket = hashCode % (uint)newSize; + uint bucket = HashHelpers.FastRange(hashCode, (uint)newSize); // Value in _buckets is 1-based entry.next = buckets![bucket] - 1; // If we get here, we have entries, therefore buckets is not null. // Value in _buckets is 1-based diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index 3b53609cd4b5..69211e588f02 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics.X86; namespace System.Collections { @@ -86,5 +88,32 @@ public static int ExpandPrime(int oldSize) return GetPrime(newSize); } + + // Takes an index and distributes it in the range [0, range) + // e.g. 0 to range exclusive. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static uint FastRange(uint index, uint range) + { + // Sse42.IsSupported check is baked into the R2R image so while the value returned + // from this method will be different on both branches, it will remain stable during + // a process' lifetime even if it is recompiled by tiered compilation. + if (Sse42.IsSupported) + { + // Using fastrange from Daniel Lemire https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/ + // + // While fastrange is "fair"; it expects an evenly distributed hashCode + // from [0,2^32) to produce a distributed range. As many hashCodes in .NET + // are generated with the high entropy in the LSB we need to redistribute + // the hash; for this we use Crc32. These two together allow us to skip the + // more costly idiv instruction. + + return (uint)((Sse42.Crc32(0, index) * (ulong)range) >> 32); + } + else + { + // Fast Crc isn't supported so we will use the slower modulo. + return index % range; + } + } } } From 79e2529969017d4238a373c0df52123216776fdc Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 24 Oct 2019 00:36:01 +0100 Subject: [PATCH 02/12] Use fastmod --- THIRD-PARTY-NOTICES.TXT | 4 +-- .../System/Collections/Generic/Dictionary.cs | 26 ++++++++++------ .../shared/System/Collections/HashHelpers.cs | 30 ++++++++----------- 3 files changed, 32 insertions(+), 28 deletions(-) diff --git a/THIRD-PARTY-NOTICES.TXT b/THIRD-PARTY-NOTICES.TXT index f55a71c70bee..01511175f6f9 100644 --- a/THIRD-PARTY-NOTICES.TXT +++ b/THIRD-PARTY-NOTICES.TXT @@ -342,10 +342,10 @@ License notice for Xorshift (Wikipedia) https://en.wikipedia.org/wiki/Xorshift License: https://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attribution-ShareAlike_3.0_Unported_License -License for fastrange (https://github.com/lemire/fastrange) +License for fastmod (https://github.com/lemire/fastmod) -------------------------------------- - Copyright 2016 Daniel Lemire + Copyright 2018 Daniel Lemire Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 67b273748a83..ba6579882045 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -49,6 +49,7 @@ private struct Entry public TValue value; // Value of entry } + private ulong _fastModMultiplier; private int[]? _buckets; private Entry[]? _entries; private int _count; @@ -339,7 +340,7 @@ private ref TValue FindValue(TKey key) if (comparer == null) { uint hashCode = (uint)key.GetHashCode(); - int i = buckets[HashHelpers.FastRange(hashCode, (uint)buckets.Length)]; + int i = buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; Entry[]? entries = _entries; uint collisionCount = 0; if (default(TKey)! != null) // TODO-NULLABLE: default(T) == null warning (https://github.com/dotnet/roslyn/issues/34757) @@ -407,7 +408,7 @@ private ref TValue FindValue(TKey key) else { uint hashCode = (uint)comparer.GetHashCode(key); - int i = buckets[HashHelpers.FastRange(hashCode, (uint)buckets.Length)]; + int i = buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; Entry[]? entries = _entries; uint collisionCount = 0; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. @@ -453,6 +454,7 @@ private ref TValue FindValue(TKey key) private int Initialize(int capacity) { int size = HashHelpers.GetPrime(capacity); + _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); _freeList = -1; _buckets = new int[size]; @@ -481,7 +483,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); uint collisionCount = 0; - ref int bucket = ref _buckets[HashHelpers.FastRange(hashCode, (uint)_buckets.Length)]; + ref int bucket = ref _buckets[HashHelpers.FastMod(hashCode, (uint)_buckets.Length, _fastModMultiplier)]; // Value in _buckets is 1-based int i = bucket - 1; @@ -625,7 +627,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) if (count == entries.Length) { Resize(); - bucket = ref _buckets[HashHelpers.FastRange(hashCode, (uint)_buckets.Length)]; + bucket = ref _buckets[HashHelpers.FastMod(hashCode, (uint)_buckets.Length, _fastModMultiplier)]; } index = count; _count = count + 1; @@ -707,7 +709,11 @@ public virtual void OnDeserialization(object? sender) } private void Resize() - => Resize(HashHelpers.ExpandPrime(_count), false); + { + int size = HashHelpers.ExpandPrime(_count); + _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); + Resize(size, false); + } private void Resize(int newSize, bool forceNewHashCodes) { @@ -738,7 +744,7 @@ private void Resize(int newSize, bool forceNewHashCodes) { if (entries[i].next >= -1) { - uint bucket = HashHelpers.FastRange(entries[i].hashCode, (uint)newSize); + uint bucket = HashHelpers.FastMod(entries[i].hashCode, (uint)newSize, _fastModMultiplier); // Value in _buckets is 1-based entries[i].next = buckets[bucket] - 1; // Value in _buckets is 1-based @@ -767,7 +773,7 @@ public bool Remove(TKey key) Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = HashHelpers.FastRange(hashCode, (uint)buckets.Length); + uint bucket = HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier); int last = -1; // Value in buckets is 1-based int i = buckets[bucket] - 1; @@ -836,7 +842,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = HashHelpers.FastRange(hashCode, (uint)buckets.Length); + uint bucket = HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier); int last = -1; // Value in buckets is 1-based int i = buckets[bucket] - 1; @@ -983,6 +989,7 @@ public int EnsureCapacity(int capacity) if (_buckets == null) return Initialize(capacity); int newSize = HashHelpers.GetPrime(capacity); + _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); Resize(newSize, forceNewHashCodes: false); return newSize; } @@ -1012,6 +1019,7 @@ public void TrimExcess(int capacity) if (capacity < Count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); int newSize = HashHelpers.GetPrime(capacity); + _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); Entry[]? oldEntries = _entries; int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; @@ -1031,7 +1039,7 @@ public void TrimExcess(int capacity) { ref Entry entry = ref entries![count]; entry = oldEntries[i]; - uint bucket = HashHelpers.FastRange(hashCode, (uint)newSize); + uint bucket = HashHelpers.FastMod(hashCode, (uint)newSize, _fastModMultiplier); // Value in _buckets is 1-based entry.next = buckets![bucket] - 1; // If we get here, we have entries, therefore buckets is not null. // Value in _buckets is 1-based diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index 69211e588f02..39b19b015cfb 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -89,30 +89,26 @@ public static int ExpandPrime(int oldSize) return GetPrime(newSize); } - // Takes an index and distributes it in the range [0, range) - // e.g. 0 to range exclusive. + public static ulong GetFastModMultiplier(uint divisor) + => ulong.MaxValue / divisor + 1; + [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static uint FastRange(uint index, uint range) + public static unsafe uint FastMod(uint value, uint divisor, ulong multiplier) { - // Sse42.IsSupported check is baked into the R2R image so while the value returned - // from this method will be different on both branches, it will remain stable during - // a process' lifetime even if it is recompiled by tiered compilation. - if (Sse42.IsSupported) + // 64bit * 64bit => 128bit isn't currently supported by Math https://github.com/dotnet/corefx/issues/41822 + if (Bmi2.X64.IsSupported) { - // Using fastrange from Daniel Lemire https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/ - // - // While fastrange is "fair"; it expects an evenly distributed hashCode - // from [0,2^32) to produce a distributed range. As many hashCodes in .NET - // are generated with the high entropy in the LSB we need to redistribute - // the hash; for this we use Crc32. These two together allow us to skip the - // more costly idiv instruction. + // Using fastmod from Daniel Lemire https://lemire.me/blog/2019/02/08/faster-remainders-when-the-divisor-is-a-constant-beating-compilers-and-libdivide/ - return (uint)((Sse42.Crc32(0, index) * (ulong)range) >> 32); + ulong lowbits = multiplier * value; + ulong low; + ulong high = Bmi2.X64.MultiplyNoFlags(lowbits, divisor, &low); + return (uint)high; } else { - // Fast Crc isn't supported so we will use the slower modulo. - return index % range; + // 64bit * 64bit => 128bit isn't supported so we will use the slower modulo. + return value % divisor; } } } From bc1818d7dcfff78997517419b85481d039c80a0c Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 24 Oct 2019 01:23:17 +0100 Subject: [PATCH 03/12] x64 only --- .../System/Collections/Generic/Dictionary.cs | 36 ++++++++++++++----- .../shared/System/Collections/HashHelpers.cs | 2 ++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index ba6579882045..bf1701d025ac 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -49,9 +49,11 @@ private struct Entry public TValue value; // Value of entry } - private ulong _fastModMultiplier; private int[]? _buckets; private Entry[]? _entries; +#if BIT64 + private ulong _fastModMultiplier; +#endif private int _count; private int _freeList; private int _freeCount; @@ -340,7 +342,7 @@ private ref TValue FindValue(TKey key) if (comparer == null) { uint hashCode = (uint)key.GetHashCode(); - int i = buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; + int i = buckets[FastMod(hashCode, (uint)buckets.Length)]; Entry[]? entries = _entries; uint collisionCount = 0; if (default(TKey)! != null) // TODO-NULLABLE: default(T) == null warning (https://github.com/dotnet/roslyn/issues/34757) @@ -408,7 +410,7 @@ private ref TValue FindValue(TKey key) else { uint hashCode = (uint)comparer.GetHashCode(key); - int i = buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; + int i = buckets[FastMod(hashCode, (uint)buckets.Length)]; Entry[]? entries = _entries; uint collisionCount = 0; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. @@ -454,7 +456,9 @@ private ref TValue FindValue(TKey key) private int Initialize(int capacity) { int size = HashHelpers.GetPrime(capacity); +#if BIT64 _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); +#endif _freeList = -1; _buckets = new int[size]; @@ -483,7 +487,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); uint collisionCount = 0; - ref int bucket = ref _buckets[HashHelpers.FastMod(hashCode, (uint)_buckets.Length, _fastModMultiplier)]; + ref int bucket = ref _buckets[FastMod(hashCode, (uint)_buckets.Length)]; // Value in _buckets is 1-based int i = bucket - 1; @@ -627,7 +631,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) if (count == entries.Length) { Resize(); - bucket = ref _buckets[HashHelpers.FastMod(hashCode, (uint)_buckets.Length, _fastModMultiplier)]; + bucket = ref _buckets[FastMod(hashCode, (uint)_buckets.Length)]; } index = count; _count = count + 1; @@ -711,7 +715,9 @@ public virtual void OnDeserialization(object? sender) private void Resize() { int size = HashHelpers.ExpandPrime(_count); +#if BIT64 _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); +#endif Resize(size, false); } @@ -744,7 +750,7 @@ private void Resize(int newSize, bool forceNewHashCodes) { if (entries[i].next >= -1) { - uint bucket = HashHelpers.FastMod(entries[i].hashCode, (uint)newSize, _fastModMultiplier); + uint bucket = FastMod(entries[i].hashCode, (uint)newSize); // Value in _buckets is 1-based entries[i].next = buckets[bucket] - 1; // Value in _buckets is 1-based @@ -773,7 +779,7 @@ public bool Remove(TKey key) Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier); + uint bucket = FastMod(hashCode, (uint)buckets.Length); int last = -1; // Value in buckets is 1-based int i = buckets[bucket] - 1; @@ -842,7 +848,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier); + uint bucket = FastMod(hashCode, (uint)buckets.Length); int last = -1; // Value in buckets is 1-based int i = buckets[bucket] - 1; @@ -989,7 +995,9 @@ public int EnsureCapacity(int capacity) if (_buckets == null) return Initialize(capacity); int newSize = HashHelpers.GetPrime(capacity); +#if BIT64 _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); +#endif Resize(newSize, forceNewHashCodes: false); return newSize; } @@ -1019,7 +1027,9 @@ public void TrimExcess(int capacity) if (capacity < Count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); int newSize = HashHelpers.GetPrime(capacity); +#if BIT64 _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); +#endif Entry[]? oldEntries = _entries; int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; @@ -1039,7 +1049,7 @@ public void TrimExcess(int capacity) { ref Entry entry = ref entries![count]; entry = oldEntries[i]; - uint bucket = HashHelpers.FastMod(hashCode, (uint)newSize, _fastModMultiplier); + uint bucket = FastMod(hashCode, (uint)newSize); // Value in _buckets is 1-based entry.next = buckets![bucket] - 1; // If we get here, we have entries, therefore buckets is not null. // Value in _buckets is 1-based @@ -1161,6 +1171,14 @@ void IDictionary.Remove(object key) } } +#if BIT64 + private uint FastMod(uint value, uint divisor) + => HashHelpers.FastMod(value, divisor, _fastModMultiplier); +#else + private static uint FastMod(uint value, uint divisor) + => value % divisor; +#endif + public struct Enumerator : IEnumerator>, IDictionaryEnumerator { diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index 39b19b015cfb..72f4d9f6b033 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -89,6 +89,7 @@ public static int ExpandPrime(int oldSize) return GetPrime(newSize); } +#if BIT64 public static ulong GetFastModMultiplier(uint divisor) => ulong.MaxValue / divisor + 1; @@ -111,5 +112,6 @@ public static unsafe uint FastMod(uint value, uint divisor, ulong multiplier) return value % divisor; } } +#endif } } From 3c198997e7ae825d1b259ffd329768fef2a89b19 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 25 Oct 2019 04:28:41 +0100 Subject: [PATCH 04/12] Feedback --- .../System/Collections/Generic/Dictionary.cs | 61 ++++++++++--------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index bf1701d025ac..8b930fed639e 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -333,16 +333,15 @@ private ref TValue FindValue(TKey key) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - int[]? buckets = _buckets; ref Entry entry = ref Unsafe.NullRef(); - if (buckets != null) + if (_buckets != null) { Debug.Assert(_entries != null, "expected entries to be != null"); IEqualityComparer? comparer = _comparer; if (comparer == null) { uint hashCode = (uint)key.GetHashCode(); - int i = buckets[FastMod(hashCode, (uint)buckets.Length)]; + int i = GetBucket(hashCode); Entry[]? entries = _entries; uint collisionCount = 0; if (default(TKey)! != null) // TODO-NULLABLE: default(T) == null warning (https://github.com/dotnet/roslyn/issues/34757) @@ -410,7 +409,7 @@ private ref TValue FindValue(TKey key) else { uint hashCode = (uint)comparer.GetHashCode(key); - int i = buckets[FastMod(hashCode, (uint)buckets.Length)]; + int i = GetBucket(hashCode); Entry[]? entries = _entries; uint collisionCount = 0; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional. @@ -487,7 +486,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) uint hashCode = (uint)((comparer == null) ? key.GetHashCode() : comparer.GetHashCode(key)); uint collisionCount = 0; - ref int bucket = ref _buckets[FastMod(hashCode, (uint)_buckets.Length)]; + ref int bucket = ref GetBucket(hashCode); // Value in _buckets is 1-based int i = bucket - 1; @@ -631,7 +630,7 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) if (count == entries.Length) { Resize(); - bucket = ref _buckets[FastMod(hashCode, (uint)_buckets.Length)]; + bucket = ref GetBucket(hashCode); } index = count; _count = count + 1; @@ -728,7 +727,7 @@ private void Resize(int newSize, bool forceNewHashCodes) Debug.Assert(_entries != null, "_entries should be non-null"); Debug.Assert(newSize >= _entries.Length); - int[] buckets = new int[newSize]; + _buckets = new int[newSize]; Entry[] entries = new Entry[newSize]; int count = _count; @@ -750,15 +749,14 @@ private void Resize(int newSize, bool forceNewHashCodes) { if (entries[i].next >= -1) { - uint bucket = FastMod(entries[i].hashCode, (uint)newSize); + ref int bucket = ref GetBucket(entries[i].hashCode); // Value in _buckets is 1-based - entries[i].next = buckets[bucket] - 1; + entries[i].next = bucket - 1; // Value in _buckets is 1-based - buckets[bucket] = i + 1; + bucket = i + 1; } } - _buckets = buckets; _entries = entries; } @@ -772,17 +770,16 @@ public bool Remove(TKey key) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - int[]? buckets = _buckets; Entry[]? entries = _entries; - if (buckets != null) + if (_buckets != null) { Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = FastMod(hashCode, (uint)buckets.Length); + ref int bucket = ref GetBucket(hashCode); int last = -1; // Value in buckets is 1-based - int i = buckets[bucket] - 1; + int i = bucket - 1; while (i >= 0) { ref Entry entry = ref entries[i]; @@ -792,7 +789,7 @@ public bool Remove(TKey key) if (last < 0) { // Value in buckets is 1-based - buckets[bucket] = entry.next + 1; + bucket = entry.next + 1; } else { @@ -841,17 +838,16 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - int[]? buckets = _buckets; Entry[]? entries = _entries; - if (buckets != null) + if (_buckets != null) { Debug.Assert(entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); - uint bucket = FastMod(hashCode, (uint)buckets.Length); + ref int bucket = ref GetBucket(hashCode); int last = -1; // Value in buckets is 1-based - int i = buckets[bucket] - 1; + int i = bucket - 1; while (i >= 0) { ref Entry entry = ref entries[i]; @@ -861,7 +857,7 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) if (last < 0) { // Value in buckets is 1-based - buckets[bucket] = entry.next + 1; + bucket = entry.next + 1; } else { @@ -1040,7 +1036,6 @@ public void TrimExcess(int capacity) _version++; Initialize(newSize); Entry[]? entries = _entries; - int[]? buckets = _buckets; int count = 0; for (int i = 0; i < oldCount; i++) { @@ -1049,11 +1044,11 @@ public void TrimExcess(int capacity) { ref Entry entry = ref entries![count]; entry = oldEntries[i]; - uint bucket = FastMod(hashCode, (uint)newSize); + ref int bucket = ref GetBucket(hashCode); // Value in _buckets is 1-based - entry.next = buckets![bucket] - 1; // If we get here, we have entries, therefore buckets is not null. + entry.next = bucket - 1; // If we get here, we have entries, therefore buckets is not null. // Value in _buckets is 1-based - buckets[bucket] = count + 1; + bucket = count + 1; count++; } } @@ -1172,11 +1167,19 @@ void IDictionary.Remove(object key) } #if BIT64 - private uint FastMod(uint value, uint divisor) - => HashHelpers.FastMod(value, divisor, _fastModMultiplier); + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private ref int GetBucket(uint hashCode) + { + int[] buckets = _buckets!; + return ref buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; + } #else - private static uint FastMod(uint value, uint divisor) - => value % divisor; + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private ref int GetBucket(uint hashCode) + { + int[] buckets = _buckets!; + return ref buckets![hashCode % (uint)buckets.Length]; + } #endif public struct Enumerator : IEnumerator>, From e40200aced6a0f49945a5211108f495a205cdf57 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 25 Oct 2019 04:58:06 +0100 Subject: [PATCH 05/12] Feedback 2 --- .../shared/System/Collections/Generic/Dictionary.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 8b930fed639e..80d44f8b0fd5 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -1166,21 +1166,16 @@ void IDictionary.Remove(object key) } } -#if BIT64 [MethodImpl(MethodImplOptions.AggressiveInlining)] private ref int GetBucket(uint hashCode) { int[] buckets = _buckets!; +#if BIT64 return ref buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; - } #else - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private ref int GetBucket(uint hashCode) - { - int[] buckets = _buckets!; return ref buckets![hashCode % (uint)buckets.Length]; - } #endif + } public struct Enumerator : IEnumerator>, IDictionaryEnumerator From a55391d96a8a8f4f4d44eb8092e05e918a74a74d Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 25 Oct 2019 05:47:09 +0100 Subject: [PATCH 06/12] Feedback 3 --- .../System/Collections/Generic/Dictionary.cs | 34 ++++++++++--------- .../shared/System/Collections/HashHelpers.cs | 34 ++++++++++++------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 80d44f8b0fd5..59ebb420bd9b 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -454,11 +454,11 @@ private ref TValue FindValue(TKey key) private int Initialize(int capacity) { - int size = HashHelpers.GetPrime(capacity); #if BIT64 - _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); + int size = HashHelpers.GetPrime(capacity, out _fastModMultiplier); +#else + int size = HashHelpers.GetPrime(capacity); #endif - _freeList = -1; _buckets = new int[size]; _entries = new Entry[size]; @@ -713,9 +713,10 @@ public virtual void OnDeserialization(object? sender) private void Resize() { - int size = HashHelpers.ExpandPrime(_count); #if BIT64 - _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); + int size = HashHelpers.ExpandPrime(_count, out _fastModMultiplier); +#else + int size = HashHelpers.ExpandPrime(_count); #endif Resize(size, false); } @@ -770,13 +771,13 @@ public bool Remove(TKey key) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - Entry[]? entries = _entries; if (_buckets != null) { - Debug.Assert(entries != null, "entries should be non-null"); + Debug.Assert(_entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); ref int bucket = ref GetBucket(hashCode); + Entry[]? entries = _entries; int last = -1; // Value in buckets is 1-based int i = bucket - 1; @@ -838,13 +839,13 @@ public bool Remove(TKey key, [MaybeNullWhen(false)] out TValue value) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); } - Entry[]? entries = _entries; if (_buckets != null) { - Debug.Assert(entries != null, "entries should be non-null"); + Debug.Assert(_entries != null, "entries should be non-null"); uint collisionCount = 0; uint hashCode = (uint)(_comparer?.GetHashCode(key) ?? key.GetHashCode()); ref int bucket = ref GetBucket(hashCode); + Entry[]? entries = _entries; int last = -1; // Value in buckets is 1-based int i = bucket - 1; @@ -990,9 +991,10 @@ public int EnsureCapacity(int capacity) _version++; if (_buckets == null) return Initialize(capacity); - int newSize = HashHelpers.GetPrime(capacity); #if BIT64 - _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); + int newSize = HashHelpers.GetPrime(capacity, out _fastModMultiplier); +#else + int newSize = HashHelpers.GetPrime(capacity); #endif Resize(newSize, forceNewHashCodes: false); return newSize; @@ -1022,11 +1024,11 @@ public void TrimExcess(int capacity) { if (capacity < Count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); - int newSize = HashHelpers.GetPrime(capacity); #if BIT64 - _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); + int newSize = HashHelpers.GetPrime(capacity, out _fastModMultiplier); +#else + int newSize = HashHelpers.GetPrime(capacity); #endif - Entry[]? oldEntries = _entries; int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; if (newSize >= currentCapacity) @@ -1046,7 +1048,7 @@ public void TrimExcess(int capacity) entry = oldEntries[i]; ref int bucket = ref GetBucket(hashCode); // Value in _buckets is 1-based - entry.next = bucket - 1; // If we get here, we have entries, therefore buckets is not null. + entry.next = bucket - 1; // Value in _buckets is 1-based bucket = count + 1; count++; @@ -1173,7 +1175,7 @@ private ref int GetBucket(uint hashCode) #if BIT64 return ref buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; #else - return ref buckets![hashCode % (uint)buckets.Length]; + return ref buckets[hashCode % (uint)buckets.Length]; #endif } diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index 72f4d9f6b033..d336d8a1cf3d 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -90,27 +90,35 @@ public static int ExpandPrime(int oldSize) } #if BIT64 + public static int GetPrime(int capacity, out ulong fastModMultiplier) + { + int prime = GetPrime(capacity); + fastModMultiplier = GetFastModMultiplier((uint)prime); + return prime; + } + + public static int ExpandPrime(int currentPrime, out ulong fastModMultiplier) + { + int prime = ExpandPrime(currentPrime); + fastModMultiplier = GetFastModMultiplier((uint)prime); + return prime; + } + public static ulong GetFastModMultiplier(uint divisor) => ulong.MaxValue / divisor + 1; [MethodImpl(MethodImplOptions.AggressiveInlining)] public static unsafe uint FastMod(uint value, uint divisor, ulong multiplier) { + // Using fastmod from Daniel Lemire https://lemire.me/blog/2019/02/08/faster-remainders-when-the-divisor-is-a-constant-beating-compilers-and-libdivide/ + + ulong lowbits = multiplier * value; // 64bit * 64bit => 128bit isn't currently supported by Math https://github.com/dotnet/corefx/issues/41822 - if (Bmi2.X64.IsSupported) - { - // Using fastmod from Daniel Lemire https://lemire.me/blog/2019/02/08/faster-remainders-when-the-divisor-is-a-constant-beating-compilers-and-libdivide/ + // otherwise we'd want this to be (uint)Math.MultiplyHigh(lowbits, divisor) + uint high = (uint)((((ulong)(uint)lowbits * divisor >> 32) + (lowbits >> 32) * divisor) >> 32); - ulong lowbits = multiplier * value; - ulong low; - ulong high = Bmi2.X64.MultiplyNoFlags(lowbits, divisor, &low); - return (uint)high; - } - else - { - // 64bit * 64bit => 128bit isn't supported so we will use the slower modulo. - return value % divisor; - } + Debug.Assert(high == value % divisor); + return high; } #endif } From 8698984a2de1263eadc8d456ad818c1913a12874 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Fri, 25 Oct 2019 06:58:05 +0100 Subject: [PATCH 07/12] Remove unsafe --- .../shared/System/Collections/HashHelpers.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index d336d8a1cf3d..340e86004372 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -108,7 +108,7 @@ public static ulong GetFastModMultiplier(uint divisor) => ulong.MaxValue / divisor + 1; [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static unsafe uint FastMod(uint value, uint divisor, ulong multiplier) + public static uint FastMod(uint value, uint divisor, ulong multiplier) { // Using fastmod from Daniel Lemire https://lemire.me/blog/2019/02/08/faster-remainders-when-the-divisor-is-a-constant-beating-compilers-and-libdivide/ From 9c9c7f67c81a0d489dcfb9481d841ebae560ccc8 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 26 Oct 2019 05:14:03 +0100 Subject: [PATCH 08/12] nits --- .../shared/System/Collections/HashHelpers.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index 340e86004372..2d772cc98dae 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -4,7 +4,6 @@ using System.Diagnostics; using System.Runtime.CompilerServices; -using System.Runtime.Intrinsics.X86; namespace System.Collections { @@ -30,12 +29,14 @@ internal static partial class HashHelpers // h1(key) + i*h2(key), 0 <= i < size. h2 and the size must be relatively prime. // We prefer the low computation costs of higher prime numbers over the increased // memory allocation of a fixed prime number i.e. when right sizing a HashSet. - public static readonly int[] primes = { + private static readonly int[] s_primes = + { 3, 7, 11, 17, 23, 29, 37, 47, 59, 71, 89, 107, 131, 163, 197, 239, 293, 353, 431, 521, 631, 761, 919, 1103, 1327, 1597, 1931, 2333, 2801, 3371, 4049, 4861, 5839, 7013, 8419, 10103, 12143, 14591, 17519, 21023, 25229, 30293, 36353, 43627, 52361, 62851, 75431, 90523, 108631, 130363, 156437, 187751, 225307, 270371, 324449, 389357, 467237, 560689, 672827, 807403, 968897, 1162687, 1395263, - 1674319, 2009191, 2411033, 2893249, 3471899, 4166287, 4999559, 5999471, 7199369 }; + 1674319, 2009191, 2411033, 2893249, 3471899, 4166287, 4999559, 5999471, 7199369 + }; public static bool IsPrime(int candidate) { @@ -57,9 +58,9 @@ public static int GetPrime(int min) if (min < 0) throw new ArgumentException(SR.Arg_HTCapacityOverflow); - for (int i = 0; i < primes.Length; i++) + for (int i = 0; i < s_primes.Length; i++) { - int prime = primes[i]; + int prime = s_primes[i]; if (prime >= min) return prime; } From 7a008ef10e767accb5b4135e8da4b7bcac97edac Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 26 Oct 2019 05:43:11 +0100 Subject: [PATCH 09/12] Guard against corruption from OOM --- .../shared/System/Collections/Generic/Dictionary.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 59ebb420bd9b..4e231c417093 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -459,9 +459,12 @@ private int Initialize(int capacity) #else int size = HashHelpers.GetPrime(capacity); #endif + int[] buckets = new int[size]; + Entry[] entries = new Entry[size]; + // Assign after both allocated to guard against corruption from OOM if second fails _freeList = -1; - _buckets = new int[size]; - _entries = new Entry[size]; + _buckets = buckets; + _entries = entries; return size; } @@ -728,7 +731,6 @@ private void Resize(int newSize, bool forceNewHashCodes) Debug.Assert(_entries != null, "_entries should be non-null"); Debug.Assert(newSize >= _entries.Length); - _buckets = new int[newSize]; Entry[] entries = new Entry[newSize]; int count = _count; @@ -746,6 +748,7 @@ private void Resize(int newSize, bool forceNewHashCodes) } } + _buckets = new int[newSize]; for (int i = 0; i < count; i++) { if (entries[i].next >= -1) From 9db1979e25780867217a8bbe4befc492ec92b79b Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 26 Oct 2019 06:13:35 +0100 Subject: [PATCH 10/12] Feedback --- .../System/Collections/Generic/Dictionary.cs | 28 ++++++++----------- .../shared/System/Collections/HashHelpers.cs | 17 +---------- 2 files changed, 12 insertions(+), 33 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 4e231c417093..5219147c5d5a 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -454,15 +454,15 @@ private ref TValue FindValue(TKey key) private int Initialize(int capacity) { -#if BIT64 - int size = HashHelpers.GetPrime(capacity, out _fastModMultiplier); -#else int size = HashHelpers.GetPrime(capacity); -#endif int[] buckets = new int[size]; Entry[] entries = new Entry[size]; - // Assign after both allocated to guard against corruption from OOM if second fails + + // Assign member varables after both arrays allocated to guard against corruption from OOM if second fails _freeList = -1; +#if BIT64 + _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); +#endif _buckets = buckets; _entries = entries; @@ -716,11 +716,7 @@ public virtual void OnDeserialization(object? sender) private void Resize() { -#if BIT64 - int size = HashHelpers.ExpandPrime(_count, out _fastModMultiplier); -#else int size = HashHelpers.ExpandPrime(_count); -#endif Resize(size, false); } @@ -748,7 +744,11 @@ private void Resize(int newSize, bool forceNewHashCodes) } } + // Assign member varables after both arrays allocated to guard against corruption from OOM if second fails _buckets = new int[newSize]; +#if BIT64 + _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize); +#endif for (int i = 0; i < count; i++) { if (entries[i].next >= -1) @@ -994,11 +994,8 @@ public int EnsureCapacity(int capacity) _version++; if (_buckets == null) return Initialize(capacity); -#if BIT64 - int newSize = HashHelpers.GetPrime(capacity, out _fastModMultiplier); -#else + int newSize = HashHelpers.GetPrime(capacity); -#endif Resize(newSize, forceNewHashCodes: false); return newSize; } @@ -1027,11 +1024,8 @@ public void TrimExcess(int capacity) { if (capacity < Count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); -#if BIT64 - int newSize = HashHelpers.GetPrime(capacity, out _fastModMultiplier); -#else + int newSize = HashHelpers.GetPrime(capacity); -#endif Entry[]? oldEntries = _entries; int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; if (newSize >= currentCapacity) diff --git a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs index 2d772cc98dae..b01b01d8746c 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/HashHelpers.cs @@ -58,9 +58,8 @@ public static int GetPrime(int min) if (min < 0) throw new ArgumentException(SR.Arg_HTCapacityOverflow); - for (int i = 0; i < s_primes.Length; i++) + foreach (int prime in s_primes) { - int prime = s_primes[i]; if (prime >= min) return prime; } @@ -91,20 +90,6 @@ public static int ExpandPrime(int oldSize) } #if BIT64 - public static int GetPrime(int capacity, out ulong fastModMultiplier) - { - int prime = GetPrime(capacity); - fastModMultiplier = GetFastModMultiplier((uint)prime); - return prime; - } - - public static int ExpandPrime(int currentPrime, out ulong fastModMultiplier) - { - int prime = ExpandPrime(currentPrime); - fastModMultiplier = GetFastModMultiplier((uint)prime); - return prime; - } - public static ulong GetFastModMultiplier(uint divisor) => ulong.MaxValue / divisor + 1; From c6c90010e641c9476c067853647f0c15cb346440 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 26 Oct 2019 06:17:10 +0100 Subject: [PATCH 11/12] nit --- .../shared/System/Collections/Generic/Dictionary.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 5219147c5d5a..0928ce9f45c8 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -715,10 +715,7 @@ public virtual void OnDeserialization(object? sender) } private void Resize() - { - int size = HashHelpers.ExpandPrime(_count); - Resize(size, false); - } + => Resize(HashHelpers.ExpandPrime(_count), false); private void Resize(int newSize, bool forceNewHashCodes) { From 06aacfd5575a721876dc16de4cac1c3a40206a6c Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sat, 26 Oct 2019 06:25:29 +0100 Subject: [PATCH 12/12] Apply suggestions from code review Co-Authored-By: Jan Kotas --- .../shared/System/Collections/Generic/Dictionary.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs index 0928ce9f45c8..e7726f96b2bf 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs @@ -458,7 +458,7 @@ private int Initialize(int capacity) int[] buckets = new int[size]; Entry[] entries = new Entry[size]; - // Assign member varables after both arrays allocated to guard against corruption from OOM if second fails + // Assign member variables after both arrays allocated to guard against corruption from OOM if second fails _freeList = -1; #if BIT64 _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)size); @@ -741,7 +741,7 @@ private void Resize(int newSize, bool forceNewHashCodes) } } - // Assign member varables after both arrays allocated to guard against corruption from OOM if second fails + // Assign member variables after both arrays allocated to guard against corruption from OOM if second fails _buckets = new int[newSize]; #if BIT64 _fastModMultiplier = HashHelpers.GetFastModMultiplier((uint)newSize);