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

A few follow up changes to LookupTypeKey change #61718

Merged
merged 3 commits into from
Nov 17, 2021
Merged

A few follow up changes to LookupTypeKey change #61718

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a391ae
re-enable optimization when a token is replaced by corresponding type…
VSadov Nov 17, 2021
aebd63b
compute "isNested" ony when we need it.
VSadov Nov 17, 2021
4407a1e
no need to make more than 2 attempts at reading
VSadov Nov 17, 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
49 changes: 27 additions & 22 deletions src/coreclr/vm/clsload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,6 @@ void ClassLoader::GetClassValue(NameHandleTable nhTable,
CONTRACTL_END


mdToken mdEncloser;
EEClassHashEntry_t *pBucket = NULL;

needsToBuildHashtable = FALSE;
Expand All @@ -643,8 +642,6 @@ void ClassLoader::GetClassValue(NameHandleTable nhTable,
}
#endif

BOOL isNested = IsNested(pName, &mdEncloser);

PTR_Assembly assembly = GetAssembly();
PREFIX_ASSUME(assembly != NULL);
ModuleIterator i = assembly->IterateModules();
Expand Down Expand Up @@ -719,6 +716,9 @@ void ClassLoader::GetClassValue(NameHandleTable nhTable,
}
_ASSERTE(pTable);

mdToken mdEncloser;
BOOL isNested = IsNested(pName, &mdEncloser);
Copy link
Member Author

@VSadov VSadov Nov 17, 2021 •
edited
Loading

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.


if (isNested)
{
Module *pNameModule = pName->GetTypeModule();
Expand Down Expand Up @@ -834,6 +834,9 @@ void ClassLoader::LazyPopulateCaseSensitiveHashTables()
}
CONTRACTL_END;


_ASSERT(m_cUnhashedModules > 0);

AllocMemTracker amTracker;
ModuleIterator i = GetAssembly()->IterateModules();

Expand Down Expand Up @@ -909,6 +912,8 @@ void ClassLoader::LazyPopulateCaseInsensitiveHashTables()
amTracker.SuppressRelease();
pModule->SetAvailableClassCaseInsHash(pNewClassCaseInsHash);
FastInterlockDecrement((LONG*)&m_cUnhashedModules);

_ASSERT(m_cUnhashedModules >= 0);
}
}
}
Expand Down Expand Up @@ -1238,18 +1243,18 @@ BOOL ClassLoader::FindClassModuleThrowing(

if (pBucket == NULL)
{
// Take the lock. To make sure the table is not being built by another thread.
AvailableClasses_LockHolder lh(this);

// Try again with the lock. This will protect against another thread reallocating
// the hash table underneath us
GetClassValue(nhTable, pName, &Data, &pTable, pLookInThisModuleOnly, &foundEntry, loadFlag, needsToBuildHashtable);
pBucket = foundEntry.GetClassHashBasedEntryValue();

if (!needsToBuildHashtable || (m_cUnhashedModules == 0))
Copy link
Member Author

@VSadov VSadov Nov 17, 2021 •
edited
Loading

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)

{
// the table should be finished now, try again
GetClassValue(nhTable, pName, &Data, &pTable, pLookInThisModuleOnly, &foundEntry, loadFlag, needsToBuildHashtable);
pBucket = foundEntry.GetClassHashBasedEntryValue();
}
#ifndef DACCESS_COMPILE
if (needsToBuildHashtable && (pBucket == NULL) && (m_cUnhashedModules > 0))
else
{
_ASSERT(needsToBuildHashtable);

if (nhTable == nhCaseInsensitive)
{
LazyPopulateCaseInsensitiveHashTables();
Expand All @@ -1268,7 +1273,7 @@ BOOL ClassLoader::FindClassModuleThrowing(
#endif
}

// Same check as above, but this time we've checked with the lock so the table will be populated
// Same check as above, but this time we've ensured that the tables are populated
if (pBucket == NULL)
{
#if defined(_DEBUG_IMPL) && !defined(DACCESS_COMPILE)
Expand Down Expand Up @@ -1536,19 +1541,19 @@ ClassLoader::LoadTypeHandleThrowing(
pClsLdr = pFoundModule->GetClassLoader();
pLookInThisModuleOnly = NULL;
}
}

#ifndef DACCESS_COMPILE
// Replace AvailableClasses Module entry with found TypeHandle
if (!typeHnd.IsNull() &&
typeHnd.IsRestored() &&
foundEntry.GetEntryType() == HashedTypeEntry::EntryType::IsHashedClassEntry &&
(foundEntry.GetClassHashBasedEntryValue() != NULL) &&
(foundEntry.GetClassHashBasedEntryValue()->GetData() != typeHnd.AsPtr()))
{
foundEntry.GetClassHashBasedEntryValue()->SetData(typeHnd.AsPtr());
}
#endif // !DACCESS_COMPILE
// Replace AvailableClasses Module entry with found TypeHandle
if (!typeHnd.IsNull() &&
Copy link
Member Author

@VSadov VSadov Nov 17, 2021 •
edited
Loading

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.

typeHnd.IsRestored() &&
foundEntry.GetEntryType() == HashedTypeEntry::EntryType::IsHashedClassEntry &&
(foundEntry.GetClassHashBasedEntryValue() != NULL) &&
(foundEntry.GetClassHashBasedEntryValue()->GetData() != typeHnd.AsPtr()))
{
foundEntry.GetClassHashBasedEntryValue()->SetData(typeHnd.AsPtr());
}
#endif // !DACCESS_COMPILE

RETURN typeHnd;
} // ClassLoader::LoadTypeHandleThrowing
Expand Down