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.

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

Move PendingTypeLoadTable from the ClassLoader object to a global structure #96653

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

davidwrighton
Copy link
Member

  • Shard the hashtable so that it will rarely have lock contention
  • Pre-allocate the PendingTypeLoad Entries. This reduces allocator pressure on startup substantially, especially in the presence of multithreaded loading where the struct is allocated on 1 thread and often freed on another.

The effectiveness of the pre-allocation and sharding heuristics was measured on a complex ASP.NET scenario tweaked to perform extremely high numbers of multithreaded loads and produced startup wins of about 10%.

…ucture

- Shard the hashtable so that it will rarely have lock contention
- Pre-allocate the PendingTypeLoad Entries. This reduces allocator pressure on startup substantially, especially in the presence of multithreaded loading where the struct is allocated on 1 thread and often freed on another.

The effectiveness of the pre-allocation and sharding heuristics was measured on a complex ASP.NET scenario tweaked to perform extremely high numbers of multithreaded loads and produced startup wins of about 10%.
src/coreclr/vm/pendingload.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/pendingload.cpp Outdated Show resolved Hide resolved
friend class ClassLoader; // workaround really need to beef up the API below
static void CallCrstEnter(Crst* pCrst)
{
pCrst->Enter(INDEBUG(Crst::CRST_NO_LEVEL_CHECK));
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for the CRST_NO_LEVEL_CHECK?

Copy link
Member

@elinor-fung elinor-fung Jan 22, 2024

Choose a reason for hiding this comment

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

I assume this is from:

// The PendingTypeLoadEntry() lock has a higher level than UnresolvedClassLock.
// But whenever we create one, we have to acquire it while holding the UnresolvedClassLock.
// This is safe since we're the ones that created the lock and are guaranteed to acquire
// it without blocking. But to prevent the crstlevel system from asserting, we
// must acquire using a special method.
//---------------------------------------------------------------------------
m_Crst.Enter(INDEBUG(Crst::CRST_NO_LEVEL_CHECK));

But I don't think that reasoning applies to the refactored/static function now?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it still applies as this is called for the entry lock, which is still held at that level. However, this needs to be clearly documented, as the reason for this static function to exist is pure irritating language syntax shenanigans. (The only reason this is a static function on PendingTypeLoadTable at all is to deal with a hole in the C++ language spec where you cannot declare a friend class which is a nested class of a forward declared class.) I need to document this better.

src/coreclr/vm/pendingload.h Outdated Show resolved Hide resolved
src/coreclr/vm/pendingload.h Outdated Show resolved Hide resolved
src/coreclr/vm/pendingload.h Outdated Show resolved Hide resolved
src/coreclr/vm/pendingload.h Outdated Show resolved Hide resolved
src/coreclr/vm/pendingload.h Outdated Show resolved Hide resolved
src/coreclr/vm/pendingload.h Show resolved Hide resolved
src/coreclr/vm/pendingload.h Outdated Show resolved Hide resolved
- Use a const TypeKey
  - Required making a fair amount of other code const correct
- Implement in cpp file instead of header
- Don't use initialization trick
#endif // PENDING_TYPE_LOAD_TABLE_STATS
}

void PendingTypeLoadTable::Entry::SetTypeKey(const TypeKey& typeKey)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void PendingTypeLoadTable::Entry::SetTypeKey(const TypeKey& typeKey)
void PendingTypeLoadTable::Entry::SetTypeKey(TypeKey typeKey)

This should be okay.

}

return FALSE;
return m_hrResult;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return m_hrResult;
return hr;

I assume the logic here is we want to return the operation we branched on. I'm being defensive for non-obvious races.

@davidwrighton davidwrighton merged commit 9a3cacd into dotnet:main Jan 26, 2024
111 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants