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 1 commit
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
40 changes: 28 additions & 12 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,7 +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_.
TADDR m_curTable;
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 @@ -177,18 +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(PTR_VolatileEntry* curBuckets, DacEnumerableHashValue iHash, LookupContext* pContext);
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 @@ -209,6 +210,21 @@ 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;

Expand Down
32 changes: 16 additions & 16 deletions src/coreclr/vm/dacenumerablehash.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::DacEnumerableHashTable(Module *pModu
S_SIZE_T cbBuckets = S_SIZE_T(sizeof(VolatileEntry*)) * (S_SIZE_T(cInitialBuckets) + S_SIZE_T(2));

m_cEntries = 0;
PTR_VolatileEntry* pBuckets = (PTR_VolatileEntry*)(void*)GetHeap()->AllocMem(cbBuckets);
((size_t*)pBuckets)[0] = cInitialBuckets;
DPTR(PTR_VolatileEntry) pBuckets = (DPTR(PTR_VolatileEntry))(void*)GetHeap()->AllocMem(cbBuckets);
((size_t*)pBuckets)[SLOT_LENGTH] = cInitialBuckets;
hoyosjs marked this conversation as resolved.
Show resolved Hide resolved

// publish after setting the length
VolatileStore(&m_pBuckets, pBuckets);
Expand Down Expand Up @@ -131,7 +131,7 @@ void DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseInsertEntry(DacEnumerableHa
pVolatileEntry->m_iHashValue = iHash;

DPTR(PTR_VolatileEntry) curBuckets = GetBuckets();
DWORD cBuckets = (DWORD)((size_t*)curBuckets)[0];
DWORD cBuckets = GetLength(curBuckets);

// Compute which bucket the entry belongs in based on the hash. (+2 to skip "length" and "next" slots)
DWORD dwBucket = iHash % cBuckets + 2;
Expand Down Expand Up @@ -170,7 +170,7 @@ void DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::GrowTable()
FAULT_NOT_FATAL();

DPTR(PTR_VolatileEntry) curBuckets = GetBuckets();
DWORD cBuckets = (DWORD)((size_t*)curBuckets)[0];
DWORD cBuckets = GetLength(curBuckets);

// Make the new bucket table larger by the scale factor requested by the subclass (but also prime).
DWORD cNewBuckets = NextLargestPrime(cBuckets * SCALE_FACTOR);
Expand All @@ -183,9 +183,9 @@ void DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::GrowTable()
return;

// element 0 stores the length of the table
((size_t*)pNewBuckets)[0] = cNewBuckets;
((size_t*)pNewBuckets)[SLOT_LENGTH] = cNewBuckets;
// element 1 stores the next version of the table (after length is written)
VolatileStore(&((PTR_VolatileEntry**)curBuckets)[1], pNewBuckets);
VolatileStore(&((PTR_VolatileEntry**)curBuckets)[SLOT_NEXT], pNewBuckets);

// All buckets are initially empty.
// Note: Memory allocated on loader heap is zero filled
Expand Down Expand Up @@ -283,12 +283,12 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindFirstEntryByHash
if (m_cEntries == 0)
return NULL;

PTR_VolatileEntry* curBuckets = GetBuckets();
DPTR(PTR_VolatileEntry) curBuckets = GetBuckets();
return BaseFindFirstEntryByHashCore(curBuckets, iHash, pContext);
}

template <DAC_ENUM_HASH_PARAMS>
DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindFirstEntryByHashCore(PTR_VolatileEntry* curBuckets, DacEnumerableHashValue iHash, LookupContext* pContext)
DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindFirstEntryByHashCore(DPTR(PTR_VolatileEntry) curBuckets, DacEnumerableHashValue iHash, LookupContext* pContext)
{
CONTRACTL
{
Expand All @@ -302,7 +302,7 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindFirstEntryByHash

do
{
DWORD cBuckets = (DWORD)dac_cast<TADDR>(curBuckets[0]);
DWORD cBuckets = GetLength(curBuckets);

// Compute which bucket the entry belongs in based on the hash.
// +2 to skip "length" and "next" slots
Expand All @@ -321,7 +321,7 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindFirstEntryByHash
// Record our current search state into the provided context so that a subsequent call to
// BaseFindNextEntryByHash can pick up the search where it left off.
pContext->m_pEntry = dac_cast<TADDR>(pEntry);
pContext->m_curTable = dac_cast<TADDR>(curBuckets);
pContext->m_curBuckets = curBuckets;

// Return the address of the sub-classes' embedded entry structure.
return VALUE_FROM_VOLATILE_ENTRY(pEntry);
Expand All @@ -336,7 +336,7 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindFirstEntryByHash
// since we unlink it from old only after linking into the new.
// check for next table must hapen after we looked through the current.
VolatileLoadBarrier();
curBuckets = (DPTR(PTR_VolatileEntry))dac_cast<TADDR>(curBuckets[1]);
curBuckets = GetNext(curBuckets);
} while (curBuckets != nullptr);

// If we get here then none of the entries in the target bucket matched the hash code and we have a miss
Expand Down Expand Up @@ -384,7 +384,7 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseFindNextEntryByHash(
VolatileLoadBarrier();

// in a case if resize is in progress, look in the new table as well.
DPTR(PTR_VolatileEntry) nextBuckets = ((DPTR(PTR_VolatileEntry)*)pContext->m_curTable)[1];
DPTR(PTR_VolatileEntry) nextBuckets = GetNext(pContext->m_curBuckets);
if (nextBuckets != nullptr)
{
return BaseFindFirstEntryByHashCore(nextBuckets, iHash, pContext);
Expand Down Expand Up @@ -435,7 +435,7 @@ void DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::EnumMemoryRegions(CLRDataEnumMe
DacEnumMemoryRegion(dac_cast<TADDR>(this), sizeof(FINAL_CLASS));

DPTR(PTR_VolatileEntry) curBuckets = GetBuckets();
DWORD cBuckets = (DWORD)dac_cast<TADDR>(curBuckets[0]);
DWORD cBuckets = GetLength(curBuckets);

// Save the bucket list.
DacEnumMemoryRegion(dac_cast<TADDR>(curBuckets), cBuckets * sizeof(VolatileEntry*));
Expand All @@ -460,7 +460,7 @@ void DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::EnumMemoryRegions(CLRDataEnumMe
}

// we should not be resizing while enumerating memory regions.
_ASSERTE(curBuckets[1] == NULL);
_ASSERTE(GetNext(curBuckets) == NULL);

// Save the module if present.
if (GetModule().IsValid())
Expand Down Expand Up @@ -496,7 +496,7 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseIterator::Next()
CONTRACTL_END;

DPTR(PTR_VolatileEntry) curBuckets = m_pTable->GetBuckets();
DWORD cBuckets = (DWORD)dac_cast<TADDR>(curBuckets[0]);
DWORD cBuckets = GetLength(curBuckets);

while (m_dwBucket < cBuckets)
{
Expand All @@ -523,7 +523,7 @@ DPTR(VALUE) DacEnumerableHashTable<DAC_ENUM_HASH_ARGS>::BaseIterator::Next()
}

// we should not be resizing while iterating.
_ASSERTE(curBuckets[1] == NULL);
_ASSERTE(GetNext(curBuckets) == NULL);

return NULL;
}