Completely lock-free ClassLoader::LookupTypeKey (#61346)

* embedded size

* next array

* read consistency while rehashing

* comments

* remove CRST_UNSAFE_ANYMODE and COOP mode in the callers

* typo

* fix 32bit builds

* couple changes from review.

* Walk the buckets in resize.

* remove a `REVIEW:` comment.

* Update src/coreclr/vm/dacenumerablehash.inl

PR review suggestion

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

* remove use of `auto`

* DAC stuff

* Constructor and GrowTable are not called by DAC, no need for DPTR, added a comment about a cast.

* SKIP_SPECIAL_SLOTS

* More compact dac_cast in GetNext

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Author: VSadov

src/coreclr/vm/clsload.cpp

@@ -1061,27 +1061,8 @@ void ClassLoader::TryEnsureLoaded(TypeHandle typeHnd, ClassLoadLevel level)
 #endif // DACCESS_COMPILE
 }
 
-// This is separated out to avoid the overhead of C++ exception handling in the non-locking case.
 /* static */
-TypeHandle ClassLoader::LookupTypeKeyUnderLock(TypeKey *pKey,
-                                               EETypeHashTable *pTable,
-                                               CrstBase *pLock)
-{
-    WRAPPER_NO_CONTRACT;
-    SUPPORTS_DAC;
-
-    // m_AvailableTypesLock has to be taken in cooperative mode to avoid deadlocks during GC
-    GCX_MAYBE_COOP_NO_THREAD_BROKEN(!IsGCThread());
-
-    CrstHolder ch(pLock);
-    return pTable->GetValue(pKey);
-}
-
-/* static */
-TypeHandle ClassLoader::LookupTypeKey(TypeKey *pKey,
-                                      EETypeHashTable *pTable,
-                                      CrstBase *pLock,
-                                      BOOL fCheckUnderLock)
+TypeHandle ClassLoader::LookupTypeKey(TypeKey *pKey, EETypeHashTable *pTable)
 {
     CONTRACTL {
         NOTHROW;
@@ -1090,26 +1071,15 @@ TypeHandle ClassLoader::LookupTypeKey(TypeKey *pKey,
         PRECONDITION(CheckPointer(pKey));
         PRECONDITION(pKey->IsConstructed());
         PRECONDITION(CheckPointer(pTable));
-        PRECONDITION(!fCheckUnderLock || CheckPointer(pLock));
         MODE_ANY;
         SUPPORTS_DAC;
     } CONTRACTL_END;
 
-    TypeHandle th;
-
-    if (fCheckUnderLock)
-    {
-        th = LookupTypeKeyUnderLock(pKey, pTable, pLock);
-    }
-    else
-    {
-        th = pTable->GetValue(pKey);
-    }
-    return th;
+    return pTable->GetValue(pKey);
 }
 
 /* static */
-TypeHandle ClassLoader::LookupInLoaderModule(TypeKey *pKey, BOOL fCheckUnderLock)
+TypeHandle ClassLoader::LookupInLoaderModule(TypeKey *pKey)
 {
     CONTRACTL {
         NOTHROW;
@@ -1124,39 +1094,12 @@ TypeHandle ClassLoader::LookupInLoaderModule(TypeKey *pKey, BOOL fCheckUnderLock
     Module *pLoaderModule = ComputeLoaderModule(pKey);
     PREFIX_ASSUME(pLoaderModule!=NULL);
 
-    return LookupTypeKey(pKey,
-                         pLoaderModule->GetAvailableParamTypes(),
-                         &pLoaderModule->GetClassLoader()->m_AvailableTypesLock,
-                         fCheckUnderLock);
+    return LookupTypeKey(pKey, pLoaderModule->GetAvailableParamTypes());
 }
 
 
 /* static */
 TypeHandle ClassLoader::LookupTypeHandleForTypeKey(TypeKey *pKey)
-{
-    WRAPPER_NO_CONTRACT;
-    SUPPORTS_DAC;
-
-    // Make an initial lookup without taking any locks.
-    TypeHandle th = LookupTypeHandleForTypeKeyInner(pKey, FALSE);
-
-    // A non-null TypeHandle for the above lookup indicates success
-    // A null TypeHandle only indicates "well, it might have been there,
-    // try again with a lock".  This kind of negative result will
-    // only happen while accessing the underlying EETypeHashTable
-    // during a resize, i.e. very rarely. In such a case, we just
-    // perform the lookup again, but indicate that appropriate locks
-    // should be taken.
-
-    if (th.IsNull())
-    {
-        th = LookupTypeHandleForTypeKeyInner(pKey, TRUE);
-    }
-
-    return th;
-}
-/* static */
-TypeHandle ClassLoader::LookupTypeHandleForTypeKeyInner(TypeKey *pKey, BOOL fCheckUnderLock)
 {
     CONTRACTL
     {
@@ -1184,7 +1127,7 @@ TypeHandle ClassLoader::LookupTypeHandleForTypeKeyInner(TypeKey *pKey, BOOL fChe
     // If the thing is not NGEN'd then this may
     // be different to pPreferredZapModule.  If they are the same then
     // we can reuse the results of the lookup above.
-    TypeHandle thLM = LookupInLoaderModule(pKey, fCheckUnderLock);
+    TypeHandle thLM = LookupInLoaderModule(pKey);
     if (!thLM.IsNull())
     {
         return thLM;
@@ -2055,11 +1998,10 @@ VOID ClassLoader::Init(AllocMemTracker *pamTracker)
                              CrstAvailableClass,
                              CRST_REENTRANCY);
 
-    // This lock is taken within the classloader whenever we have to insert a new param. type into the table
-    // This lock also needs to be taken for a read operation in a GC_NOTRIGGER scope, thus the ANYMODE flag.
+    // This lock is taken within the classloader whenever we have to insert a new param. type into the table.
     m_AvailableTypesLock.Init(
-                                  CrstAvailableParamTypes,
-                                  (CrstFlags)(CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD));
+                              CrstAvailableParamTypes,
+                              CRST_DEBUGGER_THREAD);
 
 #ifdef _DEBUG
     CorTypeInfo::CheckConsistency();
@@ -3104,9 +3046,6 @@ TypeHandle ClassLoader::PublishType(TypeKey *pTypeKey, TypeHandle typeHnd)
         Module *pLoaderModule = ComputeLoaderModule(pTypeKey);
         EETypeHashTable *pTable = pLoaderModule->GetAvailableParamTypes();
 
-        // m_AvailableTypesLock has to be taken in cooperative mode to avoid deadlocks during GC
-        GCX_COOP();
-
         CrstHolder ch(&pLoaderModule->GetClassLoader()->m_AvailableTypesLock);
 
         // The type could have been loaded by a different thread as side-effect of avoiding deadlocks caused by LoadsTypeViolation
@@ -3121,9 +3060,6 @@ TypeHandle ClassLoader::PublishType(TypeKey *pTypeKey, TypeHandle typeHnd)
         Module *pModule = pTypeKey->GetModule();
         mdTypeDef typeDef = pTypeKey->GetTypeToken();
 
-        // m_AvailableTypesLock has to be taken in cooperative mode to avoid deadlocks during GC
-        GCX_COOP();
-
         CrstHolder ch(&pModule->GetClassLoader()->m_AvailableTypesLock);
 
         // ! We cannot fail after this point.

src/coreclr/vm/clsload.hpp

@@ -899,21 +899,13 @@ class ClassLoader
                                                   ClassLoadLevel level = CLASS_LOADED,
                                                   const InstantiationContext *pInstContext = NULL);
 
-    static TypeHandle LookupTypeKeyUnderLock(TypeKey *pKey,
-                                             EETypeHashTable *pTable,
-                                             CrstBase *pLock);
+    static TypeHandle LookupTypeKey(TypeKey *pKey, EETypeHashTable *pTable);
 
-    static TypeHandle LookupTypeKey(TypeKey *pKey,
-                                    EETypeHashTable *pTable,
-                                    CrstBase *pLock,
-                                    BOOL fCheckUnderLock);
-
-    static TypeHandle LookupInLoaderModule(TypeKey* pKey, BOOL fCheckUnderLock);
+    static TypeHandle LookupInLoaderModule(TypeKey* pKey);
 
     // Lookup a handle in the appropriate table
     // (declaring module for TypeDef or loader-module for constructed types)
     static TypeHandle LookupTypeHandleForTypeKey(TypeKey *pTypeKey);
-    static TypeHandle LookupTypeHandleForTypeKeyInner(TypeKey *pTypeKey, BOOL fCheckUnderLock);
 
     static void DECLSPEC_NORETURN  ThrowTypeLoadException(TypeKey *pKey, UINT resIDWhy);
 

src/coreclr/vm/dacenumerablehash.h

@@ -96,6 +96,16 @@ class DacEnumerableHashTable
     void EnumMemoryRegions(CLRDataEnumMemoryFlags flags);
 #endif // DACCESS_COMPILE
 
+private:
+    struct VolatileEntry;
+    typedef DPTR(struct VolatileEntry) PTR_VolatileEntry;
+    struct VolatileEntry
+    {
+        VALUE               m_sValue;           // The derived-class format of an entry
+        PTR_VolatileEntry   m_pNextEntry;       // Pointer to the next entry in the bucket chain (or NULL)
+        DacEnumerableHashValue       m_iHashValue;       // The hash value associated with the entry
+    };
+
 protected:
     // This opaque structure provides enumeration context when walking the set of entries which share a common
     // hash code. Initialized by BaseFindFirstEntryByHash and read/updated by BaseFindNextEntryByHash.
@@ -106,6 +116,7 @@ class DacEnumerableHashTable
         TADDR   m_pEntry;               // The entry the caller is currently looking at (or NULL to begin
                                         // with). This is a VolatileEntry* and should always be a target address
                                         // not a DAC PTR_.
+        DPTR(PTR_VolatileEntry)   m_curBuckets;   // The bucket table we are working with.
     };
 
     // This opaque structure provides enumeration context when walking all entries in the table. Initialized
@@ -176,16 +187,9 @@ class DacEnumerableHashTable
     PTR_Module m_pModule;
 
 private:
+    private:
     // Internal implementation details. Nothing of interest to sub-classers for here on.
-
-    struct VolatileEntry;
-    typedef DPTR(struct VolatileEntry) PTR_VolatileEntry;
-    struct VolatileEntry
-    {
-        VALUE               m_sValue;           // The derived-class format of an entry
-        PTR_VolatileEntry   m_pNextEntry;       // Pointer to the next entry in the bucket chain (or NULL)
-        DacEnumerableHashValue       m_iHashValue;       // The hash value associated with the entry
-    };
+    DPTR(VALUE) BaseFindFirstEntryByHashCore(DPTR(PTR_VolatileEntry) curBuckets, DacEnumerableHashValue iHash, LookupContext* pContext);
 
 #ifndef DACCESS_COMPILE
     // Determine loader heap to be used for allocation of entries and bucket lists.
@@ -206,11 +210,27 @@ class DacEnumerableHashTable
         return m_pBuckets;
     }
 
+    // our bucket table uses two extra slots - slot [0] contains the length of the table,
+    //                                         slot [1] will contain the next version of the table if it resizes
+    static const int SLOT_LENGTH = 0;
+    static const int SLOT_NEXT = 1;
+    // normal slots start at slot #2
+    static const int SKIP_SPECIAL_SLOTS = 2;
+    
+    static DWORD GetLength(DPTR(PTR_VolatileEntry) buckets)
+    {
+        return (DWORD)dac_cast<TADDR>(buckets[SLOT_LENGTH]);
+    }
+
+    static DPTR(PTR_VolatileEntry) GetNext(DPTR(PTR_VolatileEntry) buckets)
+    {
+        return dac_cast<DPTR(PTR_VolatileEntry)>(buckets[SLOT_NEXT]);
+    }
+
     // Loader heap provided at construction time. May be NULL (in which case m_pModule must *not* be NULL).
     LoaderHeap             *m_pHeap;
 
     DPTR(PTR_VolatileEntry)                  m_pBuckets;  // Pointer to a simple bucket list (array of VolatileEntry pointers)
-    DWORD                                    m_cBuckets;  // Count of buckets in the above array (always non-zero)
     DWORD                                    m_cEntries;  // Count of elements
 };