-
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
A few follow up changes to LookupTypeKey change #61718
Conversation
GetClassValue(nhTable, pName, &Data, &pTable, pLookInThisModuleOnly, &foundEntry, loadFlag, needsToBuildHashtable); | ||
pBucket = foundEntry.GetClassHashBasedEntryValue(); | ||
|
||
if (!needsToBuildHashtable || (m_cUnhashedModules == 0)) |
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.
We do not need to do lookup 3 times if tables were populated by another thread when we did a lookup.
Just one retry after taking a lock is sufficient in all cases.
Also tweaked comments to explain the real reason why we need to re-do the lookup.
(it is not about resizes - those are safe, it is about another thread running exactly the same code and filling up the table, so the first time we could see a table that is only partially built)
} | ||
#endif // !DACCESS_COMPILE | ||
// Replace AvailableClasses Module entry with found TypeHandle | ||
if (!typeHnd.IsNull() && |
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.
This is an optimization where we can replace a token with a type handle as the value in the table. That would save a lookup in another table next time.
There is some additional infrastructure in the table to support this optimization and this is very intentional, but I think someone misplaced }
so this code never runs.
All successful lookups break from the loop, so the code taking the note of the found handle must be outside of the loop body.
@@ -719,6 +716,9 @@ void ClassLoader::GetClassValue(NameHandleTable nhTable, | |||
} | |||
_ASSERTE(pTable); | |||
|
|||
mdToken mdEncloser; | |||
BOOL isNested = IsNested(pName, &mdEncloser); |
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.
IsNested
is a bit involved. In some cases like R2R, we may not need to know isNested
, so it makes sense to move the call down to where we are sure we need it.
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.
Thanks
Thanks! |
As a followup to the LookupTypeKey PR (#61346) I looked at possibility of reducing nested types hash collisions in
EEClassHashTable
by mixing hashes of enclosers into the hash.That was implemented in #61652, but I was not convinced the results justify added complexity.
I do not think we should take the whole #61652.
So here I have a smaller PR with just few unambiguous improvements that I came up with while working on #61652 . I do not think we want the rest of the change.