-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Move PendingTypeLoadTable from the ClassLoader object to a global structure #96653
Conversation
…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.h
Outdated
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)); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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:
runtime/src/coreclr/vm/pendingload.h
Lines 54 to 60 in 971ebdb
// 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?
There was a problem hiding this comment.
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.
- 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
src/coreclr/vm/pendingload.cpp
Outdated
#endif // PENDING_TYPE_LOAD_TABLE_STATS | ||
} | ||
|
||
void PendingTypeLoadTable::Entry::SetTypeKey(const TypeKey& typeKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void PendingTypeLoadTable::Entry::SetTypeKey(const TypeKey& typeKey) | |
void PendingTypeLoadTable::Entry::SetTypeKey(TypeKey typeKey) |
This should be okay.
src/coreclr/vm/pendingload.cpp
Outdated
} | ||
|
||
return FALSE; | ||
return m_hrResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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%.