Tolerate pathological growth of optCSEHash (dotnet#40056)

The optCSEhash can grow an unbounded amount if the function has numerous trees which are put into the optCSEhash as possible CSE candidates, but fewer than MAX_CSE_CNT are found, then the compiler will spend excessive amounts of time looking up entries in the optCSEhash. This fix addresses the issue by making the optCSEhash able to grow its count of buckets.
TIHan · Jul 29, 2020 · 50d412d · 50d412d
1 parent a4bd2fb
commit 50d412d
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 14 deletions.
diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h
@@ -6363,7 +6363,12 @@ class Compiler
                                    // not used for shared const CSE's
     };
 
-    static const size_t s_optCSEhashSize;
+    static const size_t s_optCSEhashSizeInitial;
+    static const size_t s_optCSEhashGrowthFactor;
+    static const size_t s_optCSEhashBucketSize;
+    size_t              optCSEhashSize;                 // The current size of hashtable
+    size_t              optCSEhashCount;                // Number of entries in hashtable
+    size_t              optCSEhashMaxCountBeforeResize; // Number of entries before resize
     CSEdsc**            optCSEhash;
     CSEdsc**            optCSEtab;
 

diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp
@@ -20,7 +20,9 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 /*****************************************************************************/
 
 /* static */
-const size_t Compiler::s_optCSEhashSize = EXPSET_SZ * 2;
+const size_t Compiler::s_optCSEhashSizeInitial  = EXPSET_SZ * 2;
+const size_t Compiler::s_optCSEhashGrowthFactor = 2;
+const size_t Compiler::s_optCSEhashBucketSize   = 4;
 
 /*****************************************************************************
  *
@@ -36,11 +38,11 @@ void Compiler::optCSEstop()
 
     CSEdsc*  dsc;
     CSEdsc** ptr;
-    unsigned cnt;
+    size_t   cnt;
 
     optCSEtab = new (this, CMK_CSE) CSEdsc*[optCSECandidateCount]();
 
-    for (cnt = s_optCSEhashSize, ptr = optCSEhash; cnt; cnt--, ptr++)
+    for (cnt = optCSEhashSize, ptr = optCSEhash; cnt; cnt--, ptr++)
     {
         for (dsc = *ptr; dsc; dsc = dsc->csdNextInBucket)
         {
@@ -373,7 +375,11 @@ void Compiler::optValnumCSE_Init()
     cseMaskTraits = nullptr;
 
     // Allocate and clear the hash bucket table
-    optCSEhash = new (this, CMK_CSE) CSEdsc*[s_optCSEhashSize]();
+    optCSEhash = new (this, CMK_CSE) CSEdsc*[s_optCSEhashSizeInitial]();
+
+    optCSEhashSize                 = s_optCSEhashSizeInitial;
+    optCSEhashMaxCountBeforeResize = optCSEhashSize * s_optCSEhashBucketSize;
+    optCSEhashCount                = 0;
 
     optCSECandidateCount = 0;
     optDoCSE             = false; // Stays false until we find duplicate CSE tree
@@ -382,6 +388,20 @@ void Compiler::optValnumCSE_Init()
     optCseCheckedBoundMap = nullptr;
 }
 
+unsigned optCSEKeyToHashIndex(size_t key, size_t optCSEhashSize)
+{
+    unsigned hash;
+
+    hash = (unsigned)key;
+#ifdef TARGET_64BIT
+    hash ^= (unsigned)(key >> 32);
+#endif
+    hash *= (unsigned)(optCSEhashSize + 1);
+    hash >>= 7;
+
+    return hash % optCSEhashSize;
+}
+
 //---------------------------------------------------------------------------
 // optValnumCSE_Index:
 //               - Returns the CSE index to use for this tree,
@@ -402,7 +422,6 @@ void Compiler::optValnumCSE_Init()
 unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
 {
     size_t   key;
-    unsigned hash;
     unsigned hval;
     CSEdsc*  hashDsc;
     bool     isIntConstHash       = false;
@@ -502,14 +521,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
 
     // Compute the hash value for the expression
 
-    hash = (unsigned)key;
-#ifdef TARGET_64BIT
-    hash ^= (unsigned)(key >> 32);
-#endif
-    hash *= (unsigned)(s_optCSEhashSize + 1);
-    hash >>= 7;
-
-    hval = hash % s_optCSEhashSize;
+    hval = optCSEKeyToHashIndex(key, optCSEhashSize);
 
     /* Look for a matching index in the hash table */
 
@@ -627,6 +639,37 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt)
 
         if (optCSECandidateCount < MAX_CSE_CNT)
         {
+            if (optCSEhashCount == optCSEhashMaxCountBeforeResize)
+            {
+                size_t   newOptCSEhashSize = optCSEhashSize * s_optCSEhashGrowthFactor;
+                CSEdsc** newOptCSEhash     = new (this, CMK_CSE) CSEdsc*[newOptCSEhashSize]();
+
+                // Iterate through each existing entry, moving to the new table
+                CSEdsc** ptr;
+                CSEdsc*  dsc;
+                size_t   cnt;
+                for (cnt = optCSEhashSize, ptr = optCSEhash; cnt; cnt--, ptr++)
+                {
+                    for (dsc = *ptr; dsc;)
+                    {
+                        CSEdsc* nextDsc = dsc->csdNextInBucket;
+
+                        size_t newHval = optCSEKeyToHashIndex(dsc->csdHashKey, newOptCSEhashSize);
+
+                        // Move CSEdsc to bucket in enlarged table
+                        dsc->csdNextInBucket   = newOptCSEhash[newHval];
+                        newOptCSEhash[newHval] = dsc;
+
+                        dsc = nextDsc;
+                    }
+                }
+
+                optCSEhash                     = newOptCSEhash;
+                optCSEhashSize                 = newOptCSEhashSize;
+                optCSEhashMaxCountBeforeResize = optCSEhashMaxCountBeforeResize * s_optCSEhashGrowthFactor;
+            }
+
+            ++optCSEhashCount;
             hashDsc = new (this, CMK_CSE) CSEdsc;
 
             hashDsc->csdHashKey        = key;