Skip to content

Commit

Permalink
Completely lock-free ClassLoader::LookupTypeKey (#61346)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* 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 <[email protected]>
  • Loading branch information
@VSadov @jkotas
VSadov and jkotas authored Nov 14, 2021
1 parent b68328a commit 5da8f31
Show file tree
Hide file tree
Showing 8 changed files with 182 additions and 163 deletions.
80 changes: 8 additions & 72 deletions src/coreclr/vm/clsload.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
12 changes: 2 additions & 10 deletions src/coreclr/vm/clsload.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
40 changes: 30 additions & 10 deletions src/coreclr/vm/dacenumerablehash.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
};

Expand Down
Loading

0 comments on commit 5da8f31

Please sign in to comment.