From db6d1f0d35a1922440c2dab5341afce27af11bda Mon Sep 17 00:00:00 2001
From: Adam Sitnik <adam.sitnik@gmail.com>
Date: Thu, 22 Jun 2023 19:59:05 +0200
Subject: [PATCH 1/2] address code review feedback from previous PR: move the
 duplicated logic into a dedicated local method

---
 .../Collections/Frozen/FrozenHashTable.cs     | 57 ++++++++++---------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
index 363ab51d035f76..05ab18a5f4b6bb 100644
--- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
+++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
@@ -217,37 +217,27 @@ private static int CalcNumBuckets(ReadOnlySpan<int> hashCodes, bool optimizeForR
 
             int bestNumBuckets = maxNumBuckets;
             int bestNumCollisions = uniqueCodesCount;
+            int numBuckets = 0, numCollisions = 0;
 
             // Iterate through each available prime between the min and max discovered. For each, compute
             // the collision ratio.
             for (int primeIndex = minPrimeIndexInclusive; primeIndex < maxPrimeIndexExclusive; primeIndex++)
             {
                 // Get the number of buckets to try, and clear our seen bucket bitmap.
-                int numBuckets = primes[primeIndex];
+                numBuckets = primes[primeIndex];
                 Array.Clear(seenBuckets, 0, Math.Min(numBuckets, seenBuckets.Length));
 
                 // Determine the bucket for each hash code and mark it as seen. If it was already seen,
                 // track it as a collision.
-                int numCollisions = 0;
+                numCollisions = 0;
 
                 if (codes is not null && uniqueCodesCount != hashCodes.Length)
                 {
                     foreach (int code in codes)
                     {
-                        uint bucketNum = (uint)code % (uint)numBuckets;
-                        if ((seenBuckets[bucketNum / BitsPerInt32] & (1 << (int)bucketNum)) != 0)
+                        if (!IsBucketFirstVisit(code))
                         {
-                            numCollisions++;
-                            if (numCollisions >= bestNumCollisions)
-                            {
-                                // If we've already hit the previously known best number of collisions,
-                                // there's no point in continuing as worst case we'd just use that.
-                                break;
-                            }
-                        }
-                        else
-                        {
-                            seenBuckets[bucketNum / BitsPerInt32] |= 1 << (int)bucketNum;
+                            break;
                         }
                     }
                 }
@@ -256,20 +246,9 @@ private static int CalcNumBuckets(ReadOnlySpan<int> hashCodes, bool optimizeForR
                     // All of the hash codes in hashCodes are unique. In such scenario, it's faster to iterate over a span.
                     foreach (int code in hashCodes)
                     {
-                        uint bucketNum = (uint)code % (uint)numBuckets;
-                        if ((seenBuckets[bucketNum / BitsPerInt32] & (1 << (int)bucketNum)) != 0)
-                        {
-                            numCollisions++;
-                            if (numCollisions >= bestNumCollisions)
-                            {
-                                // If we've already hit the previously known best number of collisions,
-                                // there's no point in continuing as worst case we'd just use that.
-                                break;
-                            }
-                        }
-                        else
+                        if (!IsBucketFirstVisit(code))
                         {
-                            seenBuckets[bucketNum / BitsPerInt32] |= 1 << (int)bucketNum;
+                            break;
                         }
                     }
                 }
@@ -292,6 +271,28 @@ private static int CalcNumBuckets(ReadOnlySpan<int> hashCodes, bool optimizeForR
             ArrayPool<int>.Shared.Return(seenBuckets);
 
             return bestNumBuckets;
+
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            bool IsBucketFirstVisit(int code)
+            {
+                uint bucketNum = (uint)code % (uint)numBuckets;
+                if ((seenBuckets[bucketNum / BitsPerInt32] & (1 << (int)bucketNum)) != 0)
+                {
+                    numCollisions++;
+                    if (numCollisions >= bestNumCollisions)
+                    {
+                        // If we've already hit the previously known best number of collisions,
+                        // there's no point in continuing as worst case we'd just use that.
+                        return false;
+                    }
+                }
+                else
+                {
+                    seenBuckets[bucketNum / BitsPerInt32] |= 1 << (int)bucketNum;
+                }
+
+                return true;
+            }
         }
 
         private readonly struct Bucket

From 1e071d04628640e4e91f9819e73a12815e4a90b1 Mon Sep 17 00:00:00 2001
From: Adam Sitnik <adam.sitnik@gmail.com>
Date: Thu, 22 Jun 2023 20:11:41 +0200
Subject: [PATCH 2/2] Based on our observations, in more than 99.5% of cases
 the number of buckets that meets our criteria is

at least twice as big as the number of unique hash codes.

+33% gain on average
---
 .../src/System/Collections/Frozen/FrozenHashTable.cs        | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
index 05ab18a5f4b6bb..9b71eb993b0da0 100644
--- a/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
+++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
@@ -178,12 +178,16 @@ private static int CalcNumBuckets(ReadOnlySpan<int> hashCodes, bool optimizeForR
             }
             Debug.Assert(uniqueCodesCount != 0);
 
+            // Based on our observations, in more than 99.5% of cases the number of buckets that meets our criteria is
+            // at least twice as big as the number of unique hash codes.
+            int minNumBuckets = uniqueCodesCount * 2;
+
             // In our precomputed primes table, find the index of the smallest prime that's at least as large as our number of
             // hash codes. If there are more codes than in our precomputed primes table, which accommodates millions of values,
             // give up and just use the next prime.
             ReadOnlySpan<int> primes = HashHelpers.Primes;
             int minPrimeIndexInclusive = 0;
-            while ((uint)minPrimeIndexInclusive < (uint)primes.Length && uniqueCodesCount > primes[minPrimeIndexInclusive])
+            while ((uint)minPrimeIndexInclusive < (uint)primes.Length && minNumBuckets > primes[minPrimeIndexInclusive])
             {
                 minPrimeIndexInclusive++;
             }
