Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Completely lock-free ClassLoader::LookupTypeKey #61346

Merged
merged 16 commits into from
Nov 14, 2021
Merged

Completely lock-free ClassLoader::LookupTypeKey #61346

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1b003e3
embedded size
VSadov Nov 8, 2021
5330325
next array
VSadov Nov 8, 2021
c5db8c4
read consistency while rehashing
VSadov Nov 8, 2021
c264f69
comments
VSadov Nov 8, 2021
e8e7d6e
remove CRST_UNSAFE_ANYMODE and COOP mode in the callers
VSadov Nov 9, 2021
b9b4813
typo
VSadov Nov 9, 2021
749695a
fix 32bit builds
VSadov Nov 9, 2021
5569c66
couple changes from review.
VSadov Nov 9, 2021
7c3cd08
Walk the buckets in resize.
VSadov Nov 10, 2021
f8b1021
remove a `REVIEW:` comment.
VSadov Nov 10, 2021
067cefb
Update src/coreclr/vm/dacenumerablehash.inl
VSadov Nov 11, 2021
1516f6b
remove use of `auto`
VSadov Nov 13, 2021
efe6043
DAC stuff
VSadov Nov 13, 2021
f455581
Constructor and GrowTable are not called by DAC, no need for DPTR, ad…
VSadov Nov 13, 2021
e874b3c
SKIP_SPECIAL_SLOTS
VSadov Nov 13, 2021
c7eb7e7
More compact dac_cast in GetNext
VSadov Nov 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 8 additions & 72 deletions src/coreclr/vm/clsload.cpp
View file
Open in desktop
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoyosjs - If debugger only does lookups, then it will no longer take this look, then CRST_DEBUGGER_THREAD is not needed.
Can debugger load/publish types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we can't load assemblies, but it's possible that a FuncEval can load a type I believe.

Copy link
Member Author

@VSadov VSadov Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is funceval running on debugger thread?

I assumed that CRST_DEBUGGER_THREAD does not cover funceaval, since funceval could JIT and JIT does all kind of stuff (including loading assemblies). If that is attributed to the debugger thread, then debugger thread is not different from anything else.
I am not very familiar with how that all works though.

CRST_DEBUGGER_THREAD is not a big nuisance here. I was just not sure it is still necessary.

Copy link
Member Author

@VSadov VSadov Nov 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried removing CRST_DEBUGGER_THREAD and running both regular and diagnostics tests (with Chk bits) - everything runs as before. Not sure if that is enough proof that CRST_DEBUGGER_THREAD is unnecessary.

I just noticed that CRST_DEBUGGER_THREAD is always paired with CRST_UNSAFE_ANYMODE (or sometimes CRST_GC_NOTRIGGER_WHEN_TAKEN), so since we are removing the other one, maybe we do not need CRST_DEBUGGER_THREAD either.

But do not think CRST_DEBUGGER_THREAD is a big nuisance either way - it basically increments/decrements a counter that is checked in couple asserts.


#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
Open in desktop
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
38 changes: 28 additions & 10 deletions src/coreclr/vm/dacenumerablehash.h
View file
Open in desktop
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,25 @@ 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;

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 (DPTR(PTR_VolatileEntry))dac_cast<TADDR>(buckets[SLOT_NEXT]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoyosjs - is this correct or from TADDR it must be another dac_cast ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like:

    static DPTR(PTR_VolatileEntry) GetNext(DPTR(PTR_VolatileEntry) buckets)
    {
        return dac_cast<DPTR(PTR_VolatileEntry)>(dac_cast<TADDR>(buckets[SLOT_NEXT]));
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this right buckets[SLOT_NEXT] is pointing to a new hash table that's an array of linked lists? Then it's

return dac_cast<DPTR(PTR_VolatileEntry)>(dac_cast<TADDR>(buckets[SLOT_NEXT]));

which can be simplified to

return dac_cast<DPTR(PTR_VolatileEntry)>(buckets[SLOT_NEXT]);

Copy link
Member Author

@VSadov VSadov Nov 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The buckets[SLOT_NEXT] will be pointing to the next larger table, if resize happened while you are doing a lookup. It will be rare. It is possible, even though extremely rare to see more than one new version.
Every new one is 2x larger than previous, so there can't be too many even in the ultimate worst case.

}

// 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