From 4b02131e2b4e3132c9ab78c7b5d4788f2a6f9e30 Mon Sep 17 00:00:00 2001 From: fadimounir Date: Fri, 14 Jun 2019 12:56:30 -0700 Subject: [PATCH 01/21] Feature: dynamic expansion for generic dictionaries These changes introduce dynamic size expansion for generic dictionary layouts when we run out of slots. The original implementation allowed for an expansion, but using a linked list structure, which made it impossible to use fast lookup slots once we're out of slots in the first bucket. This new implementation allows for the usage of fast lookup slots always, for all generic lookups. This also removes the constraint we had on R2R, where we disabled the usage of fast slots all-together. --- src/inc/corinfo.h | 3 + src/vm/appdomain.cpp | 8 +- src/vm/ceeload.cpp | 147 +++++++++++++- src/vm/ceeload.h | 19 ++ src/vm/class.cpp | 70 +++++++ src/vm/clsload.cpp | 10 +- src/vm/clsload.hpp | 2 + src/vm/genericdict.cpp | 425 ++++++++++++++++++++++++---------------- src/vm/genericdict.h | 72 ++++--- src/vm/genmeth.cpp | 14 +- src/vm/interoputil.cpp | 2 +- src/vm/jitinterface.cpp | 4 +- src/vm/method.cpp | 4 +- src/vm/method.hpp | 17 +- src/vm/prestub.cpp | 12 +- 15 files changed, 581 insertions(+), 228 deletions(-) diff --git a/src/inc/corinfo.h b/src/inc/corinfo.h index 0fcdac29920f..ded06501c174 100644 --- a/src/inc/corinfo.h +++ b/src/inc/corinfo.h @@ -1373,6 +1373,9 @@ struct CORINFO_RUNTIME_LOOKUP // 1 means that value stored at second offset (offsets[1]) from pointer is offset2, and the next pointer is // stored at pointer+offsets[1]+offset2. bool indirectSecondOffset; + + // Dictionary slot index result for a dictionary lookup + WORD slot; } ; // Result of calling embedGenericHandle diff --git a/src/vm/appdomain.cpp b/src/vm/appdomain.cpp index 81d7d89017d4..b27cc46d9fe3 100644 --- a/src/vm/appdomain.cpp +++ b/src/vm/appdomain.cpp @@ -2068,6 +2068,10 @@ void SystemDomain::LoadBaseSystemClasses() g_pDelegateClass = MscorlibBinder::GetClass(CLASS__DELEGATE); g_pMulticastDelegateClass = MscorlibBinder::GetClass(CLASS__MULTICAST_DELEGATE); +#ifndef CROSSGEN_COMPILE + CrossLoaderAllocatorHashSetup::EnsureTypesLoaded(); +#endif + // used by IsImplicitInterfaceOfSZArray MscorlibBinder::GetClass(CLASS__IENUMERABLEGENERIC); MscorlibBinder::GetClass(CLASS__ICOLLECTIONGENERIC); @@ -2083,10 +2087,6 @@ void SystemDomain::LoadBaseSystemClasses() g_pUtf8StringClass = MscorlibBinder::GetClass(CLASS__UTF8_STRING); #endif // FEATURE_UTF8STRING -#ifndef CROSSGEN_COMPILE - CrossLoaderAllocatorHashSetup::EnsureTypesLoaded(); -#endif - #ifndef CROSSGEN_COMPILE ECall::PopulateManagedStringConstructors(); #endif // CROSSGEN_COMPILE diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index 150241ff560b..0964a846d561 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -687,8 +687,12 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) } #endif // defined (PROFILING_SUPPORTED) &&!defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) - LOG((LF_CLASSLOADER, LL_INFO10, "Loaded pModule: \"%ws\".\n", GetDebugName())); +#ifndef CROSSGEN_COMPILE + m_dynamicSlotsHashForTypes.Init(GetLoaderAllocator()); + m_dynamicSlotsHashForMethods.Init(GetLoaderAllocator()); +#endif + LOG((LF_CLASSLOADER, LL_INFO10, "Loaded pModule: \"%ws\".\n", GetDebugName())); } #endif // DACCESS_COMPILE @@ -13233,6 +13237,147 @@ void ReflectionModule::ReleaseILData() Module::ReleaseILData(); } + +void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParentMT, MethodTable* pDependencyMT) +{ + CONTRACTL + { + GC_TRIGGERS; + PRECONDITION(CheckPointer(pGenericParentMT) && CheckPointer(pDependencyMT)); + PRECONDITION(pGenericParentMT->HasInstantiation() && pGenericParentMT != pGenericParentMT->GetCanonicalMethodTable()); + PRECONDITION(SystemDomain::IsUnderDomainLock()); + } + CONTRACTL_END + + GCX_COOP(); + m_dynamicSlotsHashForTypes.Add(pGenericParentMT->GetCanonicalMethodTable(), pDependencyMT, GetLoaderAllocator()); +} + +void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD) +{ + CONTRACTL + { + GC_TRIGGERS; + PRECONDITION(CheckPointer(pDependencyMD) && pDependencyMD->HasMethodInstantiation() && pDependencyMD->IsInstantiatingStub()); + PRECONDITION(SystemDomain::IsUnderDomainLock()); + } + CONTRACTL_END + + GCX_COOP(); + m_dynamicSlotsHashForMethods.Add(pDependencyMD->GetWrappedMethodDesc(), pDependencyMD, GetLoaderAllocator()); +} + +void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout) +{ + CONTRACTL + { + STANDARD_VM_CHECK; + INJECT_FAULT(ThrowOutOfMemory();); + PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); + PRECONDITION(CheckPointer(pMT) && pMT->HasInstantiation() && pMT->GetClass()->GetDictionaryLayout() == pOldLayout); + PRECONDITION(SystemDomain::IsUnderDomainLock()); + + } + CONTRACTL_END + + GCX_COOP(); + + MethodTable* pCanonMT = pMT->GetCanonicalMethodTable(); + DWORD oldInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMT->GetNumGenericArgs(), pOldLayout); + DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMT->GetNumGenericArgs(), pNewLayout); + + // + // Dictionary expansion for types needs to be done in two steps, given how derived types do not directly embed dictionaries + // from parent types, but instead reference them from directly from the parent types: + // 1) Allocate new dictionaries for all instantiated types of the same typedef as the one being expanded. + // 2) For all types that derive from the one being expanded, update the embedded dictinoary pointer to the newly + // allocated one. + // + + auto expandPerInstInfos = [oldInfoSize, newInfoSize, pCanonMT](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) + { + if (!pMTToUpdate->HasSameTypeDefAs(pMTKey)) + return true; + + _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey && pMTKey == pCanonMT); + + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMTToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); + ZeroMemory(pInstOrPerInstInfo, newInfoSize); + + // Copy old dictionary entry contents + memcpy(pInstOrPerInstInfo, (const void*)pMTToUpdate->GetPerInstInfo()[pMTToUpdate->GetNumDicts() - 1].GetValue(), oldInfoSize); + + pMTToUpdate->GetPerInstInfo()[pMTToUpdate->GetNumDicts() - 1].SetValue((Dictionary*)pInstOrPerInstInfo); + + return true; // Keep walking + }; + + m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, expandPerInstInfos); + + auto updateDependentDicts = [pCanonMT](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) + { + if (pMTToUpdate->HasSameTypeDefAs(pMTKey)) + return true; + + MethodTable* pCurrentMT = pMTToUpdate->GetParentMethodTable(); + while (pCurrentMT) + { + if (pCurrentMT->HasSameTypeDefAs(pMTKey)) + { + DWORD dictToUpdate = pCurrentMT->GetNumDicts() - 1; + Dictionary* pUpdatedParentDict = pCurrentMT->GetPerInstInfo()[dictToUpdate].GetValue(); + pMTToUpdate->GetPerInstInfo()[dictToUpdate].SetValue(pUpdatedParentDict); + + return true; // Keep walking + } + pCurrentMT = pCurrentMT->GetParentMethodTable(); + } + + UNREACHABLE(); + }; + + m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, updateDependentDicts); +} + +void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout) +{ + CONTRACTL + { + STANDARD_VM_CHECK; + INJECT_FAULT(ThrowOutOfMemory();); + PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); + PRECONDITION(CheckPointer(pMD)); + PRECONDITION(pMD->HasMethodInstantiation() && pMD->GetDictionaryLayout() == pOldLayout); + PRECONDITION(SystemDomain::IsUnderDomainLock()); + } + CONTRACTL_END + + GCX_COOP(); + + MethodDesc* pCanonMD = pMD->IsInstantiatingStub() ? pMD->GetWrappedMethodDesc() : pMD; + DWORD oldInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pOldLayout); + DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pNewLayout); + + auto lambda = [oldInfoSize, newInfoSize, pCanonMD](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) + { + // Update m_pPerInstInfo for the pMethodDesc being visited here + _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetWrappedMethodDesc() == pMDKey && pMDKey == pCanonMD); + + InstantiatedMethodDesc* pInstantiatedMD = pMDToUpdate->AsInstantiatedMethodDesc(); + + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMDToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); + ZeroMemory(pInstOrPerInstInfo, newInfoSize); + + // Copy old dictionary entry contents + memcpy(pInstOrPerInstInfo, (const void*)pInstantiatedMD->m_pPerInstInfo.GetValue(), oldInfoSize); + + pInstantiatedMD->m_pPerInstInfo.SetValue((Dictionary*)pInstOrPerInstInfo); + + return true; // Keep walking + }; + + m_dynamicSlotsHashForMethods.VisitValuesOfKey(pCanonMD, lambda); +} #endif // !CROSSGEN_COMPILE #endif // !DACCESS_COMPILE diff --git a/src/vm/ceeload.h b/src/vm/ceeload.h index 6c440ed109f0..0533b855ad22 100644 --- a/src/vm/ceeload.h +++ b/src/vm/ceeload.h @@ -3240,6 +3240,25 @@ class Module void SetNativeMetadataAssemblyRefInCache(DWORD rid, PTR_Assembly pAssembly); #endif // !defined(DACCESS_COMPILE) + +#ifndef CROSSGEN_COMPILE +private: + class DynamicDictSlotsForTypesTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; + typedef CrossLoaderAllocatorHash DynamicDictSlotsForTypesTrackerHash; + + class DynamicDictSlotsForMethodsTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; + typedef CrossLoaderAllocatorHash DynamicDictSlotsForMethodsTrackerHash; + + DynamicDictSlotsForTypesTrackerHash m_dynamicSlotsHashForTypes; + DynamicDictSlotsForMethodsTrackerHash m_dynamicSlotsHashForMethods; + +public: + void RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParentMT, MethodTable* pDependencyMT); + void RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD); + + void ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout); + void ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout); +#endif //!CROSSGEN_COMPILE }; // diff --git a/src/vm/class.cpp b/src/vm/class.cpp index b8d3faed61a6..60cc1e24ef42 100644 --- a/src/vm/class.cpp +++ b/src/vm/class.cpp @@ -973,6 +973,76 @@ void ClassLoader::LoadExactParents(MethodTable *pMT) RETURN; } +/*static*/ +void ClassLoader::RecordDependenciesForDictionaryExpansion(MethodTable* pMT) +{ + CONTRACT_VOID + { + STANDARD_VM_CHECK; + PRECONDITION(CheckPointer(pMT)); + PRECONDITION(pMT->CheckLoadLevel(CLASS_LOAD_APPROXPARENTS)); + } + CONTRACT_END; + + +#ifndef CROSSGEN_COMPILE + if (pMT->GetNumDicts() == 0) + RETURN; + + // Check if there are no dependencies that need tracking. There is no point in taking the lock + // below if we don't need to track anything. + { + bool hasSharedMethodTables = false; + MethodTable* pCurrentMT = pMT; + while (pCurrentMT) + { + if (pCurrentMT->HasInstantiation() && !pCurrentMT->IsCanonicalMethodTable()) + { + hasSharedMethodTables = true; + break; + } + pCurrentMT = pCurrentMT->GetParentMethodTable(); + } + + if (!hasSharedMethodTables) + RETURN; + } + + SystemDomain::LockHolder lh; + { + // Update all inherited dictionary pointers which we could not embed, in case they got updated by some + // other thread during a dictionary expansion before taking this current lock. + MethodTable* pParentMT = pMT->GetParentMethodTable(); + if (pParentMT != NULL && pParentMT->HasPerInstInfo()) + { + DWORD nDicts = pParentMT->GetNumDicts(); + for (DWORD iDict = 0; iDict < nDicts; iDict++) + { + if (pMT->GetPerInstInfo()[iDict].GetValueMaybeNull() != pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()) + { + EnsureWritablePages(&pMT->GetPerInstInfo()[iDict]); + pMT->GetPerInstInfo()[iDict].SetValueMaybeNull(pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()); + } + } + } + + // Add the current type as a dependency to its canonical version, as well as a dependency to all parent + // types in the hierarchy with dictionaries, so that if one of the base types gets a dictionary expansion, we make + // sure to update the derived type's parent dictionary pointer. + MethodTable* pCurrentMT = pMT; + while (pCurrentMT) + { + if (pCurrentMT->HasInstantiation() && !pCurrentMT->IsCanonicalMethodTable()) + pCurrentMT->GetModule()->RecordTypeForDictionaryExpansion_Locked(pCurrentMT, pMT); + + pCurrentMT = pCurrentMT->GetParentMethodTable(); + } + } +#endif + + RETURN; +} + //******************************************************************************* // This is the routine that computes the internal type of a given type. It normalizes // structs that have only one field (of int/ptr sized values), to be that underlying type. diff --git a/src/vm/clsload.cpp b/src/vm/clsload.cpp index 59c5de369304..ffd165ebe9ba 100644 --- a/src/vm/clsload.cpp +++ b/src/vm/clsload.cpp @@ -3219,6 +3219,7 @@ TypeHandle ClassLoader::DoIncrementalLoad(TypeKey *pTypeKey, TypeHandle typeHnd, if (!typeHnd.IsTypeDesc()) { LoadExactParents(typeHnd.AsMethodTable()); + RecordDependenciesForDictionaryExpansion(typeHnd.AsMethodTable()); } break; @@ -3268,14 +3269,13 @@ TypeHandle ClassLoader::CreateTypeHandleForTypeKey(TypeKey* pKey, AllocMemTracke if (IsCanonicalGenericInstantiation(pKey->GetInstantiation())) { typeHnd = CreateTypeHandleForTypeDefThrowing(pKey->GetModule(), - pKey->GetTypeToken(), - pKey->GetInstantiation(), - pamTracker); + pKey->GetTypeToken(), + pKey->GetInstantiation(), + pamTracker); } else { - typeHnd = CreateTypeHandleForNonCanonicalGenericInstantiation(pKey, - pamTracker); + typeHnd = CreateTypeHandleForNonCanonicalGenericInstantiation(pKey, pamTracker); } #if defined(_DEBUG) && !defined(CROSSGEN_COMPILE) if (Nullable::IsNullableType(typeHnd)) diff --git a/src/vm/clsload.hpp b/src/vm/clsload.hpp index 79f406c3e3b2..b62945f8c262 100644 --- a/src/vm/clsload.hpp +++ b/src/vm/clsload.hpp @@ -1007,6 +1007,8 @@ class ClassLoader // Load exact parents and interfaces and dependent structures (generics dictionary, vtable fixes) static void LoadExactParents(MethodTable *pMT); + static void RecordDependenciesForDictionaryExpansion(MethodTable* pMT); + static void LoadExactParentAndInterfacesTransitively(MethodTable *pMT); // Create a non-canonical instantiation of a generic type based off the canonical instantiation diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index e573988c7982..e74f3e5b7f06 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -63,9 +63,6 @@ DictionaryLayout::Allocate( DictionaryLayout * pD = (DictionaryLayout *)(void *)ptr; - // When bucket spills we'll allocate another layout structure - pD->m_pNext = NULL; - // This is the number of slots excluding the type parameters pD->m_numSlots = numSlots; @@ -109,183 +106,286 @@ DictionaryLayout::GetFirstDictionaryBucketSize( // Optimize the case of a token being !i (for class dictionaries) or !!i (for method dictionaries) // /* static */ -BOOL -DictionaryLayout::FindTokenWorker(LoaderAllocator * pAllocator, - DWORD numGenericArgs, - DictionaryLayout * pDictLayout, - CORINFO_RUNTIME_LOOKUP * pResult, - SigBuilder * pSigBuilder, - BYTE * pSig, - DWORD cbSig, - int nFirstOffset, - DictionaryEntrySignatureSource signatureSource, - WORD * pSlotOut) +BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocator, + DWORD numGenericArgs, + DictionaryLayout* pDictLayout, + SigBuilder* pSigBuilder, + BYTE* pSig, + DWORD cbSig, + int nFirstOffset, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult, + BOOL useEmptySlotIfFound /* = FALSE */) + { CONTRACTL { STANDARD_VM_CHECK; PRECONDITION(numGenericArgs > 0); PRECONDITION(CheckPointer(pDictLayout)); - PRECONDITION(CheckPointer(pSlotOut)); + PRECONDITION(CheckPointer(pResult)); PRECONDITION(CheckPointer(pSig)); PRECONDITION((pSigBuilder == NULL && cbSig == -1) || (CheckPointer(pSigBuilder) && cbSig > 0)); } CONTRACTL_END -#ifndef FEATURE_NATIVE_IMAGE_GENERATION - // If the tiered compilation is on, save the fast dictionary slots for the hot Tier1 code - if (g_pConfig->TieredCompilation() && signatureSource == FromReadyToRunImage) - { - pResult->signature = pSig; - return FALSE; - } -#endif - - BOOL isFirstBucket = TRUE; - - // First bucket also contains type parameters + // First slots contain the type parameters _ASSERTE(FitsIn(numGenericArgs)); WORD slot = static_cast(numGenericArgs); - for (;;) + + for (DWORD iSlot = 0; iSlot < pDictLayout->m_numSlots; iSlot++) { - for (DWORD iSlot = 0; iSlot < pDictLayout->m_numSlots; iSlot++) + BYTE* pCandidate = (BYTE*)pDictLayout->m_slots[iSlot].m_signature; + if (pCandidate != NULL) { - RetryMatch: - BYTE * pCandidate = (BYTE *)pDictLayout->m_slots[iSlot].m_signature; - if (pCandidate != NULL) - { - bool signaturesMatch = false; + bool signaturesMatch = false; - if (pSigBuilder != NULL) - { - // JIT case: compare signatures by comparing the bytes in them. We exclude - // any ReadyToRun signatures from the JIT case. - - if (pDictLayout->m_slots[iSlot].m_signatureSource != FromReadyToRunImage) - { - // Compare the signatures. We do not need to worry about the size of pCandidate. - // As long as we are comparing one byte at a time we are guaranteed to not overrun. - DWORD j; - for (j = 0; j < cbSig; j++) - { - if (pCandidate[j] != pSig[j]) - break; - } - signaturesMatch = (j == cbSig); - } - } - else - { - // ReadyToRun case: compare signatures by comparing their pointer values - signaturesMatch = (pCandidate == pSig); - } + if (pSigBuilder != NULL) + { + // JIT case: compare signatures by comparing the bytes in them. We exclude + // any ReadyToRun signatures from the JIT case. - // We've found it - if (signaturesMatch) + if (pDictLayout->m_slots[iSlot].m_signatureSource != FromReadyToRunImage) { - pResult->signature = pDictLayout->m_slots[iSlot].m_signature; - - // We don't store entries outside the first bucket in the layout in the dictionary (they'll be cached in a hash - // instead). - if (!isFirstBucket) + // Compare the signatures. We do not need to worry about the size of pCandidate. + // As long as we are comparing one byte at a time we are guaranteed to not overrun. + DWORD j; + for (j = 0; j < cbSig; j++) { - return FALSE; + if (pCandidate[j] != pSig[j]) + break; } - _ASSERTE(FitsIn(nFirstOffset + 1)); - pResult->indirections = static_cast(nFirstOffset + 1); - pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); - *pSlotOut = slot; - return TRUE; + signaturesMatch = (j == cbSig); } } - // If we hit an empty slot then there's no more so use it else { - { - BaseDomain::LockHolder lh(pAllocator->GetDomain()); - - if (pDictLayout->m_slots[iSlot].m_signature != NULL) - goto RetryMatch; - - PVOID pResultSignature = pSig; - - if (pSigBuilder != NULL) - { - pSigBuilder->AppendData(isFirstBucket ? slot : 0); - - DWORD cbNewSig; - PVOID pNewSig = pSigBuilder->GetSignature(&cbNewSig); - - pResultSignature = pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(cbNewSig)); - memcpy(pResultSignature, pNewSig, cbNewSig); - } - - pDictLayout->m_slots[iSlot].m_signature = pResultSignature; - pDictLayout->m_slots[iSlot].m_signatureSource = signatureSource; - } + // ReadyToRun case: compare signatures by comparing their pointer values + signaturesMatch = (pCandidate == pSig); + } + // We've found it + if (signaturesMatch) + { pResult->signature = pDictLayout->m_slots[iSlot].m_signature; - // Again, we only store entries in the first layout bucket in the dictionary. - if (!isFirstBucket) - { - return FALSE; - } _ASSERTE(FitsIn(nFirstOffset + 1)); pResult->indirections = static_cast(nFirstOffset + 1); pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); - *pSlotOut = slot; + pResult->slot = slot; return TRUE; } - slot++; } + // If we hit an empty slot then there's no more so use it + else + { + if (!useEmptySlotIfFound) + return FALSE; - // If we've reached the end of the chain we need to allocate another bucket. Make the pointer update carefully to avoid - // orphaning a bucket in a race. We leak the loser in such a race (since the allocation comes from the loader heap) but both - // the race and the overflow should be very rare. - if (pDictLayout->m_pNext == NULL) - FastInterlockCompareExchangePointer(&pDictLayout->m_pNext, Allocate(4, pAllocator, NULL), 0); + // A lock should be taken by FindToken before being allowed to use an empty slot in the layout + _ASSERT(SystemDomain::IsUnderDomainLock()); - pDictLayout = pDictLayout->m_pNext; - isFirstBucket = FALSE; + PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); + *EnsureWritablePages(&(pDictLayout->m_slots[iSlot].m_signature)) = pResultSignature; + *EnsureWritablePages(&(pDictLayout->m_slots[iSlot].m_signatureSource)) = signatureSource; + + pResult->signature = pDictLayout->m_slots[iSlot].m_signature; + + _ASSERTE(FitsIn(nFirstOffset + 1)); + pResult->indirections = static_cast(nFirstOffset + 1); + pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); + pResult->slot = slot; + return TRUE; + } + + slot++; } -} // DictionaryLayout::FindToken + return FALSE; +} + +#ifndef CROSSGEN_COMPILE /* static */ -BOOL -DictionaryLayout::FindToken(LoaderAllocator * pAllocator, - DWORD numGenericArgs, - DictionaryLayout * pDictLayout, - CORINFO_RUNTIME_LOOKUP * pResult, - SigBuilder * pSigBuilder, - int nFirstOffset, - DictionaryEntrySignatureSource signatureSource) +DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* pAllocator, + DictionaryLayout* pCurrentDictLayout, + DWORD numGenericArgs, + SigBuilder* pSigBuilder, + BYTE* pSig, + int nFirstOffset, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult) { - WRAPPER_NO_CONTRACT; + CONTRACTL + { + STANDARD_VM_CHECK; + INJECT_FAULT(ThrowOutOfMemory();); + PRECONDITION(SystemDomain::IsUnderDomainLock()); + } + CONTRACTL_END + + // There shouldn't be any empty slots remaining in the current dictionary. + _ASSERTE(pCurrentDictLayout->m_slots[pCurrentDictLayout->m_numSlots - 1].m_signature != NULL); + + // Expand the dictionary layout to add more slots + _ASSERTE(FitsIn(pCurrentDictLayout->m_numSlots + NUM_DICTIONARY_SLOTS)); + DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots + NUM_DICTIONARY_SLOTS, pAllocator, NULL); + + for (DWORD iSlot = 0; iSlot < pCurrentDictLayout->m_numSlots; iSlot++) + pNewDictionaryLayout->m_slots[iSlot] = pCurrentDictLayout->m_slots[iSlot]; - DWORD cbSig; - BYTE * pSig = (BYTE *)pSigBuilder->GetSignature(&cbSig); + WORD layoutSlotIndex = pCurrentDictLayout->m_numSlots; + WORD slot = static_cast(numGenericArgs) + layoutSlotIndex; - WORD slotDummy; - return FindTokenWorker(pAllocator, numGenericArgs, pDictLayout, pResult, pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, &slotDummy); + PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); + *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature)) = pResultSignature; + *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signatureSource)) = signatureSource; + + pResult->signature = pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature; + + _ASSERTE(FitsIn(nFirstOffset + 1)); + pResult->indirections = static_cast(nFirstOffset + 1); + pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); + pResult->slot = slot; + + return pNewDictionaryLayout; } +#endif /* static */ -BOOL -DictionaryLayout::FindToken(LoaderAllocator * pAllocator, - DWORD numGenericArgs, - DictionaryLayout * pDictLayout, - CORINFO_RUNTIME_LOOKUP * pResult, - BYTE * signature, - int nFirstOffset, - DictionaryEntrySignatureSource signatureSource, - WORD * pSlotOut) +BOOL DictionaryLayout::FindToken(MethodTable* pMT, + LoaderAllocator* pAllocator, + int nFirstOffset, + SigBuilder* pSigBuilder, + BYTE* pSig, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult) +{ + CONTRACTL + { + STANDARD_VM_CHECK; + PRECONDITION(CheckPointer(pMT)); + PRECONDITION(CheckPointer(pAllocator)); + PRECONDITION(CheckPointer(pResult)); + PRECONDITION(pMT->HasInstantiation()); + } + CONTRACTL_END + + DWORD cbSig = -1; + pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; + if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult)) + return TRUE; + + SystemDomain::LockHolder lh; + { + // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot + if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, TRUE)) + return TRUE; + + +#ifndef CROSSGEN_COMPILE + DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout(); + DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMT->GetNumGenericArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult); + if (pNewLayout == NULL) + { + pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); + return FALSE; + } + + // First, expand the PerInstInfo dictionaries on types that were using the dictionary layout that just got expanded, and expand their slots + pMT->GetModule()->ExpandTypeDictionaries_Locked(pMT, pOldLayout, pNewLayout); + + // Finally, update the dictionary layout pointer after all dictionaries of instantiated types have expanded, so that subsequent calls to + // DictionaryLayout::FindToken can use this. It is important to update the dictionary layout at the very last step, otherwise some other threads + // can start using newly added dictionary layout slots on types where the PerInstInfo hasn't expanded yet, and cause runtime failures. + pMT->GetClass()->SetDictionaryLayout(pNewLayout); + + return TRUE; +#else + pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); + return FALSE; +#endif + } +} + +/* static */ +BOOL DictionaryLayout::FindToken(MethodDesc* pMD, + LoaderAllocator* pAllocator, + int nFirstOffset, + SigBuilder* pSigBuilder, + BYTE* pSig, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult) { - WRAPPER_NO_CONTRACT; + CONTRACTL + { + STANDARD_VM_CHECK; + PRECONDITION(CheckPointer(pMD)); + PRECONDITION(CheckPointer(pAllocator)); + PRECONDITION(CheckPointer(pResult)); + PRECONDITION(pMD->HasMethodInstantiation()); + } + CONTRACTL_END + + DWORD cbSig = -1; + pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult)) + return TRUE; + + SystemDomain::LockHolder lh; + { + // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, TRUE)) + return TRUE; - return FindTokenWorker(pAllocator, numGenericArgs, pDictLayout, pResult, NULL, signature, -1, nFirstOffset, signatureSource, pSlotOut); +#ifndef CROSSGEN_COMPILE + DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout(); + DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMD->GetNumGenericMethodArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult); + if (pNewLayout == NULL) + { + pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); + return FALSE; + } + + // First, expand the PerInstInfo dictionaries on methods that were using the dictionary layout that just got expanded, and expand their slots + pMD->GetModule()->ExpandMethodDictionaries_Locked(pMD, pOldLayout, pNewLayout); + + // Finally, update the dictionary layout pointer after all dictionaries of instantiated methods have expanded, so that subsequent calls to + // DictionaryLayout::FindToken can use this. It is important to update the dictionary layout at the very last step, otherwise some other threads + // can start using newly added dictionary layout slots on methods where the PerInstInfo hasn't expanded yet, and cause runtime failures. + pMD->AsInstantiatedMethodDesc()->IMD_SetDictionaryLayout(pNewLayout); + + return TRUE; +#else + pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); + return FALSE; +#endif + } } +/* static */ +PVOID DictionaryLayout::CreateSignatureWithSlotData(SigBuilder* pSigBuilder, LoaderAllocator* pAllocator, WORD slot) +{ + CONTRACTL + { + STANDARD_VM_CHECK; + PRECONDITION(CheckPointer(pSigBuilder) && CheckPointer(pAllocator)); + } + CONTRACTL_END + + pSigBuilder->AppendData(slot); + + DWORD cbNewSig; + PVOID pNewSig = pSigBuilder->GetSignature(&cbNewSig); + + PVOID pResultSignature = pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(cbNewSig)); + _ASSERT(pResultSignature != NULL); + + memcpy(pResultSignature, pNewSig, cbNewSig); + + return pResultSignature; +} + + #endif //!DACCESS_COMPILE //--------------------------------------------------------------------------------------- @@ -347,21 +447,13 @@ DictionaryLayout::GetObjectSize() //--------------------------------------------------------------------------------------- // // Save the dictionary layout for prejitting -// -void -DictionaryLayout::Save( - DataImage * image) +// +void +DictionaryLayout::Save(DataImage * image) { STANDARD_VM_CONTRACT; - DictionaryLayout *pDictLayout = this; - - while (pDictLayout) - { - image->StoreStructure(pDictLayout, pDictLayout->GetObjectSize(), DataImage::ITEM_DICTIONARY_LAYOUT); - pDictLayout = pDictLayout->m_pNext; - } - + image->StoreStructure(this, GetObjectSize(), DataImage::ITEM_DICTIONARY_LAYOUT); } //--------------------------------------------------------------------------------------- @@ -378,15 +470,10 @@ DictionaryLayout::Trim() } CONTRACTL_END; - // Only the last bucket in the chain may have unused entries - DictionaryLayout *pDictLayout = this; - while (pDictLayout->m_pNext) - pDictLayout = pDictLayout->m_pNext; - // Trim down the size to what's actually used - DWORD dwSlots = pDictLayout->GetNumUsedSlots(); + DWORD dwSlots = GetNumUsedSlots(); _ASSERTE(FitsIn(dwSlots)); - pDictLayout->m_numSlots = static_cast(dwSlots); + *EnsureWritablePages(&m_numSlots) = static_cast(dwSlots); } @@ -401,21 +488,14 @@ DictionaryLayout::Fixup( { STANDARD_VM_CONTRACT; - DictionaryLayout *pDictLayout = this; - - while (pDictLayout) + for (DWORD i = 0; i < m_numSlots; i++) { - for (DWORD i = 0; i < pDictLayout->m_numSlots; i++) + PVOID signature = m_slots[i].m_signature; + if (signature != NULL) { - PVOID signature = pDictLayout->m_slots[i].m_signature; - if (signature != NULL) - { - image->FixupFieldToNode(pDictLayout, (BYTE *)&pDictLayout->m_slots[i].m_signature - (BYTE *)pDictLayout, - image->GetGenericSignature(signature, fMethod)); - } + image->FixupFieldToNode(this, (BYTE *)&m_slots[i].m_signature - (BYTE *)this, + image->GetGenericSignature(signature, fMethod)); } - image->FixupPointerField(pDictLayout, offsetof(DictionaryLayout, m_pNext)); - pDictLayout = pDictLayout->m_pNext; } } @@ -658,7 +738,6 @@ Dictionary::PopulateEntry( } CONTRACTL_END; CORINFO_GENERIC_HANDLE result = NULL; - Dictionary * pDictionary = NULL; *ppSlot = NULL; bool isReadyToRunModule = (pModule != NULL && pModule->IsReadyToRun()); @@ -749,7 +828,6 @@ Dictionary::PopulateEntry( // prepare for every possible derived type of the type containing the method). So instead we have to locate the exactly // instantiated (non-shared) super-type of the class passed in. - pDictionary = pMT->GetDictionary(); ULONG dictionaryIndex = 0; @@ -761,13 +839,15 @@ Dictionary::PopulateEntry( { IfFailThrow(ptr.GetData(&dictionaryIndex)); } + +#if _DEBUG + // Lock is needed because dictionary pointers can get updated during dictionary size expansion + SystemDomain::LockHolder lh; // MethodTable is expected to be normalized + Dictionary* pDictionary = pMT->GetDictionary(); _ASSERTE(pDictionary == pMT->GetPerInstInfo()[dictionaryIndex].GetValueMaybeNull()); - } - else - { - pDictionary = pMD->GetMethodDictionary(); +#endif } { @@ -1245,7 +1325,12 @@ Dictionary::PopulateEntry( if ((slotIndex != 0) && !IsCompilationProcess()) { - *(pDictionary->GetSlotAddr(0, slotIndex)) = result; + // Lock is needed because dictionary pointers can get updated during dictionary size expansion + SystemDomain::LockHolder lh; + + Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary() : pMD->GetMethodDictionary(); + + *EnsureWritablePages(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); } } diff --git a/src/vm/genericdict.h b/src/vm/genericdict.h index 44993886bbdf..cf5b327be390 100644 --- a/src/vm/genericdict.h +++ b/src/vm/genericdict.h @@ -92,6 +92,10 @@ typedef DPTR(DictionaryEntryLayout) PTR_DictionaryEntryLayout; class DictionaryLayout; typedef DPTR(DictionaryLayout) PTR_DictionaryLayout; +// Number of slots to initially allocate in a generic method dictionary layout. +// Also the number of additional slots to add to an existing dictionary layout when expanding its size +#define NUM_DICTIONARY_SLOTS 8 + // The type of dictionary layouts. We don't include the number of type // arguments as this is obtained elsewhere class DictionaryLayout @@ -101,26 +105,34 @@ class DictionaryLayout friend class NativeImageDumper; #endif private: - // Next bucket of slots (only used to track entries that won't fit in the dictionary) - DictionaryLayout* m_pNext; - // Number of non-type-argument slots in this bucket WORD m_numSlots; // m_numSlots of these DictionaryEntryLayout m_slots[1]; - static BOOL FindTokenWorker(LoaderAllocator *pAllocator, - DWORD numGenericArgs, - DictionaryLayout *pDictLayout, - CORINFO_RUNTIME_LOOKUP *pResult, - SigBuilder * pSigBuilder, - BYTE * pSig, - DWORD cbSig, - int nFirstOffset, - DictionaryEntrySignatureSource signatureSource, - WORD * pSlotOut); - + static BOOL FindTokenWorker(LoaderAllocator* pAllocator, + DWORD numGenericArgs, + DictionaryLayout* pDictLayout, + SigBuilder* pSigBuilder, + BYTE* pSig, + DWORD cbSig, + int nFirstOffset, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult, + BOOL useEmptySlotIfFound = FALSE); + + + static DictionaryLayout* ExpandDictionaryLayout(LoaderAllocator* pAllocator, + DictionaryLayout* pCurrentDictLayout, + DWORD numGenericArgs, + SigBuilder* pSigBuilder, + BYTE* pSig, + int nFirstOffset, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult); + + static PVOID CreateSignatureWithSlotData(SigBuilder* pSigBuilder, LoaderAllocator* pAllocator, WORD slot); public: // Create an initial dictionary layout with a single bucket containing numSlots slots @@ -130,22 +142,21 @@ class DictionaryLayout // another structure (e.g. MethodTable) static DWORD GetFirstDictionaryBucketSize(DWORD numGenericArgs, PTR_DictionaryLayout pDictLayout); - static BOOL FindToken(LoaderAllocator *pAllocator, - DWORD numGenericArgs, - DictionaryLayout *pDictLayout, - CORINFO_RUNTIME_LOOKUP *pResult, - SigBuilder * pSigBuilder, - int nFirstOffset, - DictionaryEntrySignatureSource signatureSource); - - static BOOL FindToken(LoaderAllocator * pAllocator, - DWORD numGenericArgs, - DictionaryLayout * pDictLayout, - CORINFO_RUNTIME_LOOKUP * pResult, - BYTE * signature, - int nFirstOffset, - DictionaryEntrySignatureSource signatureSource, - WORD * pSlotOut); + static BOOL FindToken(MethodTable* pMT, + LoaderAllocator* pAllocator, + int nFirstOffset, + SigBuilder* pSigBuilder, + BYTE* pSig, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult); + + static BOOL FindToken(MethodDesc* pMD, + LoaderAllocator* pAllocator, + int nFirstOffset, + SigBuilder* pSigBuilder, + BYTE* pSig, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult); DWORD GetMaxSlots(); DWORD GetNumUsedSlots(); @@ -158,7 +169,6 @@ class DictionaryLayout dac_cast(this) + offsetof(DictionaryLayout, m_slots) + sizeof(DictionaryEntryLayout) * i); } - DictionaryLayout* GetNextLayout() { LIMITED_METHOD_CONTRACT; return m_pNext; } #ifdef FEATURE_PREJIT DWORD GetObjectSize(); diff --git a/src/vm/genmeth.cpp b/src/vm/genmeth.cpp index 3eb3ac1bebbb..41f567720658 100644 --- a/src/vm/genmeth.cpp +++ b/src/vm/genmeth.cpp @@ -63,7 +63,6 @@ // should be the normalized representative genericMethodArgs (see typehandle.h) // - // Helper method that creates a method-desc off a template method desc static MethodDesc* CreateMethodDesc(LoaderAllocator *pAllocator, MethodTable *pMT, @@ -378,9 +377,8 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, } else if (getWrappedCode) { - // 4 seems like a good number - pDL = DictionaryLayout::Allocate(4, pAllocator, &amt); -#ifdef _DEBUG + pDL = DictionaryLayout::Allocate(NUM_DICTIONARY_SLOTS, pAllocator, &amt); +#ifdef _DEBUG { SString name; TypeString::AppendMethodDebug(name, pGenericMDescInRepMT); @@ -1186,6 +1184,14 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pWrappedMD, methodInst, FALSE); + +#ifndef CROSSGEN_COMPILE + if (pInstMD->HasMethodInstantiation()) + { + SystemDomain::LockHolder lh; + pInstMD->GetModule()->RecordMethodForDictionaryExpansion_Locked(pInstMD); + } +#endif } } else diff --git a/src/vm/interoputil.cpp b/src/vm/interoputil.cpp index 3981bca6c567..f07adc92dada 100644 --- a/src/vm/interoputil.cpp +++ b/src/vm/interoputil.cpp @@ -5833,7 +5833,7 @@ MethodDesc *WinRTInterfaceRedirector::GetStubMethodForRedirectedInterface(WinMDA if (interfaceIndex == WinMDAdapter::RedirectedTypeIndex_System_Collections_Generic_IDictionary || interfaceIndex == WinMDAdapter::RedirectedTypeIndex_System_Collections_Generic_IReadOnlyDictionary) - { + { thKvPair = TypeHandle(MscorlibBinder::GetClass(CLASS__KEYVALUEPAIRGENERIC)).Instantiate(inst); inst = Instantiation(&thKvPair, 1); } diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index b09e7d2bff49..ee47e49a77c8 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -3494,7 +3494,7 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr _ASSERTE(pContextMD != NULL); _ASSERTE(pContextMD->HasMethodInstantiation()); - if (DictionaryLayout::FindToken(pContextMD->GetLoaderAllocator(), pContextMD->GetNumGenericMethodArgs(), pContextMD->GetDictionaryLayout(), pResult, &sigBuilder, 1, signatureSource)) + if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult)) { pResult->testForNull = 1; pResult->testForFixup = 0; @@ -3512,7 +3512,7 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr // It's a class dictionary lookup (CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ) else { - if (DictionaryLayout::FindToken(pContextMT->GetLoaderAllocator(), pContextMT->GetNumGenericArgs(), pContextMT->GetClass()->GetDictionaryLayout(), pResult, &sigBuilder, 2, signatureSource)) + if (DictionaryLayout::FindToken(pContextMT, pContextMT->GetLoaderAllocator(), 2, &sigBuilder, NULL, signatureSource, pResult)) { pResult->testForNull = 1; pResult->testForFixup = 0; diff --git a/src/vm/method.cpp b/src/vm/method.cpp index b36c78c5422f..3c639705a1ad 100644 --- a/src/vm/method.cpp +++ b/src/vm/method.cpp @@ -1594,7 +1594,9 @@ MethodDesc *MethodDesc::GetWrappedMethodDesc() this->GetMethodTable(), FALSE, /* no unboxing entrypoint */ this->GetMethodInstantiation(), - TRUE /* get shared code */ ); + TRUE, /* get shared code */ + FALSE /* forceRemotableMethod */, + FALSE /* allowCreate */); _ASSERTE(pAltMD == pRet); #endif // _DEBUG return pRet; diff --git a/src/vm/method.hpp b/src/vm/method.hpp index 377b8996667e..f78436ee08c9 100644 --- a/src/vm/method.hpp +++ b/src/vm/method.hpp @@ -3574,12 +3574,25 @@ class InstantiatedMethodDesc : public MethodDesc InstantiatedMethodDesc* pIMD = IMD_GetWrappedMethodDesc()->AsInstantiatedMethodDesc(); return pIMD->m_pDictLayout.GetValueMaybeNull(); } - else - if (IMD_IsSharedByGenericMethodInstantiations()) + else if (IMD_IsSharedByGenericMethodInstantiations()) return m_pDictLayout.GetValueMaybeNull(); else return NULL; } + + void IMD_SetDictionaryLayout(DictionaryLayout* pNewLayout) + { + WRAPPER_NO_CONTRACT; + if (IMD_IsWrapperStubWithInstantiations() && IMD_HasMethodInstantiation()) + { + InstantiatedMethodDesc* pIMD = IMD_GetWrappedMethodDesc()->AsInstantiatedMethodDesc(); + pIMD->m_pDictLayout.SetValueMaybeNull(pNewLayout); + } + else if (IMD_IsSharedByGenericMethodInstantiations()) + { + m_pDictLayout.SetValueMaybeNull(pNewLayout); + } + } #endif // !DACCESS_COMPILE // Setup the IMD as shared code diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp index f18d9593b24a..e562f768d1cc 100644 --- a/src/vm/prestub.cpp +++ b/src/vm/prestub.cpp @@ -2886,12 +2886,10 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock // are used for the dictionary index, and the lower 16 bits for the slot number. *pDictionaryIndexAndSlot = (pContextMT == NULL ? 0 : pContextMT->GetNumDicts() - 1); *pDictionaryIndexAndSlot <<= 16; - - WORD dictionarySlot; - + if (kind == ENCODE_DICTIONARY_LOOKUP_METHOD) { - if (DictionaryLayout::FindToken(pModule->GetLoaderAllocator(), numGenericArgs, pContextMD->GetDictionaryLayout(), pResult, (BYTE*)pBlobStart, 1, FromReadyToRunImage, &dictionarySlot)) + if (DictionaryLayout::FindToken(pContextMD, pModule->GetLoaderAllocator(), 1, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult)) { pResult->testForNull = 1; @@ -2903,14 +2901,14 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock pResult->indirectFirstOffset = 1; } - *pDictionaryIndexAndSlot |= dictionarySlot; + *pDictionaryIndexAndSlot |= pResult->slot; } } // It's a class dictionary lookup (CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ) else { - if (DictionaryLayout::FindToken(pModule->GetLoaderAllocator(), numGenericArgs, pContextMT->GetClass()->GetDictionaryLayout(), pResult, (BYTE*)pBlobStart, 2, FromReadyToRunImage, &dictionarySlot)) + if (DictionaryLayout::FindToken(pContextMT, pModule->GetLoaderAllocator(), 2, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult)) { pResult->testForNull = 1; @@ -2926,7 +2924,7 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock pResult->indirectSecondOffset = 1; } - *pDictionaryIndexAndSlot |= dictionarySlot; + *pDictionaryIndexAndSlot |= pResult->slot; } } } From 4e0d59b84a9b16f8564213abb9d5a87e9851f87c Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 19 Aug 2019 14:22:20 -0700 Subject: [PATCH 02/21] Fix build break on mac/linux --- src/vm/ceeload.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index 0964a846d561..00cfb91410bf 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -13314,7 +13314,7 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, expandPerInstInfos); - auto updateDependentDicts = [pCanonMT](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) + auto updateDependentDicts = [](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) { if (pMTToUpdate->HasSameTypeDefAs(pMTKey)) return true; From bf7cdbf69d31270900fa535276236588784cdaf0 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 21 Aug 2019 11:15:12 -0700 Subject: [PATCH 03/21] Remove slot from CORINFO_RUNTIME_LOOKUP. Fix linux build break on release builds --- src/inc/corinfo.h | 3 --- src/vm/ceeload.cpp | 8 ++++---- src/vm/genericdict.cpp | 31 ++++++++++++++++++------------- src/vm/genericdict.h | 10 +++++++--- src/vm/jitinterface.cpp | 6 ++++-- src/vm/prestub.cpp | 10 ++++++---- 6 files changed, 39 insertions(+), 29 deletions(-) diff --git a/src/inc/corinfo.h b/src/inc/corinfo.h index ded06501c174..0fcdac29920f 100644 --- a/src/inc/corinfo.h +++ b/src/inc/corinfo.h @@ -1373,9 +1373,6 @@ struct CORINFO_RUNTIME_LOOKUP // 1 means that value stored at second offset (offsets[1]) from pointer is offset2, and the next pointer is // stored at pointer+offsets[1]+offset2. bool indirectSecondOffset; - - // Dictionary slot index result for a dictionary lookup - WORD slot; } ; // Result of calling embedGenericHandle diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index 00cfb91410bf..a25e253da101 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -13294,12 +13294,12 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p // allocated one. // - auto expandPerInstInfos = [oldInfoSize, newInfoSize, pCanonMT](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) + auto expandPerInstInfos = [oldInfoSize, newInfoSize](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) { if (!pMTToUpdate->HasSameTypeDefAs(pMTKey)) return true; - _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey && pMTKey == pCanonMT); + _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey); TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMTToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); ZeroMemory(pInstOrPerInstInfo, newInfoSize); @@ -13358,10 +13358,10 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* DWORD oldInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pOldLayout); DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pNewLayout); - auto lambda = [oldInfoSize, newInfoSize, pCanonMD](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) + auto lambda = [oldInfoSize, newInfoSize](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) { // Update m_pPerInstInfo for the pMethodDesc being visited here - _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetWrappedMethodDesc() == pMDKey && pMDKey == pCanonMD); + _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetWrappedMethodDesc() == pMDKey); InstantiatedMethodDesc* pInstantiatedMD = pMDToUpdate->AsInstantiatedMethodDesc(); diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index e74f3e5b7f06..a8699401994b 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -115,6 +115,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat int nFirstOffset, DictionaryEntrySignatureSource signatureSource, CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut, BOOL useEmptySlotIfFound /* = FALSE */) { @@ -123,7 +124,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat STANDARD_VM_CHECK; PRECONDITION(numGenericArgs > 0); PRECONDITION(CheckPointer(pDictLayout)); - PRECONDITION(CheckPointer(pResult)); + PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); PRECONDITION(CheckPointer(pSig)); PRECONDITION((pSigBuilder == NULL && cbSig == -1) || (CheckPointer(pSigBuilder) && cbSig > 0)); } @@ -172,7 +173,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat _ASSERTE(FitsIn(nFirstOffset + 1)); pResult->indirections = static_cast(nFirstOffset + 1); pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); - pResult->slot = slot; + *pSlotOut = slot; return TRUE; } } @@ -194,7 +195,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat _ASSERTE(FitsIn(nFirstOffset + 1)); pResult->indirections = static_cast(nFirstOffset + 1); pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); - pResult->slot = slot; + *pSlotOut = slot; return TRUE; } @@ -213,13 +214,15 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* BYTE* pSig, int nFirstOffset, DictionaryEntrySignatureSource signatureSource, - CORINFO_RUNTIME_LOOKUP* pResult) + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut) { CONTRACTL { STANDARD_VM_CHECK; INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(SystemDomain::IsUnderDomainLock()); + PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); } CONTRACTL_END @@ -245,7 +248,7 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* _ASSERTE(FitsIn(nFirstOffset + 1)); pResult->indirections = static_cast(nFirstOffset + 1); pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); - pResult->slot = slot; + *pSlotOut = slot; return pNewDictionaryLayout; } @@ -258,7 +261,8 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, SigBuilder* pSigBuilder, BYTE* pSig, DictionaryEntrySignatureSource signatureSource, - CORINFO_RUNTIME_LOOKUP* pResult) + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut) { CONTRACTL { @@ -272,19 +276,19 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult)) + if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut)) return TRUE; SystemDomain::LockHolder lh; { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, TRUE)) + if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout(); - DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMT->GetNumGenericArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult); + DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMT->GetNumGenericArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult, pSlotOut); if (pNewLayout == NULL) { pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); @@ -314,7 +318,8 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, SigBuilder* pSigBuilder, BYTE* pSig, DictionaryEntrySignatureSource signatureSource, - CORINFO_RUNTIME_LOOKUP* pResult) + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut) { CONTRACTL { @@ -328,18 +333,18 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut)) return TRUE; SystemDomain::LockHolder lh; { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, TRUE)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout(); - DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMD->GetNumGenericMethodArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult); + DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMD->GetNumGenericMethodArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult, pSlotOut); if (pNewLayout == NULL) { pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); diff --git a/src/vm/genericdict.h b/src/vm/genericdict.h index cf5b327be390..a906bb3e25af 100644 --- a/src/vm/genericdict.h +++ b/src/vm/genericdict.h @@ -120,6 +120,7 @@ class DictionaryLayout int nFirstOffset, DictionaryEntrySignatureSource signatureSource, CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut, BOOL useEmptySlotIfFound = FALSE); @@ -130,7 +131,8 @@ class DictionaryLayout BYTE* pSig, int nFirstOffset, DictionaryEntrySignatureSource signatureSource, - CORINFO_RUNTIME_LOOKUP* pResult); + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut); static PVOID CreateSignatureWithSlotData(SigBuilder* pSigBuilder, LoaderAllocator* pAllocator, WORD slot); @@ -148,7 +150,8 @@ class DictionaryLayout SigBuilder* pSigBuilder, BYTE* pSig, DictionaryEntrySignatureSource signatureSource, - CORINFO_RUNTIME_LOOKUP* pResult); + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut); static BOOL FindToken(MethodDesc* pMD, LoaderAllocator* pAllocator, @@ -156,7 +159,8 @@ class DictionaryLayout SigBuilder* pSigBuilder, BYTE* pSig, DictionaryEntrySignatureSource signatureSource, - CORINFO_RUNTIME_LOOKUP* pResult); + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut); DWORD GetMaxSlots(); DWORD GetNumUsedSlots(); diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index ee47e49a77c8..0d1d12244529 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -3488,13 +3488,15 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr DictionaryEntrySignatureSource signatureSource = (IsCompilationProcess() ? FromZapImage : FromJIT); + WORD dummySlot; + // It's a method dictionary lookup if (pResultLookup->lookupKind.runtimeLookupKind == CORINFO_LOOKUP_METHODPARAM) { _ASSERTE(pContextMD != NULL); _ASSERTE(pContextMD->HasMethodInstantiation()); - if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult)) + if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult, &dummySlot)) { pResult->testForNull = 1; pResult->testForFixup = 0; @@ -3512,7 +3514,7 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr // It's a class dictionary lookup (CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ) else { - if (DictionaryLayout::FindToken(pContextMT, pContextMT->GetLoaderAllocator(), 2, &sigBuilder, NULL, signatureSource, pResult)) + if (DictionaryLayout::FindToken(pContextMT, pContextMT->GetLoaderAllocator(), 2, &sigBuilder, NULL, signatureSource, pResult, &dummySlot)) { pResult->testForNull = 1; pResult->testForFixup = 0; diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp index e562f768d1cc..6e4ce2cf2acc 100644 --- a/src/vm/prestub.cpp +++ b/src/vm/prestub.cpp @@ -2887,9 +2887,11 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock *pDictionaryIndexAndSlot = (pContextMT == NULL ? 0 : pContextMT->GetNumDicts() - 1); *pDictionaryIndexAndSlot <<= 16; + WORD dictionarySlot; + if (kind == ENCODE_DICTIONARY_LOOKUP_METHOD) { - if (DictionaryLayout::FindToken(pContextMD, pModule->GetLoaderAllocator(), 1, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult)) + if (DictionaryLayout::FindToken(pContextMD, pModule->GetLoaderAllocator(), 1, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult, &dictionarySlot)) { pResult->testForNull = 1; @@ -2901,14 +2903,14 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock pResult->indirectFirstOffset = 1; } - *pDictionaryIndexAndSlot |= pResult->slot; + *pDictionaryIndexAndSlot |= dictionarySlot; } } // It's a class dictionary lookup (CORINFO_LOOKUP_CLASSPARAM or CORINFO_LOOKUP_THISOBJ) else { - if (DictionaryLayout::FindToken(pContextMT, pModule->GetLoaderAllocator(), 2, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult)) + if (DictionaryLayout::FindToken(pContextMT, pModule->GetLoaderAllocator(), 2, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult, &dictionarySlot)) { pResult->testForNull = 1; @@ -2924,7 +2926,7 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock pResult->indirectSecondOffset = 1; } - *pDictionaryIndexAndSlot |= pResult->slot; + *pDictionaryIndexAndSlot |= dictionarySlot; } } } From 44d89240362df20c4e877652580a350737c135eb Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 21 Aug 2019 16:59:11 -0700 Subject: [PATCH 04/21] Fixing the Crst levels bug found with CI tests --- src/vm/ceeload.cpp | 10 +++++----- src/vm/ceeload.h | 3 +++ src/vm/class.cpp | 3 ++- src/vm/genericdict.cpp | 12 ++++++------ src/vm/genmeth.cpp | 2 +- 5 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index a25e253da101..ace335bd32e8 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -556,6 +556,7 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) m_FixupCrst.Init(CrstModuleFixup, (CrstFlags)(CRST_HOST_BREAKABLE|CRST_REENTRANCY)); m_InstMethodHashTableCrst.Init(CrstInstMethodHashTable, CRST_REENTRANCY); m_ISymUnmanagedReaderCrst.Init(CrstISymUnmanagedReader, CRST_DEBUGGER_THREAD); + m_DictionaryCrst.Init(CrstDomainLocalBlock); if (!m_file->HasNativeImage()) { @@ -13245,7 +13246,7 @@ void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParent GC_TRIGGERS; PRECONDITION(CheckPointer(pGenericParentMT) && CheckPointer(pDependencyMT)); PRECONDITION(pGenericParentMT->HasInstantiation() && pGenericParentMT != pGenericParentMT->GetCanonicalMethodTable()); - PRECONDITION(SystemDomain::IsUnderDomainLock()); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END @@ -13259,7 +13260,7 @@ void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD { GC_TRIGGERS; PRECONDITION(CheckPointer(pDependencyMD) && pDependencyMD->HasMethodInstantiation() && pDependencyMD->IsInstantiatingStub()); - PRECONDITION(SystemDomain::IsUnderDomainLock()); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END @@ -13275,8 +13276,7 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); PRECONDITION(CheckPointer(pMT) && pMT->HasInstantiation() && pMT->GetClass()->GetDictionaryLayout() == pOldLayout); - PRECONDITION(SystemDomain::IsUnderDomainLock()); - + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END @@ -13348,7 +13348,7 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); PRECONDITION(CheckPointer(pMD)); PRECONDITION(pMD->HasMethodInstantiation() && pMD->GetDictionaryLayout() == pOldLayout); - PRECONDITION(SystemDomain::IsUnderDomainLock()); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END diff --git a/src/vm/ceeload.h b/src/vm/ceeload.h index 0533b855ad22..25364f1c7c70 100644 --- a/src/vm/ceeload.h +++ b/src/vm/ceeload.h @@ -3241,6 +3241,9 @@ class Module void SetNativeMetadataAssemblyRefInCache(DWORD rid, PTR_Assembly pAssembly); #endif // !defined(DACCESS_COMPILE) + // For protecting dictionary layout slot additions, and additions to the m_dynamicSlotsHashFor{Types/Methods} below + CrstExplicitInit m_DictionaryCrst; + #ifndef CROSSGEN_COMPILE private: class DynamicDictSlotsForTypesTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; diff --git a/src/vm/class.cpp b/src/vm/class.cpp index 60cc1e24ef42..f883160f2b87 100644 --- a/src/vm/class.cpp +++ b/src/vm/class.cpp @@ -1008,8 +1008,9 @@ void ClassLoader::RecordDependenciesForDictionaryExpansion(MethodTable* pMT) RETURN; } - SystemDomain::LockHolder lh; { + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); + // Update all inherited dictionary pointers which we could not embed, in case they got updated by some // other thread during a dictionary expansion before taking this current lock. MethodTable* pParentMT = pMT->GetParentMethodTable(); diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index a8699401994b..c545d5b21bef 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -184,7 +184,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat return FALSE; // A lock should be taken by FindToken before being allowed to use an empty slot in the layout - _ASSERT(SystemDomain::IsUnderDomainLock()); + _ASSERT(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); *EnsureWritablePages(&(pDictLayout->m_slots[iSlot].m_signature)) = pResultSignature; @@ -221,7 +221,7 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* { STANDARD_VM_CHECK; INJECT_FAULT(ThrowOutOfMemory();); - PRECONDITION(SystemDomain::IsUnderDomainLock()); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); } CONTRACTL_END @@ -279,7 +279,7 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut)) return TRUE; - SystemDomain::LockHolder lh; + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, TRUE)) @@ -336,7 +336,7 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut)) return TRUE; - SystemDomain::LockHolder lh; + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, TRUE)) @@ -847,7 +847,7 @@ Dictionary::PopulateEntry( #if _DEBUG // Lock is needed because dictionary pointers can get updated during dictionary size expansion - SystemDomain::LockHolder lh; + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); // MethodTable is expected to be normalized Dictionary* pDictionary = pMT->GetDictionary(); @@ -1331,7 +1331,7 @@ Dictionary::PopulateEntry( if ((slotIndex != 0) && !IsCompilationProcess()) { // Lock is needed because dictionary pointers can get updated during dictionary size expansion - SystemDomain::LockHolder lh; + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary() : pMD->GetMethodDictionary(); diff --git a/src/vm/genmeth.cpp b/src/vm/genmeth.cpp index 41f567720658..8315294a19a2 100644 --- a/src/vm/genmeth.cpp +++ b/src/vm/genmeth.cpp @@ -1188,7 +1188,7 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, #ifndef CROSSGEN_COMPILE if (pInstMD->HasMethodInstantiation()) { - SystemDomain::LockHolder lh; + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); pInstMD->GetModule()->RecordMethodForDictionaryExpansion_Locked(pInstMD); } #endif From 02b8c03c08c181296e77328ea8d3b2a1503aa15e Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 22 Aug 2019 16:23:26 -0700 Subject: [PATCH 05/21] Reduce number of initial slots back to 4 --- src/vm/genericdict.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/vm/genericdict.h b/src/vm/genericdict.h index a906bb3e25af..d0266281db40 100644 --- a/src/vm/genericdict.h +++ b/src/vm/genericdict.h @@ -94,7 +94,11 @@ typedef DPTR(DictionaryLayout) PTR_DictionaryLayout; // Number of slots to initially allocate in a generic method dictionary layout. // Also the number of additional slots to add to an existing dictionary layout when expanding its size -#define NUM_DICTIONARY_SLOTS 8 +#if _DEBUG +#define NUM_DICTIONARY_SLOTS 1 // Smaller number to stress the dictionary expansion logic +#else +#define NUM_DICTIONARY_SLOTS 4 +#endif // The type of dictionary layouts. We don't include the number of type // arguments as this is obtained elsewhere From 9b09953295e6799dcf98f65e2430525f9eb82a38 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Tue, 27 Aug 2019 13:52:32 -0700 Subject: [PATCH 06/21] Small optimization: avoid rescanning slots that were previously determined not equal to the one we're looking for, after taking the lock --- src/vm/genericdict.cpp | 45 +++++++++++++++++++++++++++++++++++++----- src/vm/genericdict.h | 1 + 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index c545d5b21bef..92fb6fabf0a0 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -116,6 +116,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat DictionaryEntrySignatureSource signatureSource, CORINFO_RUNTIME_LOOKUP* pResult, WORD* pSlotOut, + DWORD scanFromSlot /* = 0 */, BOOL useEmptySlotIfFound /* = FALSE */) { @@ -123,6 +124,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat { STANDARD_VM_CHECK; PRECONDITION(numGenericArgs > 0); + PRECONDITION(scanFromSlot >= 0 && scanFromSlot < pDictLayout->m_numSlots); PRECONDITION(CheckPointer(pDictLayout)); PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); PRECONDITION(CheckPointer(pSig)); @@ -131,10 +133,40 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat CONTRACTL_END // First slots contain the type parameters - _ASSERTE(FitsIn(numGenericArgs)); - WORD slot = static_cast(numGenericArgs); + _ASSERTE(FitsIn(numGenericArgs + scanFromSlot)); + WORD slot = static_cast(numGenericArgs + scanFromSlot); - for (DWORD iSlot = 0; iSlot < pDictLayout->m_numSlots; iSlot++) +#if _DEBUG + if (scanFromSlot > 0) + { + _ASSERT(useEmptySlotIfFound); + + for (DWORD iSlot = 0; iSlot < scanFromSlot; iSlot++) + { + // Verify that no entry before scanFromSlot matches the entry we're searching for + BYTE* pCandidate = (BYTE*)pDictLayout->m_slots[iSlot].m_signature; + if (pSigBuilder != NULL) + { + if (pDictLayout->m_slots[iSlot].m_signatureSource != FromReadyToRunImage) + { + DWORD j; + for (j = 0; j < cbSig; j++) + { + if (pCandidate[j] != pSig[j]) + break; + } + _ASSERT(j != cbSig); + } + } + else + { + _ASSERT(pCandidate != pSig); + } + } + } +#endif + + for (DWORD iSlot = scanFromSlot; iSlot < pDictLayout->m_numSlots; iSlot++) { BYTE* pCandidate = (BYTE*)pDictLayout->m_slots[iSlot].m_signature; if (pCandidate != NULL) @@ -181,7 +213,10 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat else { if (!useEmptySlotIfFound) + { + *pSlotOut = static_cast(iSlot); return FALSE; + } // A lock should be taken by FindToken before being allowed to use an empty slot in the layout _ASSERT(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); @@ -282,7 +317,7 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, TRUE)) + if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) return TRUE; @@ -339,7 +374,7 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, TRUE)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE diff --git a/src/vm/genericdict.h b/src/vm/genericdict.h index d0266281db40..13e5d3ff00e8 100644 --- a/src/vm/genericdict.h +++ b/src/vm/genericdict.h @@ -125,6 +125,7 @@ class DictionaryLayout DictionaryEntrySignatureSource signatureSource, CORINFO_RUNTIME_LOOKUP* pResult, WORD* pSlotOut, + DWORD scanFromSlot = 0, BOOL useEmptySlotIfFound = FALSE); From bd690578cb0332371464d2681dfc422bb984d1c6 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Tue, 27 Aug 2019 17:18:31 -0700 Subject: [PATCH 07/21] Add FlushProcessWriteBuffers --- src/vm/genericdict.cpp | 6 ++++++ src/vm/interoputil.cpp | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index 92fb6fabf0a0..a4734b4653c2 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -338,6 +338,9 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, // can start using newly added dictionary layout slots on types where the PerInstInfo hasn't expanded yet, and cause runtime failures. pMT->GetClass()->SetDictionaryLayout(pNewLayout); + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); + return TRUE; #else pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); @@ -394,6 +397,9 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, // can start using newly added dictionary layout slots on methods where the PerInstInfo hasn't expanded yet, and cause runtime failures. pMD->AsInstantiatedMethodDesc()->IMD_SetDictionaryLayout(pNewLayout); + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); + return TRUE; #else pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); diff --git a/src/vm/interoputil.cpp b/src/vm/interoputil.cpp index f07adc92dada..3981bca6c567 100644 --- a/src/vm/interoputil.cpp +++ b/src/vm/interoputil.cpp @@ -5833,7 +5833,7 @@ MethodDesc *WinRTInterfaceRedirector::GetStubMethodForRedirectedInterface(WinMDA if (interfaceIndex == WinMDAdapter::RedirectedTypeIndex_System_Collections_Generic_IDictionary || interfaceIndex == WinMDAdapter::RedirectedTypeIndex_System_Collections_Generic_IReadOnlyDictionary) - { + { thKvPair = TypeHandle(MscorlibBinder::GetClass(CLASS__KEYVALUEPAIRGENERIC)).Instantiate(inst); inst = Instantiation(&thKvPair, 1); } From 97221cf338a6e7ed61c0488f749315f7435db0a6 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 28 Aug 2019 18:28:55 -0700 Subject: [PATCH 08/21] Fix slot bug --- src/vm/genericdict.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index a4734b4653c2..587df45f5db0 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -124,7 +124,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat { STANDARD_VM_CHECK; PRECONDITION(numGenericArgs > 0); - PRECONDITION(scanFromSlot >= 0 && scanFromSlot < pDictLayout->m_numSlots); + PRECONDITION(scanFromSlot >= 0 && scanFromSlot <= pDictLayout->m_numSlots); PRECONDITION(CheckPointer(pDictLayout)); PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); PRECONDITION(CheckPointer(pSig)); @@ -237,6 +237,7 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat slot++; } + *pSlotOut = pDictLayout->m_numSlots; return FALSE; } From 0e1c6c8a08b537003b9030aa7d6451ee62e146ba Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 29 Aug 2019 11:48:02 -0700 Subject: [PATCH 09/21] CR feedback from David --- src/vm/genericdict.cpp | 16 ++++++++-------- src/vm/genericdict.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index 587df45f5db0..cf075754daea 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -312,7 +312,7 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut)) + if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) return TRUE; CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); @@ -334,14 +334,14 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, // First, expand the PerInstInfo dictionaries on types that were using the dictionary layout that just got expanded, and expand their slots pMT->GetModule()->ExpandTypeDictionaries_Locked(pMT, pOldLayout, pNewLayout); + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); + // Finally, update the dictionary layout pointer after all dictionaries of instantiated types have expanded, so that subsequent calls to // DictionaryLayout::FindToken can use this. It is important to update the dictionary layout at the very last step, otherwise some other threads // can start using newly added dictionary layout slots on types where the PerInstInfo hasn't expanded yet, and cause runtime failures. pMT->GetClass()->SetDictionaryLayout(pNewLayout); - // Ensure no other thread uses old dictionary pointers - FlushProcessWriteBuffers(); - return TRUE; #else pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); @@ -372,7 +372,7 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) return TRUE; CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); @@ -393,14 +393,14 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, // First, expand the PerInstInfo dictionaries on methods that were using the dictionary layout that just got expanded, and expand their slots pMD->GetModule()->ExpandMethodDictionaries_Locked(pMD, pOldLayout, pNewLayout); + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); + // Finally, update the dictionary layout pointer after all dictionaries of instantiated methods have expanded, so that subsequent calls to // DictionaryLayout::FindToken can use this. It is important to update the dictionary layout at the very last step, otherwise some other threads // can start using newly added dictionary layout slots on methods where the PerInstInfo hasn't expanded yet, and cause runtime failures. pMD->AsInstantiatedMethodDesc()->IMD_SetDictionaryLayout(pNewLayout); - // Ensure no other thread uses old dictionary pointers - FlushProcessWriteBuffers(); - return TRUE; #else pResult->signature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, 0); diff --git a/src/vm/genericdict.h b/src/vm/genericdict.h index 13e5d3ff00e8..fbe11205b715 100644 --- a/src/vm/genericdict.h +++ b/src/vm/genericdict.h @@ -125,8 +125,8 @@ class DictionaryLayout DictionaryEntrySignatureSource signatureSource, CORINFO_RUNTIME_LOOKUP* pResult, WORD* pSlotOut, - DWORD scanFromSlot = 0, - BOOL useEmptySlotIfFound = FALSE); + DWORD scanFromSlot, + BOOL useEmptySlotIfFound); static DictionaryLayout* ExpandDictionaryLayout(LoaderAllocator* pAllocator, From c0eb5369b69e611d49110efcdad497216d9e9311 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 18 Sep 2019 17:45:55 -0700 Subject: [PATCH 10/21] Fixing a race condition between type loader and generic dictionary expansion code --- src/debug/daccess/nidump.cpp | 12 +-- src/vm/ceeload.cpp | 146 ++++++++++++++++++++++++++++++---- src/vm/class.cpp | 14 ++-- src/vm/class.h | 4 +- src/vm/genericdict.cpp | 31 ++++---- src/vm/generics.cpp | 14 +++- src/vm/genmeth.cpp | 35 +++++++- src/vm/method.cpp | 30 +++---- src/vm/method.hpp | 22 +++-- src/vm/methodtable.cpp | 53 ++++++++---- src/vm/methodtable.h | 16 +++- src/vm/methodtable.inl | 4 +- src/vm/methodtablebuilder.cpp | 9 ++- 13 files changed, 297 insertions(+), 93 deletions(-) diff --git a/src/debug/daccess/nidump.cpp b/src/debug/daccess/nidump.cpp index 4a9e6367ae97..2079b261640a 100644 --- a/src/debug/daccess/nidump.cpp +++ b/src/debug/daccess/nidump.cpp @@ -6562,7 +6562,7 @@ void NativeImageDumper::MethodDescToString( PTR_MethodDesc md, { PTR_InstantiatedMethodDesc imd(PTR_TO_TADDR(md)); unsigned numArgs = imd->m_wNumGenericArgs; - PTR_Dictionary dict(imd->IMD_GetMethodDictionary()); + PTR_Dictionary dict(imd->IMD_GetMethodDictionary_Unsafe()); if( dict != NULL ) { DictionaryToArgString( dict, numArgs, tempName ); @@ -6812,7 +6812,7 @@ NativeImageDumper::DumpMethodTable( PTR_MethodTable mt, const char * name, if( !mt->IsCanonicalMethodTable() && CORCOMPILE_IS_POINTER_TAGGED(PTR_TO_TADDR(mt->GetCanonicalMethodTable())) ) { /* REVISIT_TODO Wed 02/01/2006 - * GetExtent requires the class in order to compute GetInstAndDictSize. + * GetExtent requires the class in order to compute GetInstAndDictSize_Unsafe. * If the EEClass isn't present, I cannot compute the size. If we are * in this case, skip all of the generic dictionaries. */ @@ -6980,12 +6980,12 @@ NativeImageDumper::DumpMethodTable( PTR_MethodTable mt, const char * name, * only print the last one, which is the one for this class). * cloned from Genericdict.cpp */ - PTR_Dictionary currentDictionary(mt->GetDictionary()); + PTR_Dictionary currentDictionary(mt->GetDictionary_Unsafe()); if( currentDictionary != NULL ) { PTR_DictionaryEntry entry(currentDictionary->EntryAddr(0)); - - PTR_DictionaryLayout layout( clazz->GetDictionaryLayout() ); + + PTR_DictionaryLayout layout( clazz->GetDictionaryLayout_Unsafe() ); DisplayStartStructure( "Dictionary", DPtrToPreferredAddr(currentDictionary), @@ -7994,7 +7994,7 @@ void NativeImageDumper::DumpMethodDesc( PTR_MethodDesc md, PTR_Module module ) } //now handle the contents of the m_pMethInst/m_pPerInstInfo union. unsigned numSlots = imd->m_wNumGenericArgs; - PTR_Dictionary inst(imd->IMD_GetMethodDictionary()); + PTR_Dictionary inst(imd->IMD_GetMethodDictionary_Unsafe()); unsigned dictSize; if( kind == InstantiatedMethodDesc::SharedMethodInstantiation ) { diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index ace335bd32e8..9d98d5e719f3 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -9023,7 +9023,7 @@ void Module::PlaceType(DataImage *image, TypeHandle th, DWORD profilingFlags) } } - Dictionary * pDictionary = pMT->GetDictionary(); + Dictionary * pDictionary = pMT->GetDictionary_Unsafe(); if (pDictionary != NULL) { BOOL fIsWriteable; @@ -9037,7 +9037,7 @@ void Module::PlaceType(DataImage *image, TypeHandle th, DWORD profilingFlags) fIsWriteable = pDictionary->IsWriteable(image, canSaveSlots, pMT->GetNumGenericArgs(), pMT->GetModule(), - pClass->GetDictionaryLayout()); + pClass->GetDictionaryLayout_Unsafe()); } else { @@ -9047,7 +9047,7 @@ void Module::PlaceType(DataImage *image, TypeHandle th, DWORD profilingFlags) if (fIsWriteable) { image->PlaceStructureForAddress(pDictionary, CORCOMPILE_SECTION_HOT_WRITEABLE); - image->PlaceStructureForAddress(pClass->GetDictionaryLayout(), CORCOMPILE_SECTION_WARM); + image->PlaceStructureForAddress(pClass->GetDictionaryLayout_Unsafe(), CORCOMPILE_SECTION_WARM); } else { @@ -13250,6 +13250,35 @@ void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParent } CONTRACTL_END + DictionaryLayout* pDictLayout = pDependencyMT->GetClass()->GetDictionaryLayout_Unsafe(); + if (pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0) + { + DWORD sizeFromDictLayout = DictionaryLayout::GetFirstDictionaryBucketSize(pDependencyMT->GetNumGenericArgs(), pDictLayout); + if (pDependencyMT->GetDictionarySlotsSize() != sizeFromDictLayout) + { + _ASSERT(pDependencyMT->GetDictionarySlotsSize() < sizeFromDictLayout); + + // + // Another thread got a chance to expand the dictionary layout and expand the dictionary slots of + // other types, but not for this one yet because we're still in the process of recording it for + // expansions. + // Expand the dictionary slots here before finally adding the type to the hashtable. + // + + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pDependencyMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout)); + ZeroMemory(pInstOrPerInstInfo, sizeFromDictLayout); + + // Copy old dictionary entry contents + memcpy(pInstOrPerInstInfo, (const void*)pDependencyMT->GetPerInstInfo()[pDependencyMT->GetNumDicts() - 1].GetValue(), pDependencyMT->GetDictionarySlotsSize()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pDependencyMT->GetNumGenericArgs(); + *pSizeSlot = sizeFromDictLayout; + + // Publish the new dictionary slots to the type. + pDependencyMT->GetPerInstInfo()[pDependencyMT->GetNumDicts() - 1].SetValue((Dictionary*)pInstOrPerInstInfo); + } + } + GCX_COOP(); m_dynamicSlotsHashForTypes.Add(pGenericParentMT->GetCanonicalMethodTable(), pDependencyMT, GetLoaderAllocator()); } @@ -13260,10 +13289,38 @@ void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD { GC_TRIGGERS; PRECONDITION(CheckPointer(pDependencyMD) && pDependencyMD->HasMethodInstantiation() && pDependencyMD->IsInstantiatingStub()); + PRECONDITION(pDependencyMD->GetDictionaryLayout_Unsafe() != NULL && pDependencyMD->GetDictionaryLayout_Unsafe()->GetMaxSlots() > 0); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END - + + DictionaryLayout* pDictLayout = pDependencyMD->GetDictionaryLayout_Unsafe(); + InstantiatedMethodDesc* pIMDDependency = pDependencyMD->AsInstantiatedMethodDesc(); + + DWORD sizeFromDictLayout = DictionaryLayout::GetFirstDictionaryBucketSize(pDependencyMD->GetNumGenericMethodArgs(), pDictLayout); + if (pIMDDependency->GetDictionarySlotsSize() != sizeFromDictLayout) + { + _ASSERT(pIMDDependency->GetDictionarySlotsSize() < sizeFromDictLayout); + + // + // Another thread got a chance to expand the dictionary layout and expand the dictionary slots of + // other methods, but not for this one yet because we're still in the process of recording it for + // expansions. + // Expand the dictionary slots here before finally adding the method to the hashtable. + // + + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pIMDDependency->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout)); + ZeroMemory(pInstOrPerInstInfo, sizeFromDictLayout); + + // Copy old dictionary entry contents + memcpy(pInstOrPerInstInfo, (const void*)pIMDDependency->m_pPerInstInfo.GetValue(), pIMDDependency->GetDictionarySlotsSize()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pIMDDependency->GetNumGenericMethodArgs(); + *pSizeSlot = sizeFromDictLayout; + + pIMDDependency->m_pPerInstInfo.SetValue((Dictionary*)pInstOrPerInstInfo); + } + GCX_COOP(); m_dynamicSlotsHashForMethods.Add(pDependencyMD->GetWrappedMethodDesc(), pDependencyMD, GetLoaderAllocator()); } @@ -13275,7 +13332,7 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p STANDARD_VM_CHECK; INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); - PRECONDITION(CheckPointer(pMT) && pMT->HasInstantiation() && pMT->GetClass()->GetDictionaryLayout() == pOldLayout); + PRECONDITION(CheckPointer(pMT) && pMT->HasInstantiation() && pMT->GetClass()->GetDictionaryLayout_Unsafe() == pOldLayout); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END @@ -13287,33 +13344,59 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMT->GetNumGenericArgs(), pNewLayout); // - // Dictionary expansion for types needs to be done in two steps, given how derived types do not directly embed dictionaries - // from parent types, but instead reference them from directly from the parent types: + // Dictionary expansion for types needs to be done in multiple steps, given how derived types do not directly embed dictionaries + // from parent types, but instead reference them from directly from the parent types. Also, this is necessary to ensure correctness + // for lock-free read operations for dictionary slots: // 1) Allocate new dictionaries for all instantiated types of the same typedef as the one being expanded. - // 2) For all types that derive from the one being expanded, update the embedded dictinoary pointer to the newly - // allocated one. + // 2) After all allocations and initializations are completed, publish the dictionaries to the types in #1 after + // flushing write buffers + // 3) For all types that derive from #1, update the embedded dictinoary pointer to the newly allocated one. // - auto expandPerInstInfos = [oldInfoSize, newInfoSize](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) + struct NewDictionary + { + MethodTable* pMT; + TypeHandle* pDictSlots; + }; + StackSArray dictionaryUpdates; + + auto expandPerInstInfos = [oldInfoSize, newInfoSize, &dictionaryUpdates](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) { if (!pMTToUpdate->HasSameTypeDefAs(pMTKey)) return true; _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey); + _ASSERTE(pMTToUpdate->GetDictionarySlotsSize() == oldInfoSize); TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMTToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); ZeroMemory(pInstOrPerInstInfo, newInfoSize); // Copy old dictionary entry contents - memcpy(pInstOrPerInstInfo, (const void*)pMTToUpdate->GetPerInstInfo()[pMTToUpdate->GetNumDicts() - 1].GetValue(), oldInfoSize); + memcpy(pInstOrPerInstInfo, (const void*)pMTToUpdate->GetPerInstInfo()[pMTToUpdate->GetNumDicts() - 1].GetValue(), pMTToUpdate->GetDictionarySlotsSize()); - pMTToUpdate->GetPerInstInfo()[pMTToUpdate->GetNumDicts() - 1].SetValue((Dictionary*)pInstOrPerInstInfo); + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMTToUpdate->GetNumGenericArgs(); + *pSizeSlot = newInfoSize; + + NewDictionary entry; + entry.pMT = pMTToUpdate; + entry.pDictSlots = pInstOrPerInstInfo; + dictionaryUpdates.Append(entry); return true; // Keep walking }; m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, expandPerInstInfos); + // Flush write buffers to ensure new dictionaries are fully writted and initalized before publishing them + FlushProcessWriteBuffers(); + + for (SArray::Iterator i = dictionaryUpdates.Begin(); i != dictionaryUpdates.End(); i++) + { + MethodTable* pMT = i->pMT; + pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].SetValue((Dictionary*)i->pDictSlots); + _ASSERTE(pMT->GetDictionarySlotsSize() == newInfoSize); + } + auto updateDependentDicts = [](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) { if (pMTToUpdate->HasSameTypeDefAs(pMTKey)) @@ -13347,36 +13430,67 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); PRECONDITION(CheckPointer(pMD)); - PRECONDITION(pMD->HasMethodInstantiation() && pMD->GetDictionaryLayout() == pOldLayout); + PRECONDITION(pMD->HasMethodInstantiation() && pMD->GetDictionaryLayout_Unsafe() == pOldLayout); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END GCX_COOP(); + // + // Dictionary expansion for methods needs to be done in two steps to ensure correctness for lock-free read operations + // for dictionary slots: + // 1) Allocate new dictionaries for all instantiated methods sharing the same canonical form as the input method + // 2) After all allocations and initializations are completed, publish the dictionaries to the methods after + // flushing write buffers + // + MethodDesc* pCanonMD = pMD->IsInstantiatingStub() ? pMD->GetWrappedMethodDesc() : pMD; DWORD oldInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pOldLayout); DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pNewLayout); - auto lambda = [oldInfoSize, newInfoSize](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) + struct NewDictionary + { + InstantiatedMethodDesc* pIMD; + TypeHandle* pDictSlots; + }; + StackSArray dictionaryUpdates; + + auto lambda = [oldInfoSize, newInfoSize, &dictionaryUpdates](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) { // Update m_pPerInstInfo for the pMethodDesc being visited here _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetWrappedMethodDesc() == pMDKey); InstantiatedMethodDesc* pInstantiatedMD = pMDToUpdate->AsInstantiatedMethodDesc(); + _ASSERTE(pInstantiatedMD->GetDictionarySlotsSize() == oldInfoSize); TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMDToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); ZeroMemory(pInstOrPerInstInfo, newInfoSize); // Copy old dictionary entry contents - memcpy(pInstOrPerInstInfo, (const void*)pInstantiatedMD->m_pPerInstInfo.GetValue(), oldInfoSize); + memcpy(pInstOrPerInstInfo, (const void*)pInstantiatedMD->m_pPerInstInfo.GetValue(), pInstantiatedMD->GetDictionarySlotsSize()); - pInstantiatedMD->m_pPerInstInfo.SetValue((Dictionary*)pInstOrPerInstInfo); + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMDToUpdate->GetNumGenericMethodArgs(); + *pSizeSlot = newInfoSize; + + NewDictionary entry; + entry.pIMD = pInstantiatedMD; + entry.pDictSlots = pInstOrPerInstInfo; + dictionaryUpdates.Append(entry); return true; // Keep walking }; m_dynamicSlotsHashForMethods.VisitValuesOfKey(pCanonMD, lambda); + + // Flush write buffers to ensure new dictionaries are fully writted and initalized before publishing them + FlushProcessWriteBuffers(); + + for (SArray::Iterator i = dictionaryUpdates.Begin(); i != dictionaryUpdates.End(); i++) + { + i->pIMD->m_pPerInstInfo.SetValue((Dictionary*)i->pDictSlots); + _ASSERTE(i->pIMD->GetDictionarySlotsSize() == newInfoSize); + } } #endif // !CROSSGEN_COMPILE diff --git a/src/vm/class.cpp b/src/vm/class.cpp index f883160f2b87..42768fcc9ea3 100644 --- a/src/vm/class.cpp +++ b/src/vm/class.cpp @@ -901,7 +901,7 @@ ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable *pMT) } - if (pParentMT != NULL && pParentMT->HasPerInstInfo()) + /*if (pParentMT != NULL && pParentMT->HasPerInstInfo()) { // Copy down all inherited dictionary pointers which we // could not embed. @@ -913,7 +913,7 @@ ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable *pMT) pMT->GetPerInstInfo()[iDict].SetValueMaybeNull(pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()); } } - } + }*/ #ifdef FEATURE_PREJIT // Restore action, not in MethodTable::Restore because we may have had approx parents at that point @@ -1011,11 +1011,13 @@ void ClassLoader::RecordDependenciesForDictionaryExpansion(MethodTable* pMT) { CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - // Update all inherited dictionary pointers which we could not embed, in case they got updated by some - // other thread during a dictionary expansion before taking this current lock. MethodTable* pParentMT = pMT->GetParentMethodTable(); if (pParentMT != NULL && pParentMT->HasPerInstInfo()) { + // Copy down all inherited dictionary pointers which we could not embed. + // This step has to be done under the dictionary lock, to prevent other threads from making updates + // the the dictionaries of the parent types while we're also copying them over to the derived type here. + DWORD nDicts = pParentMT->GetNumDicts(); for (DWORD iDict = 0; iDict < nDicts; iDict++) { @@ -2798,7 +2800,7 @@ void EEClass::Save(DataImage *image, MethodTable *pMT) } // Save dictionary layout information - DictionaryLayout *pDictLayout = GetDictionaryLayout(); + DictionaryLayout *pDictLayout = GetDictionaryLayout_Unsafe(); if (pMT->IsSharedByGenericInstantiations() && pDictLayout != NULL) { pDictLayout->Save(image); @@ -2917,7 +2919,7 @@ void EEClass::Fixup(DataImage *image, MethodTable *pMT) } #endif // FEATURE_COMINTEROP - DictionaryLayout *pDictLayout = GetDictionaryLayout(); + DictionaryLayout *pDictLayout = GetDictionaryLayout_Unsafe(); if (pDictLayout != NULL) { pDictLayout->Fixup(image, FALSE); diff --git a/src/vm/class.h b/src/vm/class.h index 39ebcb31fb66..1baeb2a13312 100644 --- a/src/vm/class.h +++ b/src/vm/class.h @@ -1797,7 +1797,9 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! public: - PTR_DictionaryLayout GetDictionaryLayout() + // This API is not multi-threaded safe: the dictionary layout pointer can be updated by another + // thread during a generic dictionary size expansion. + PTR_DictionaryLayout GetDictionaryLayout_Unsafe() { SUPPORTS_DAC; WRAPPER_NO_CONTRACT; diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index cf075754daea..d86a9dcbcdd6 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -85,9 +85,12 @@ DictionaryLayout::GetFirstDictionaryBucketSize( PRECONDITION(numGenericArgs > 0); PRECONDITION(CheckPointer(pDictLayout, NULL_OK)); - DWORD bytes = numGenericArgs * sizeof(TypeHandle); + DWORD bytes = numGenericArgs * sizeof(TypeHandle); // Slots for instantiation arguments if (pDictLayout != NULL) - bytes += pDictLayout->m_numSlots * sizeof(void*); + { + bytes += sizeof(ULONG_PTR*); // Slot for dictionary size + bytes += pDictLayout->m_numSlots * sizeof(void*); // Slots for diciontary slots based on a dictionary layout + } return bytes; } @@ -133,8 +136,8 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat CONTRACTL_END // First slots contain the type parameters - _ASSERTE(FitsIn(numGenericArgs + scanFromSlot)); - WORD slot = static_cast(numGenericArgs + scanFromSlot); + _ASSERTE(FitsIn(numGenericArgs + 1 + scanFromSlot)); + WORD slot = static_cast(numGenericArgs + 1 + scanFromSlot); #if _DEBUG if (scanFromSlot > 0) @@ -273,7 +276,7 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* pNewDictionaryLayout->m_slots[iSlot] = pCurrentDictLayout->m_slots[iSlot]; WORD layoutSlotIndex = pCurrentDictLayout->m_numSlots; - WORD slot = static_cast(numGenericArgs) + layoutSlotIndex; + WORD slot = static_cast(numGenericArgs) + 1 + layoutSlotIndex; PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature)) = pResultSignature; @@ -312,18 +315,18 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) + if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) return TRUE; CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) + if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE - DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout(); + DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout_Unsafe(); DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMT->GetNumGenericArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult, pSlotOut); if (pNewLayout == NULL) { @@ -372,17 +375,17 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) return TRUE; CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE - DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout(); + DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout_Unsafe(); DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMD->GetNumGenericMethodArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult, pSlotOut); if (pNewLayout == NULL) { @@ -892,7 +895,7 @@ Dictionary::PopulateEntry( CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); // MethodTable is expected to be normalized - Dictionary* pDictionary = pMT->GetDictionary(); + Dictionary* pDictionary = pMT->GetDictionary_Unsafe(); _ASSERTE(pDictionary == pMT->GetPerInstInfo()[dictionaryIndex].GetValueMaybeNull()); #endif } @@ -1375,7 +1378,7 @@ Dictionary::PopulateEntry( // Lock is needed because dictionary pointers can get updated during dictionary size expansion CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary() : pMD->GetMethodDictionary(); + Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary_Unsafe() : pMD->GetMethodDictionary_Unsafe(); *EnsureWritablePages(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); @@ -1395,7 +1398,7 @@ Dictionary::PrepopulateDictionary( { STANDARD_VM_CONTRACT; - DictionaryLayout * pDictLayout = (pMT != NULL) ? pMT->GetClass()->GetDictionaryLayout() : pMD->GetDictionaryLayout(); + DictionaryLayout * pDictLayout = (pMT != NULL) ? pMT->GetClass()->GetDictionaryLayout_Unsafe() : pMD->GetDictionaryLayout_Unsafe(); DWORD numGenericArgs = (pMT != NULL) ? pMT->GetNumGenericArgs() : pMD->GetNumGenericMethodArgs(); if (pDictLayout != NULL) diff --git a/src/vm/generics.cpp b/src/vm/generics.cpp index c018f8a8a7b1..76c1871ce30d 100644 --- a/src/vm/generics.cpp +++ b/src/vm/generics.cpp @@ -278,7 +278,12 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( DWORD cbPerInst = sizeof(GenericsDictInfo) + pOldMT->GetPerInstInfoSize(); // Finally we need space for the instantiation/dictionary for this type - DWORD cbInstAndDict = pOldMT->GetInstAndDictSize(); + // Note that this is an unsafe operation because it uses the dictionary layout to compute the size needed, + // and the dictionary layout can be updated by other threads during a dictionary size expansion. To account for + // this rare race condition, right before registering this type for dictionary expansions, we will check that its + // dictionary has enough slots to match its dictionary layout if it got updated. + // See: Module::RecordTypeForDictionaryExpansion_Locked. + DWORD cbInstAndDict = pOldMT->GetInstAndDictSize_Unsafe(); // Allocate from the high frequence heap of the correct domain S_SIZE_T allocSize = safe_cbMT; @@ -488,6 +493,13 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( pInstDest[iArg] = inst[iArg]; } + if (cbInstAndDict != 0 && pOldMT->GetClass()->GetDictionaryLayout_Unsafe() != NULL && pOldMT->GetClass()->GetDictionaryLayout_Unsafe()->GetMaxSlots() > 0) + { + UINT_PTR* pDictionarySlots = (UINT_PTR*)pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); + UINT_PTR* pSizeSlot = pDictionarySlots + ntypars; + *pSizeSlot = cbInstAndDict; + } + // Copy interface map across InterfaceInfo_t * pInterfaceMap = (InterfaceInfo_t *)(pMemory + cbMT + cbOptional + (fHasDynamicInterfaceMap ? sizeof(DWORD_PTR) : 0)); diff --git a/src/vm/genmeth.cpp b/src/vm/genmeth.cpp index 8315294a19a2..1ad81ddbb7fd 100644 --- a/src/vm/genmeth.cpp +++ b/src/vm/genmeth.cpp @@ -390,9 +390,19 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, // Allocate space for the instantiation and dictionary infoSize = DictionaryLayout::GetFirstDictionaryBucketSize(methodInst.GetNumArgs(), pDL); - pInstOrPerInstInfo = (TypeHandle *) (void*) amt.Track(pAllocator->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(infoSize))); + pInstOrPerInstInfo = (TypeHandle*)(void*)amt.Track(pAllocator->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(infoSize))); for (DWORD i = 0; i < methodInst.GetNumArgs(); i++) pInstOrPerInstInfo[i] = methodInst[i]; + + if (pDL != NULL && pDL->GetMaxSlots() > 0) + { + // Has to be at least larger than the first slots containing the instantiation arguments, + // and the slot with size information. Otherwise, we shouldn't really have a size slot + _ASSERTE(infoSize > (sizeof(TypeHandle*) * methodInst.GetNumArgs() + sizeof(ULONG_PTR*))); + + UINT_PTR* pDictSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + methodInst.GetNumArgs(); + *pDictSizeSlot = infoSize; + } } BOOL forComInterop = FALSE; @@ -1438,7 +1448,9 @@ void InstantiatedMethodDesc::SetupGenericMethodDefinition(IMDInternalImport *pIM // the memory allocated for m_pMethInst will be freed if the declaring type fails to load m_pPerInstInfo.SetValue((Dictionary *) pamTracker->Track(pAllocator->GetLowFrequencyHeap()->AllocMem(dwAllocSize))); - TypeHandle * pInstDest = (TypeHandle *) IMD_GetMethodDictionaryNonNull(); + // No lock needed here. This is actually a safe operation because dictionary on generic method definitions + // will never be expanded in size. + TypeHandle * pInstDest = (TypeHandle *) IMD_GetMethodDictionaryNonNull_Unsafe(); for(unsigned int i = 0; i < numTyPars; i++) { hEnumTyPars.EnumNext(&tkTyPar); @@ -1624,10 +1636,10 @@ void MethodDesc::PrepopulateDictionary(DataImage * image, BOOL nonExpansive) STANDARD_VM_CONTRACT; // Note the strong similarity to MethodTable::PrepopulateDictionary - if (GetMethodDictionary()) + if (GetMethodDictionary_Unsafe()) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Prepopulating dictionary for MD %s\n", this)); - GetMethodDictionary()->PrepopulateDictionary(this, NULL, nonExpansive); + GetMethodDictionary_Unsafe()->PrepopulateDictionary(this, NULL, nonExpansive); } } @@ -1704,4 +1716,19 @@ BOOL MethodDesc::SatisfiesMethodConstraints(TypeHandle thParent, BOOL fThrowIfNo return TRUE; } +DWORD InstantiatedMethodDesc::GetDictionarySlotsSize() +{ + CONTRACTL + { + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END + + UINT_PTR* pDictionarySlots = (UINT_PTR*)IMD_GetMethodDictionary_Unsafe(); + if (pDictionarySlots == NULL) + return 0; + UINT_PTR* pSizeSlot = pDictionarySlots + m_wNumGenericArgs; + return (DWORD)* pSizeSlot; +} + #endif // !DACCESS_COMPILE diff --git a/src/vm/method.cpp b/src/vm/method.cpp index 3c639705a1ad..744e6ab175a4 100644 --- a/src/vm/method.cpp +++ b/src/vm/method.cpp @@ -2395,18 +2395,18 @@ void MethodDesc::Reset() } //******************************************************************************* -Dictionary* MethodDesc::GetMethodDictionary() +Dictionary* MethodDesc::GetMethodDictionary_Unsafe() { WRAPPER_NO_CONTRACT; return (GetClassification() == mcInstantiated) - ? (Dictionary*) (AsInstantiatedMethodDesc()->IMD_GetMethodDictionary()) + ? (Dictionary*) (AsInstantiatedMethodDesc()->IMD_GetMethodDictionary_Unsafe()) : NULL; } //******************************************************************************* -DictionaryLayout* MethodDesc::GetDictionaryLayout() +DictionaryLayout* MethodDesc::GetDictionaryLayout_Unsafe() { WRAPPER_NO_CONTRACT; @@ -2644,11 +2644,11 @@ void MethodDesc::Save(DataImage *image) } } } - - if (GetMethodDictionary()) + + if (GetMethodDictionary_Unsafe()) { - DWORD cBytes = DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericMethodArgs(), GetDictionaryLayout()); - void* pBytes = GetMethodDictionary()->AsPtr(); + DWORD cBytes = DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericMethodArgs(), GetDictionaryLayout_Unsafe()); + void* pBytes = GetMethodDictionary_Unsafe()->AsPtr(); LOG((LF_ZAP, LL_INFO10000, " MethodDesc::Save dictionary size %d\n", cBytes)); image->StoreStructure(pBytes, cBytes, @@ -2927,9 +2927,9 @@ BOOL MethodDesc::ComputeNeedsRestore(DataImage *image, TypeHandleList *pVisited, return TRUE; } - if (GetMethodDictionary()) + if (GetMethodDictionary_Unsafe()) { - if (GetMethodDictionary()->ComputeNeedsRestore(image, pVisited, GetNumGenericMethodArgs())) + if (GetMethodDictionary_Unsafe()->ComputeNeedsRestore(image, pVisited, GetNumGenericMethodArgs())) return TRUE; } } @@ -3352,14 +3352,14 @@ MethodDesc::Fixup( image->FixupTypeHandlePointer(pInst, &pInst[j]); } } - else if (GetMethodDictionary()) + else if (GetMethodDictionary_Unsafe()) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Fixup dictionary for MD %s\n", m_pszDebugMethodName ? m_pszDebugMethodName : "")); BOOL canSaveInstantiation = TRUE; if (IsGenericMethodDefinition() && !IsTypicalMethodDefinition()) { - if (GetMethodDictionary()->ComputeNeedsRestore(image, NULL, GetNumGenericMethodArgs())) + if (GetMethodDictionary_Unsafe()->ComputeNeedsRestore(image, NULL, GetNumGenericMethodArgs())) { _ASSERTE(needsRestore); canSaveInstantiation = FALSE; @@ -3393,12 +3393,12 @@ MethodDesc::Fixup( pIMD->IMD_IsWrapperStubWithInstantiations() && image->CanEagerBindToMethodDesc(pIMD->IMD_GetWrappedMethodDesc()); - GetMethodDictionary()->Fixup(image, + GetMethodDictionary_Unsafe()->Fixup(image, canSaveInstantiation, canSaveSlots, GetNumGenericMethodArgs(), GetModule(), - GetDictionaryLayout()); + GetDictionaryLayout_Unsafe()); } if (needsRestore) @@ -3993,9 +3993,9 @@ void MethodDesc::CheckRestore(ClassLoadLevel level) Module::RestoreMethodDescPointer(&pIMD->m_pWrappedMethodDesc); // Finally restore the dictionary itself (including instantiation) - if (GetMethodDictionary()) + if (GetMethodDictionary_Unsafe()) { - GetMethodDictionary()->Restore(GetNumGenericMethodArgs(), level); + GetMethodDictionary_Unsafe()->Restore(GetNumGenericMethodArgs(), level); } #else ClassLoader::EnsureLoaded(TypeHandle(GetMethodTable()), level); diff --git a/src/vm/method.hpp b/src/vm/method.hpp index f78436ee08c9..a4ca5f4c4de8 100644 --- a/src/vm/method.hpp +++ b/src/vm/method.hpp @@ -500,8 +500,10 @@ class MethodDesc // Return a pointer to the method dictionary for an instantiated generic method // The initial slots in a method dictionary are the type arguments themselves // Return NULL if not an instantiated method - Dictionary* GetMethodDictionary(); - DictionaryLayout* GetDictionaryLayout(); + // These APIs are not multi-threaded safe: the dictionary and dictionary layout pointers + // can be updated by other threads during a dictionary size expansion. + Dictionary* GetMethodDictionary_Unsafe(); + DictionaryLayout* GetDictionaryLayout_Unsafe(); InstantiatedMethodDesc* AsInstantiatedMethodDesc() const; @@ -3468,18 +3470,26 @@ class InstantiatedMethodDesc : public MethodDesc Instantiation IMD_GetMethodInstantiation() { LIMITED_METHOD_DAC_CONTRACT; - - return Instantiation(IMD_GetMethodDictionary()->GetInstantiation(), m_wNumGenericArgs); + + // No lock needed here. This is considered a safe operation here because in the case of a generic dictionary + // expansion, the values of the old dictionary slots are copied to the newly allocated dictionary, and the old + // dictionary is kept around, so whether IMD_GetMethodDictionary_Unsafe returns the new or old dictionaries, the + // values of the instantiation arguments will always be the same. + return Instantiation(IMD_GetMethodDictionary_Unsafe()->GetInstantiation(), m_wNumGenericArgs); } - PTR_Dictionary IMD_GetMethodDictionary() + PTR_Dictionary IMD_GetMethodDictionary_Unsafe() { LIMITED_METHOD_DAC_CONTRACT; return ReadPointerMaybeNull(this, &InstantiatedMethodDesc::m_pPerInstInfo); } - PTR_Dictionary IMD_GetMethodDictionaryNonNull() +#ifndef DACCESS_COMPILE + DWORD GetDictionarySlotsSize(); +#endif + + PTR_Dictionary IMD_GetMethodDictionaryNonNull_Unsafe() { LIMITED_METHOD_DAC_CONTRACT; diff --git a/src/vm/methodtable.cpp b/src/vm/methodtable.cpp index c49f819418da..8b877afaa96b 100644 --- a/src/vm/methodtable.cpp +++ b/src/vm/methodtable.cpp @@ -475,6 +475,25 @@ void MethodTable::SetModule(Module * pModule) _ASSERTE(GetModule() == pModule); } + +DWORD MethodTable::GetDictionarySlotsSize() +{ + CONTRACTL + { + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END + + if (!HasPerInstInfo() || GetGenericsDictInfo()->m_wNumTyPars == 0 || GetClass()->GetDictionaryLayout_Unsafe() == NULL) + return 0; + + if (GetClass()->GetDictionaryLayout_Unsafe()->GetMaxSlots() == 0) + return 0; + + UINT_PTR* pDictionarySlots = (UINT_PTR*)GetPerInstInfo()[GetGenericsDictInfo()->m_wNumDicts - 1].GetValue(); + UINT_PTR* pSizeSlot = pDictionarySlots + GetGenericsDictInfo()->m_wNumTyPars; + return (DWORD)* pSizeSlot; +} #endif // DACCESS_COMPILE //========================================================================================== @@ -3972,7 +3991,7 @@ void MethodTable::PrepopulateDictionary(DataImage * image, BOOL nonExpansive) { STANDARD_VM_CONTRACT; - if (GetDictionary()) + if (GetDictionary_Unsafe()) { // We can only save elements of the dictionary if we are sure of its // layout, which means we must be either tightly-knit to the EEClass @@ -3984,7 +4003,7 @@ void MethodTable::PrepopulateDictionary(DataImage * image, BOOL nonExpansive) if (!IsCanonicalMethodTable() && image->CanEagerBindToMethodTable(GetCanonicalMethodTable())) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Prepopulating dictionary for MT %s\n", GetDebugClassName())); - GetDictionary()->PrepopulateDictionary(NULL, this, nonExpansive); + GetDictionary_Unsafe()->PrepopulateDictionary(NULL, this, nonExpansive); } } } @@ -4056,11 +4075,11 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) // Be careful about calling DictionaryLayout::Trim - strict conditions apply. // See note on that method. - if (GetDictionary() && - GetClass()->GetDictionaryLayout() && + if (GetDictionary_Unsafe() && + GetClass()->GetDictionaryLayout_Unsafe() && image->CanEagerBindToMethodTable(GetCanonicalMethodTable())) { - GetClass()->GetDictionaryLayout()->Trim(); + GetClass()->GetDictionaryLayout_Unsafe()->Trim(); } // Set the "restore" flags. They may not have been set yet. @@ -4222,7 +4241,7 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) image->BindPointer(GetPerInstInfo(), pPerInstInfoNode, sizeof(GenericsDictInfo)); } - Dictionary * pDictionary = GetDictionary(); + Dictionary * pDictionary = GetDictionary_Unsafe(); if (pDictionary != NULL) { BOOL fIsWriteable; @@ -4236,7 +4255,7 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) fIsWriteable = pDictionary->IsWriteable(image, canSaveSlots, GetNumGenericArgs(), GetModule(), - GetClass()->GetDictionaryLayout()); + GetClass()->GetDictionaryLayout_Unsafe()); } else { @@ -4246,11 +4265,11 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) if (!fIsWriteable) { - image->StoreInternedStructure(pDictionary, GetInstAndDictSize(), DataImage::ITEM_DICTIONARY); + image->StoreInternedStructure(pDictionary, GetInstAndDictSize_Unsafe(), DataImage::ITEM_DICTIONARY); } else { - image->StoreStructure(pDictionary, GetInstAndDictSize(), DataImage::ITEM_DICTIONARY_WRITEABLE); + image->StoreStructure(pDictionary, GetInstAndDictSize_Unsafe(), DataImage::ITEM_DICTIONARY_WRITEABLE); } } @@ -4445,9 +4464,9 @@ BOOL MethodTable::ComputeNeedsRestoreWorker(DataImage *image, TypeHandleList *pV } // Now check if the dictionary (if any) owned by this methodtable needs a restore. - if (GetDictionary()) + if (GetDictionary_Unsafe()) { - if (GetDictionary()->ComputeNeedsRestore(image, pVisited, GetNumGenericArgs())) + if (GetDictionary_Unsafe()->ComputeNeedsRestore(image, pVisited, GetNumGenericArgs())) { UPDATE_RESTORE_REASON(GenericsDictionaryNeedsRestore); return TRUE; @@ -4969,7 +4988,7 @@ void MethodTable::Fixup(DataImage *image) // // Fixup instantiation+dictionary for this method table (if any) // - if (GetDictionary()) + if (GetDictionary_Unsafe()) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Fixup dictionary for MT %s\n", GetDebugClassName())); @@ -4978,12 +4997,12 @@ void MethodTable::Fixup(DataImage *image) BOOL canSaveSlots = !IsCanonicalMethodTable() && (image->GetModule() == GetCanonicalMethodTable()->GetLoaderModule()); // See comment on Dictionary::Fixup - GetDictionary()->Fixup(image, + GetDictionary_Unsafe()->Fixup(image, TRUE, canSaveSlots, GetNumGenericArgs(), GetModule(), - GetClass()->GetDictionaryLayout()); + GetClass()->GetDictionaryLayout_Unsafe()); } // Fixup per-inst statics info @@ -6118,7 +6137,7 @@ BOOL MethodTable::IsWinRTObjectType() //========================================================================================== // Return a pointer to the dictionary for an instantiated type // Return NULL if not instantiated -PTR_Dictionary MethodTable::GetDictionary() +PTR_Dictionary MethodTable::GetDictionary_Unsafe() { LIMITED_METHOD_DAC_CONTRACT; @@ -9429,9 +9448,9 @@ MethodTable::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) DacEnumMemoryRegion(dac_cast(GetPerInstInfo()) - sizeof(GenericsDictInfo), GetPerInstInfoSize() + sizeof(GenericsDictInfo)); } - if (GetDictionary() != NULL) + if (GetDictionary_Unsafe() != NULL) { - DacEnumMemoryRegion(dac_cast(GetDictionary()), GetInstAndDictSize()); + DacEnumMemoryRegion(dac_cast(GetDictionary_Unsafe()), GetInstAndDictSize_Unsafe()); } VtableIndirectionSlotIterator it = IterateVtableIndirectionSlots(); diff --git a/src/vm/methodtable.h b/src/vm/methodtable.h index 2a7b96781d36..ccb2219f8f2b 100644 --- a/src/vm/methodtable.h +++ b/src/vm/methodtable.h @@ -2907,7 +2907,7 @@ class MethodTable // Advantage: no need to deallocate when growing dictionaries. // Disadvantage: more indirections required at run-time.) // - // The layout of dictionaries is determined by GetClass()->GetDictionaryLayout() + // The layout of dictionaries is determined by GetClass()->GetDictionaryLayout_Unsafe() // Thus the layout can vary between incompatible instantiations. This is sometimes useful because individual type // parameters may or may not be shared. For example, consider a two parameter class Dict. In instantiations shared with // Dict any reference to K is known at JIT-compile-time (it's double) but any token containing D @@ -2957,6 +2957,9 @@ class MethodTable pInfo->m_wNumDicts = numDicts; pInfo->m_wNumTyPars = numTyPars; } + + DWORD GetDictionarySlotsSize(); + #endif // !DACCESS_COMPILE PTR_GenericsDictInfo GetGenericsDictInfo() { @@ -2968,7 +2971,9 @@ class MethodTable // Get a pointer to the dictionary for this instantiated type // (The instantiation is stored in the initial slots of the dictionary) // If not instantiated, return NULL - PTR_Dictionary GetDictionary(); + // This operation is not multi-threaded safe: other thread can update the + // dictionary pointer during a dictionary size expansion. + PTR_Dictionary GetDictionary_Unsafe(); #ifdef FEATURE_PREJIT // @@ -4072,8 +4077,11 @@ public : inline DWORD GetInterfaceMapSize(); // The instantiation/dictionary comes at the end of the MethodTable after - // the interface map. - inline DWORD GetInstAndDictSize(); + // the interface map. + // This operation is not multi-threaded safe: it uses the dictionary layout to compute + // the size, and the dictionary layout can be updated by other threads in the case of a + // generic dictionary size expansion. + inline DWORD GetInstAndDictSize_Unsafe(); private: // Helper template to compute the offsets at compile time diff --git a/src/vm/methodtable.inl b/src/vm/methodtable.inl index 023a3fe2dfe9..f99d293e9d91 100644 --- a/src/vm/methodtable.inl +++ b/src/vm/methodtable.inl @@ -1254,14 +1254,14 @@ inline DWORD MethodTable::GetInterfaceMapSize() //========================================================================================== // These are the generic dictionaries themselves and are come after // the interface map. In principle they need not be inline in the method table. -inline DWORD MethodTable::GetInstAndDictSize() +inline DWORD MethodTable::GetInstAndDictSize_Unsafe() { LIMITED_METHOD_DAC_CONTRACT; if (!HasInstantiation()) return 0; else - return DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericArgs(), GetClass()->GetDictionaryLayout()); + return DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericArgs(), GetClass()->GetDictionaryLayout_Unsafe()); } //========================================================================================== diff --git a/src/vm/methodtablebuilder.cpp b/src/vm/methodtablebuilder.cpp index 82518eafdcb7..36be8ccf176a 100644 --- a/src/vm/methodtablebuilder.cpp +++ b/src/vm/methodtablebuilder.cpp @@ -10164,7 +10164,7 @@ MethodTableBuilder::SetupMethodTable2( DWORD cbDict = bmtGenerics->HasInstantiation() ? DictionaryLayout::GetFirstDictionaryBucketSize( - bmtGenerics->GetNumGenericArgs(), pClass->GetDictionaryLayout()) + bmtGenerics->GetNumGenericArgs(), pClass->GetDictionaryLayout_Unsafe()) : 0; #ifdef FEATURE_COLLECTIBLE_TYPES @@ -10461,6 +10461,13 @@ MethodTableBuilder::SetupMethodTable2( { pInstDest[j] = inst[j]; } + + if (pClass->GetDictionaryLayout_Unsafe() != NULL && pClass->GetDictionaryLayout_Unsafe()->GetMaxSlots() > 0) + { + UINT_PTR* pDictionarySlots = (UINT_PTR*)pMT->GetPerInstInfo()[bmtGenerics->numDicts - 1].GetValue(); + UINT_PTR* pSizeSlot = pDictionarySlots + bmtGenerics->GetNumGenericArgs(); + *pSizeSlot = cbDict; + } } CorElementType normalizedType = ELEMENT_TYPE_CLASS; From 90e26695416d24770958cd13dd6d9e18e6f61817 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 19 Sep 2019 11:13:29 -0700 Subject: [PATCH 11/21] Code review feedback changes --- src/debug/daccess/nidump.cpp | 14 +++++------ src/vm/ceeload.cpp | 36 +++++++++++++++++----------- src/vm/class.cpp | 10 ++++---- src/vm/class.h | 2 +- src/vm/genericdict.cpp | 44 +++++++++++++++++++++-------------- src/vm/genericdict.h | 5 ++-- src/vm/generics.cpp | 8 +++---- src/vm/genmeth.cpp | 20 +++++++--------- src/vm/method.cpp | 28 +++++++++++----------- src/vm/method.hpp | 12 +++++----- src/vm/methodtable.cpp | 44 +++++++++++++++++------------------ src/vm/methodtable.h | 6 ++--- src/vm/methodtable.inl | 4 ++-- src/vm/methodtablebuilder.cpp | 10 ++++---- 14 files changed, 129 insertions(+), 114 deletions(-) diff --git a/src/debug/daccess/nidump.cpp b/src/debug/daccess/nidump.cpp index 2079b261640a..818eafd0ea6c 100644 --- a/src/debug/daccess/nidump.cpp +++ b/src/debug/daccess/nidump.cpp @@ -6562,7 +6562,7 @@ void NativeImageDumper::MethodDescToString( PTR_MethodDesc md, { PTR_InstantiatedMethodDesc imd(PTR_TO_TADDR(md)); unsigned numArgs = imd->m_wNumGenericArgs; - PTR_Dictionary dict(imd->IMD_GetMethodDictionary_Unsafe()); + PTR_Dictionary dict(imd->IMD_GetMethodDictionary()); if( dict != NULL ) { DictionaryToArgString( dict, numArgs, tempName ); @@ -6812,7 +6812,7 @@ NativeImageDumper::DumpMethodTable( PTR_MethodTable mt, const char * name, if( !mt->IsCanonicalMethodTable() && CORCOMPILE_IS_POINTER_TAGGED(PTR_TO_TADDR(mt->GetCanonicalMethodTable())) ) { /* REVISIT_TODO Wed 02/01/2006 - * GetExtent requires the class in order to compute GetInstAndDictSize_Unsafe. + * GetExtent requires the class in order to compute GetInstAndDictSize. * If the EEClass isn't present, I cannot compute the size. If we are * in this case, skip all of the generic dictionaries. */ @@ -6980,19 +6980,19 @@ NativeImageDumper::DumpMethodTable( PTR_MethodTable mt, const char * name, * only print the last one, which is the one for this class). * cloned from Genericdict.cpp */ - PTR_Dictionary currentDictionary(mt->GetDictionary_Unsafe()); + PTR_Dictionary currentDictionary(mt->GetDictionary()); if( currentDictionary != NULL ) { PTR_DictionaryEntry entry(currentDictionary->EntryAddr(0)); - PTR_DictionaryLayout layout( clazz->GetDictionaryLayout_Unsafe() ); + PTR_DictionaryLayout layout( clazz->GetDictionaryLayout() ); DisplayStartStructure( "Dictionary", DPtrToPreferredAddr(currentDictionary), //if there is a layout, use it to compute //the size, otherwise there is just the one //entry. - DictionaryLayout::GetFirstDictionaryBucketSize(mt->GetNumGenericArgs(), layout), + DictionaryLayout::GetDictionarySizeFromLayout(mt->GetNumGenericArgs(), layout), METHODTABLES ); DisplayStartArrayWithOffset( m_pEntries, NULL, Dictionary, @@ -7994,7 +7994,7 @@ void NativeImageDumper::DumpMethodDesc( PTR_MethodDesc md, PTR_Module module ) } //now handle the contents of the m_pMethInst/m_pPerInstInfo union. unsigned numSlots = imd->m_wNumGenericArgs; - PTR_Dictionary inst(imd->IMD_GetMethodDictionary_Unsafe()); + PTR_Dictionary inst(imd->IMD_GetMethodDictionary()); unsigned dictSize; if( kind == InstantiatedMethodDesc::SharedMethodInstantiation ) { @@ -8018,7 +8018,7 @@ void NativeImageDumper::DumpMethodDesc( PTR_MethodDesc md, PTR_Module module ) { PTR_DictionaryLayout layout(wrapped->IsSharedByGenericMethodInstantiations() ? dac_cast(wrapped->GetDictLayoutRaw()) : NULL ); - dictSize = DictionaryLayout::GetFirstDictionaryBucketSize(imd->GetNumGenericMethodArgs(), + dictSize = DictionaryLayout::GetDictionarySizeFromLayout(imd->GetNumGenericMethodArgs(), layout); } } diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index 9d98d5e719f3..bb2bc9192fd0 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -9023,7 +9023,7 @@ void Module::PlaceType(DataImage *image, TypeHandle th, DWORD profilingFlags) } } - Dictionary * pDictionary = pMT->GetDictionary_Unsafe(); + Dictionary * pDictionary = pMT->GetDictionary(); if (pDictionary != NULL) { BOOL fIsWriteable; @@ -9037,7 +9037,7 @@ void Module::PlaceType(DataImage *image, TypeHandle th, DWORD profilingFlags) fIsWriteable = pDictionary->IsWriteable(image, canSaveSlots, pMT->GetNumGenericArgs(), pMT->GetModule(), - pClass->GetDictionaryLayout_Unsafe()); + pClass->GetDictionaryLayout()); } else { @@ -9047,7 +9047,7 @@ void Module::PlaceType(DataImage *image, TypeHandle th, DWORD profilingFlags) if (fIsWriteable) { image->PlaceStructureForAddress(pDictionary, CORCOMPILE_SECTION_HOT_WRITEABLE); - image->PlaceStructureForAddress(pClass->GetDictionaryLayout_Unsafe(), CORCOMPILE_SECTION_WARM); + image->PlaceStructureForAddress(pClass->GetDictionaryLayout(), CORCOMPILE_SECTION_WARM); } else { @@ -13250,10 +13250,10 @@ void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParent } CONTRACTL_END - DictionaryLayout* pDictLayout = pDependencyMT->GetClass()->GetDictionaryLayout_Unsafe(); + DictionaryLayout* pDictLayout = pDependencyMT->GetClass()->GetDictionaryLayout(); if (pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0) { - DWORD sizeFromDictLayout = DictionaryLayout::GetFirstDictionaryBucketSize(pDependencyMT->GetNumGenericArgs(), pDictLayout); + DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pDependencyMT->GetNumGenericArgs(), pDictLayout); if (pDependencyMT->GetDictionarySlotsSize() != sizeFromDictLayout) { _ASSERT(pDependencyMT->GetDictionarySlotsSize() < sizeFromDictLayout); @@ -13289,15 +13289,15 @@ void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD { GC_TRIGGERS; PRECONDITION(CheckPointer(pDependencyMD) && pDependencyMD->HasMethodInstantiation() && pDependencyMD->IsInstantiatingStub()); - PRECONDITION(pDependencyMD->GetDictionaryLayout_Unsafe() != NULL && pDependencyMD->GetDictionaryLayout_Unsafe()->GetMaxSlots() > 0); + PRECONDITION(pDependencyMD->GetDictionaryLayout() != NULL && pDependencyMD->GetDictionaryLayout()->GetMaxSlots() > 0); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END - DictionaryLayout* pDictLayout = pDependencyMD->GetDictionaryLayout_Unsafe(); + DictionaryLayout* pDictLayout = pDependencyMD->GetDictionaryLayout(); InstantiatedMethodDesc* pIMDDependency = pDependencyMD->AsInstantiatedMethodDesc(); - DWORD sizeFromDictLayout = DictionaryLayout::GetFirstDictionaryBucketSize(pDependencyMD->GetNumGenericMethodArgs(), pDictLayout); + DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pDependencyMD->GetNumGenericMethodArgs(), pDictLayout); if (pIMDDependency->GetDictionarySlotsSize() != sizeFromDictLayout) { _ASSERT(pIMDDependency->GetDictionarySlotsSize() < sizeFromDictLayout); @@ -13332,7 +13332,7 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p STANDARD_VM_CHECK; INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); - PRECONDITION(CheckPointer(pMT) && pMT->HasInstantiation() && pMT->GetClass()->GetDictionaryLayout_Unsafe() == pOldLayout); + PRECONDITION(CheckPointer(pMT) && pMT->HasInstantiation() && pMT->GetClass()->GetDictionaryLayout() == pOldLayout); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END @@ -13340,8 +13340,8 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p GCX_COOP(); MethodTable* pCanonMT = pMT->GetCanonicalMethodTable(); - DWORD oldInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMT->GetNumGenericArgs(), pOldLayout); - DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMT->GetNumGenericArgs(), pNewLayout); + DWORD oldInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pOldLayout); + DWORD newInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pNewLayout); // // Dictionary expansion for types needs to be done in multiple steps, given how derived types do not directly embed dictionaries @@ -13360,7 +13360,11 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p }; StackSArray dictionaryUpdates; +#ifdef _DEBUG auto expandPerInstInfos = [oldInfoSize, newInfoSize, &dictionaryUpdates](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) +#else + auto expandPerInstInfos = [newInfoSize, &dictionaryUpdates](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) +#endif { if (!pMTToUpdate->HasSameTypeDefAs(pMTKey)) return true; @@ -13430,7 +13434,7 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* INJECT_FAULT(ThrowOutOfMemory();); PRECONDITION(CheckPointer(pOldLayout) && CheckPointer(pNewLayout)); PRECONDITION(CheckPointer(pMD)); - PRECONDITION(pMD->HasMethodInstantiation() && pMD->GetDictionaryLayout_Unsafe() == pOldLayout); + PRECONDITION(pMD->HasMethodInstantiation() && pMD->GetDictionaryLayout() == pOldLayout); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END @@ -13446,8 +13450,8 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* // MethodDesc* pCanonMD = pMD->IsInstantiatingStub() ? pMD->GetWrappedMethodDesc() : pMD; - DWORD oldInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pOldLayout); - DWORD newInfoSize = DictionaryLayout::GetFirstDictionaryBucketSize(pMD->GetNumGenericMethodArgs(), pNewLayout); + DWORD oldInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pOldLayout); + DWORD newInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pNewLayout); struct NewDictionary { @@ -13456,7 +13460,11 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* }; StackSArray dictionaryUpdates; +#ifdef _DEBUG auto lambda = [oldInfoSize, newInfoSize, &dictionaryUpdates](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) +#else + auto lambda = [newInfoSize, &dictionaryUpdates](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDToUpdate) +#endif { // Update m_pPerInstInfo for the pMethodDesc being visited here _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetWrappedMethodDesc() == pMDKey); diff --git a/src/vm/class.cpp b/src/vm/class.cpp index 42768fcc9ea3..01e789a626a2 100644 --- a/src/vm/class.cpp +++ b/src/vm/class.cpp @@ -901,7 +901,7 @@ ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable *pMT) } - /*if (pParentMT != NULL && pParentMT->HasPerInstInfo()) + if (pParentMT != NULL && pParentMT->HasPerInstInfo()) { // Copy down all inherited dictionary pointers which we // could not embed. @@ -913,7 +913,7 @@ ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable *pMT) pMT->GetPerInstInfo()[iDict].SetValueMaybeNull(pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()); } } - }*/ + } #ifdef FEATURE_PREJIT // Restore action, not in MethodTable::Restore because we may have had approx parents at that point @@ -1014,7 +1014,7 @@ void ClassLoader::RecordDependenciesForDictionaryExpansion(MethodTable* pMT) MethodTable* pParentMT = pMT->GetParentMethodTable(); if (pParentMT != NULL && pParentMT->HasPerInstInfo()) { - // Copy down all inherited dictionary pointers which we could not embed. + // Copy/update down all inherited dictionary pointers which we could not embed. // This step has to be done under the dictionary lock, to prevent other threads from making updates // the the dictionaries of the parent types while we're also copying them over to the derived type here. @@ -2800,7 +2800,7 @@ void EEClass::Save(DataImage *image, MethodTable *pMT) } // Save dictionary layout information - DictionaryLayout *pDictLayout = GetDictionaryLayout_Unsafe(); + DictionaryLayout *pDictLayout = GetDictionaryLayout(); if (pMT->IsSharedByGenericInstantiations() && pDictLayout != NULL) { pDictLayout->Save(image); @@ -2919,7 +2919,7 @@ void EEClass::Fixup(DataImage *image, MethodTable *pMT) } #endif // FEATURE_COMINTEROP - DictionaryLayout *pDictLayout = GetDictionaryLayout_Unsafe(); + DictionaryLayout *pDictLayout = GetDictionaryLayout(); if (pDictLayout != NULL) { pDictLayout->Fixup(image, FALSE); diff --git a/src/vm/class.h b/src/vm/class.h index 1baeb2a13312..4ece86142fa5 100644 --- a/src/vm/class.h +++ b/src/vm/class.h @@ -1799,7 +1799,7 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW! public: // This API is not multi-threaded safe: the dictionary layout pointer can be updated by another // thread during a generic dictionary size expansion. - PTR_DictionaryLayout GetDictionaryLayout_Unsafe() + PTR_DictionaryLayout GetDictionaryLayout() { SUPPORTS_DAC; WRAPPER_NO_CONTRACT; diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index d86a9dcbcdd6..ba268e8f5249 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -73,12 +73,12 @@ DictionaryLayout::Allocate( //--------------------------------------------------------------------------------------- // -// Count the number of bytes that are required by the first bucket in a dictionary with the specified layout -// +// Count the number of bytes that are required in a dictionary with the specified layout +// //static -DWORD -DictionaryLayout::GetFirstDictionaryBucketSize( - DWORD numGenericArgs, +DWORD +DictionaryLayout::GetDictionarySizeFromLayout( + DWORD numGenericArgs, PTR_DictionaryLayout pDictLayout) { LIMITED_METHOD_DAC_CONTRACT; @@ -89,7 +89,7 @@ DictionaryLayout::GetFirstDictionaryBucketSize( if (pDictLayout != NULL) { bytes += sizeof(ULONG_PTR*); // Slot for dictionary size - bytes += pDictLayout->m_numSlots * sizeof(void*); // Slots for diciontary slots based on a dictionary layout + bytes += pDictLayout->m_numSlots * sizeof(void*); // Slots for dictionary slots based on a dictionary layout } return bytes; @@ -269,8 +269,18 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* _ASSERTE(pCurrentDictLayout->m_slots[pCurrentDictLayout->m_numSlots - 1].m_signature != NULL); // Expand the dictionary layout to add more slots - _ASSERTE(FitsIn(pCurrentDictLayout->m_numSlots + NUM_DICTIONARY_SLOTS)); - DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots + NUM_DICTIONARY_SLOTS, pAllocator, NULL); + DWORD newNumSlots = (DWORD)pCurrentDictLayout->m_numSlots * 2; + +#ifdef _DEBUG + // Stress debug mode by increasing size by only 1 slot + if (!FitsIn((DWORD)pCurrentDictLayout->m_numSlots + 1)) + return NULL; + DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots + 1, pAllocator, NULL); +#else + if (!FitsIn((DWORD)pCurrentDictLayout->m_numSlots * 2)) + return NULL; + DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots * 2, pAllocator, NULL); +#endif for (DWORD iSlot = 0; iSlot < pCurrentDictLayout->m_numSlots; iSlot++) pNewDictionaryLayout->m_slots[iSlot] = pCurrentDictLayout->m_slots[iSlot]; @@ -315,18 +325,18 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) + if (FindTokenWorker(pAllocator, pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) return TRUE; CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) + if (FindTokenWorker(pMT->GetLoaderAllocator(), pMT->GetNumGenericArgs(), pMT->GetClass()->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE - DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout_Unsafe(); + DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout(); DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMT->GetNumGenericArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult, pSlotOut); if (pNewLayout == NULL) { @@ -375,17 +385,17 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, 0, FALSE)) return TRUE; CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); { // Try again under lock in case another thread already expanded the dictionaries or filled an empty slot - if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout_Unsafe(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) + if (FindTokenWorker(pAllocator, pMD->GetNumGenericMethodArgs(), pMD->GetDictionaryLayout(), pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, pResult, pSlotOut, *pSlotOut, TRUE)) return TRUE; #ifndef CROSSGEN_COMPILE - DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout_Unsafe(); + DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout(); DictionaryLayout* pNewLayout = ExpandDictionaryLayout(pAllocator, pOldLayout, pMD->GetNumGenericMethodArgs(), pSigBuilder, pSig, nFirstOffset, signatureSource, pResult, pSlotOut); if (pNewLayout == NULL) { @@ -895,7 +905,7 @@ Dictionary::PopulateEntry( CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); // MethodTable is expected to be normalized - Dictionary* pDictionary = pMT->GetDictionary_Unsafe(); + Dictionary* pDictionary = pMT->GetDictionary(); _ASSERTE(pDictionary == pMT->GetPerInstInfo()[dictionaryIndex].GetValueMaybeNull()); #endif } @@ -1378,7 +1388,7 @@ Dictionary::PopulateEntry( // Lock is needed because dictionary pointers can get updated during dictionary size expansion CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary_Unsafe() : pMD->GetMethodDictionary_Unsafe(); + Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary() : pMD->GetMethodDictionary(); *EnsureWritablePages(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); @@ -1398,7 +1408,7 @@ Dictionary::PrepopulateDictionary( { STANDARD_VM_CONTRACT; - DictionaryLayout * pDictLayout = (pMT != NULL) ? pMT->GetClass()->GetDictionaryLayout_Unsafe() : pMD->GetDictionaryLayout_Unsafe(); + DictionaryLayout * pDictLayout = (pMT != NULL) ? pMT->GetClass()->GetDictionaryLayout() : pMD->GetDictionaryLayout(); DWORD numGenericArgs = (pMT != NULL) ? pMT->GetNumGenericArgs() : pMD->GetNumGenericMethodArgs(); if (pDictLayout != NULL) diff --git a/src/vm/genericdict.h b/src/vm/genericdict.h index fbe11205b715..b3e7d73ba87f 100644 --- a/src/vm/genericdict.h +++ b/src/vm/genericdict.h @@ -93,7 +93,6 @@ class DictionaryLayout; typedef DPTR(DictionaryLayout) PTR_DictionaryLayout; // Number of slots to initially allocate in a generic method dictionary layout. -// Also the number of additional slots to add to an existing dictionary layout when expanding its size #if _DEBUG #define NUM_DICTIONARY_SLOTS 1 // Smaller number to stress the dictionary expansion logic #else @@ -145,9 +144,9 @@ class DictionaryLayout // Create an initial dictionary layout with a single bucket containing numSlots slots static DictionaryLayout* Allocate(WORD numSlots, LoaderAllocator *pAllocator, AllocMemTracker *pamTracker); - // Bytes used for the first bucket of this dictionary, which might be stored inline in + // Bytes used for this dictionary, which might be stored inline in // another structure (e.g. MethodTable) - static DWORD GetFirstDictionaryBucketSize(DWORD numGenericArgs, PTR_DictionaryLayout pDictLayout); + static DWORD GetDictionarySizeFromLayout(DWORD numGenericArgs, PTR_DictionaryLayout pDictLayout); static BOOL FindToken(MethodTable* pMT, LoaderAllocator* pAllocator, diff --git a/src/vm/generics.cpp b/src/vm/generics.cpp index 76c1871ce30d..986e2d546e44 100644 --- a/src/vm/generics.cpp +++ b/src/vm/generics.cpp @@ -283,7 +283,7 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( // this rare race condition, right before registering this type for dictionary expansions, we will check that its // dictionary has enough slots to match its dictionary layout if it got updated. // See: Module::RecordTypeForDictionaryExpansion_Locked. - DWORD cbInstAndDict = pOldMT->GetInstAndDictSize_Unsafe(); + DWORD cbInstAndDict = pOldMT->GetInstAndDictSize(); // Allocate from the high frequence heap of the correct domain S_SIZE_T allocSize = safe_cbMT; @@ -493,10 +493,10 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( pInstDest[iArg] = inst[iArg]; } - if (cbInstAndDict != 0 && pOldMT->GetClass()->GetDictionaryLayout_Unsafe() != NULL && pOldMT->GetClass()->GetDictionaryLayout_Unsafe()->GetMaxSlots() > 0) + if (cbInstAndDict != 0 && pOldMT->GetClass()->GetDictionaryLayout() != NULL && pOldMT->GetClass()->GetDictionaryLayout()->GetMaxSlots() > 0) { - UINT_PTR* pDictionarySlots = (UINT_PTR*)pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); - UINT_PTR* pSizeSlot = pDictionarySlots + ntypars; + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); + ULONG_PTR* pSizeSlot = pDictionarySlots + ntypars; *pSizeSlot = cbInstAndDict; } diff --git a/src/vm/genmeth.cpp b/src/vm/genmeth.cpp index 1ad81ddbb7fd..a568c5343caf 100644 --- a/src/vm/genmeth.cpp +++ b/src/vm/genmeth.cpp @@ -383,13 +383,13 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, SString name; TypeString::AppendMethodDebug(name, pGenericMDescInRepMT); LOG((LF_JIT, LL_INFO1000, "GENERICS: Created new dictionary layout for dictionary of size %d for %S\n", - DictionaryLayout::GetFirstDictionaryBucketSize(pGenericMDescInRepMT->GetNumGenericMethodArgs(), pDL), name.GetUnicode())); + DictionaryLayout::GetDictionarySizeFromLayout(pGenericMDescInRepMT->GetNumGenericMethodArgs(), pDL), name.GetUnicode())); } #endif // _DEBUG } // Allocate space for the instantiation and dictionary - infoSize = DictionaryLayout::GetFirstDictionaryBucketSize(methodInst.GetNumArgs(), pDL); + infoSize = DictionaryLayout::GetDictionarySizeFromLayout(methodInst.GetNumArgs(), pDL); pInstOrPerInstInfo = (TypeHandle*)(void*)amt.Track(pAllocator->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(infoSize))); for (DWORD i = 0; i < methodInst.GetNumArgs(); i++) pInstOrPerInstInfo[i] = methodInst[i]; @@ -400,7 +400,7 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, // and the slot with size information. Otherwise, we shouldn't really have a size slot _ASSERTE(infoSize > (sizeof(TypeHandle*) * methodInst.GetNumArgs() + sizeof(ULONG_PTR*))); - UINT_PTR* pDictSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + methodInst.GetNumArgs(); + ULONG_PTR* pDictSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + methodInst.GetNumArgs(); *pDictSizeSlot = infoSize; } } @@ -1448,9 +1448,7 @@ void InstantiatedMethodDesc::SetupGenericMethodDefinition(IMDInternalImport *pIM // the memory allocated for m_pMethInst will be freed if the declaring type fails to load m_pPerInstInfo.SetValue((Dictionary *) pamTracker->Track(pAllocator->GetLowFrequencyHeap()->AllocMem(dwAllocSize))); - // No lock needed here. This is actually a safe operation because dictionary on generic method definitions - // will never be expanded in size. - TypeHandle * pInstDest = (TypeHandle *) IMD_GetMethodDictionaryNonNull_Unsafe(); + TypeHandle * pInstDest = (TypeHandle *) IMD_GetMethodDictionaryNonNull(); for(unsigned int i = 0; i < numTyPars; i++) { hEnumTyPars.EnumNext(&tkTyPar); @@ -1636,10 +1634,10 @@ void MethodDesc::PrepopulateDictionary(DataImage * image, BOOL nonExpansive) STANDARD_VM_CONTRACT; // Note the strong similarity to MethodTable::PrepopulateDictionary - if (GetMethodDictionary_Unsafe()) + if (GetMethodDictionary()) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Prepopulating dictionary for MD %s\n", this)); - GetMethodDictionary_Unsafe()->PrepopulateDictionary(this, NULL, nonExpansive); + GetMethodDictionary()->PrepopulateDictionary(this, NULL, nonExpansive); } } @@ -1724,11 +1722,11 @@ DWORD InstantiatedMethodDesc::GetDictionarySlotsSize() } CONTRACTL_END - UINT_PTR* pDictionarySlots = (UINT_PTR*)IMD_GetMethodDictionary_Unsafe(); + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)IMD_GetMethodDictionary(); if (pDictionarySlots == NULL) return 0; - UINT_PTR* pSizeSlot = pDictionarySlots + m_wNumGenericArgs; - return (DWORD)* pSizeSlot; + ULONG_PTR* pSizeSlot = pDictionarySlots + m_wNumGenericArgs; + return (DWORD)(*pSizeSlot); } #endif // !DACCESS_COMPILE diff --git a/src/vm/method.cpp b/src/vm/method.cpp index 744e6ab175a4..2ba13a7c49df 100644 --- a/src/vm/method.cpp +++ b/src/vm/method.cpp @@ -2395,18 +2395,18 @@ void MethodDesc::Reset() } //******************************************************************************* -Dictionary* MethodDesc::GetMethodDictionary_Unsafe() +Dictionary* MethodDesc::GetMethodDictionary() { WRAPPER_NO_CONTRACT; return (GetClassification() == mcInstantiated) - ? (Dictionary*) (AsInstantiatedMethodDesc()->IMD_GetMethodDictionary_Unsafe()) + ? (Dictionary*) (AsInstantiatedMethodDesc()->IMD_GetMethodDictionary()) : NULL; } //******************************************************************************* -DictionaryLayout* MethodDesc::GetDictionaryLayout_Unsafe() +DictionaryLayout* MethodDesc::GetDictionaryLayout() { WRAPPER_NO_CONTRACT; @@ -2645,10 +2645,10 @@ void MethodDesc::Save(DataImage *image) } } - if (GetMethodDictionary_Unsafe()) + if (GetMethodDictionary()) { - DWORD cBytes = DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericMethodArgs(), GetDictionaryLayout_Unsafe()); - void* pBytes = GetMethodDictionary_Unsafe()->AsPtr(); + DWORD cBytes = DictionaryLayout::GetDictionarySizeFromLayout(GetNumGenericMethodArgs(), GetDictionaryLayout()); + void* pBytes = GetMethodDictionary()->AsPtr(); LOG((LF_ZAP, LL_INFO10000, " MethodDesc::Save dictionary size %d\n", cBytes)); image->StoreStructure(pBytes, cBytes, @@ -2927,9 +2927,9 @@ BOOL MethodDesc::ComputeNeedsRestore(DataImage *image, TypeHandleList *pVisited, return TRUE; } - if (GetMethodDictionary_Unsafe()) + if (GetMethodDictionary()) { - if (GetMethodDictionary_Unsafe()->ComputeNeedsRestore(image, pVisited, GetNumGenericMethodArgs())) + if (GetMethodDictionary()->ComputeNeedsRestore(image, pVisited, GetNumGenericMethodArgs())) return TRUE; } } @@ -3352,14 +3352,14 @@ MethodDesc::Fixup( image->FixupTypeHandlePointer(pInst, &pInst[j]); } } - else if (GetMethodDictionary_Unsafe()) + else if (GetMethodDictionary()) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Fixup dictionary for MD %s\n", m_pszDebugMethodName ? m_pszDebugMethodName : "")); BOOL canSaveInstantiation = TRUE; if (IsGenericMethodDefinition() && !IsTypicalMethodDefinition()) { - if (GetMethodDictionary_Unsafe()->ComputeNeedsRestore(image, NULL, GetNumGenericMethodArgs())) + if (GetMethodDictionary()->ComputeNeedsRestore(image, NULL, GetNumGenericMethodArgs())) { _ASSERTE(needsRestore); canSaveInstantiation = FALSE; @@ -3393,12 +3393,12 @@ MethodDesc::Fixup( pIMD->IMD_IsWrapperStubWithInstantiations() && image->CanEagerBindToMethodDesc(pIMD->IMD_GetWrappedMethodDesc()); - GetMethodDictionary_Unsafe()->Fixup(image, + GetMethodDictionary()->Fixup(image, canSaveInstantiation, canSaveSlots, GetNumGenericMethodArgs(), GetModule(), - GetDictionaryLayout_Unsafe()); + GetDictionaryLayout()); } if (needsRestore) @@ -3993,9 +3993,9 @@ void MethodDesc::CheckRestore(ClassLoadLevel level) Module::RestoreMethodDescPointer(&pIMD->m_pWrappedMethodDesc); // Finally restore the dictionary itself (including instantiation) - if (GetMethodDictionary_Unsafe()) + if (GetMethodDictionary()) { - GetMethodDictionary_Unsafe()->Restore(GetNumGenericMethodArgs(), level); + GetMethodDictionary()->Restore(GetNumGenericMethodArgs(), level); } #else ClassLoader::EnsureLoaded(TypeHandle(GetMethodTable()), level); diff --git a/src/vm/method.hpp b/src/vm/method.hpp index a4ca5f4c4de8..7b113700109b 100644 --- a/src/vm/method.hpp +++ b/src/vm/method.hpp @@ -502,8 +502,8 @@ class MethodDesc // Return NULL if not an instantiated method // These APIs are not multi-threaded safe: the dictionary and dictionary layout pointers // can be updated by other threads during a dictionary size expansion. - Dictionary* GetMethodDictionary_Unsafe(); - DictionaryLayout* GetDictionaryLayout_Unsafe(); + Dictionary* GetMethodDictionary(); + DictionaryLayout* GetDictionaryLayout(); InstantiatedMethodDesc* AsInstantiatedMethodDesc() const; @@ -3473,12 +3473,12 @@ class InstantiatedMethodDesc : public MethodDesc // No lock needed here. This is considered a safe operation here because in the case of a generic dictionary // expansion, the values of the old dictionary slots are copied to the newly allocated dictionary, and the old - // dictionary is kept around, so whether IMD_GetMethodDictionary_Unsafe returns the new or old dictionaries, the + // dictionary is kept around, so whether IMD_GetMethodDictionary returns the new or old dictionaries, the // values of the instantiation arguments will always be the same. - return Instantiation(IMD_GetMethodDictionary_Unsafe()->GetInstantiation(), m_wNumGenericArgs); + return Instantiation(IMD_GetMethodDictionary()->GetInstantiation(), m_wNumGenericArgs); } - PTR_Dictionary IMD_GetMethodDictionary_Unsafe() + PTR_Dictionary IMD_GetMethodDictionary() { LIMITED_METHOD_DAC_CONTRACT; @@ -3489,7 +3489,7 @@ class InstantiatedMethodDesc : public MethodDesc DWORD GetDictionarySlotsSize(); #endif - PTR_Dictionary IMD_GetMethodDictionaryNonNull_Unsafe() + PTR_Dictionary IMD_GetMethodDictionaryNonNull() { LIMITED_METHOD_DAC_CONTRACT; diff --git a/src/vm/methodtable.cpp b/src/vm/methodtable.cpp index 8b877afaa96b..a9ef969bdfa6 100644 --- a/src/vm/methodtable.cpp +++ b/src/vm/methodtable.cpp @@ -484,15 +484,15 @@ DWORD MethodTable::GetDictionarySlotsSize() } CONTRACTL_END - if (!HasPerInstInfo() || GetGenericsDictInfo()->m_wNumTyPars == 0 || GetClass()->GetDictionaryLayout_Unsafe() == NULL) + if (!HasPerInstInfo() || GetGenericsDictInfo()->m_wNumTyPars == 0 || GetClass()->GetDictionaryLayout() == NULL) return 0; - if (GetClass()->GetDictionaryLayout_Unsafe()->GetMaxSlots() == 0) + if (GetClass()->GetDictionaryLayout()->GetMaxSlots() == 0) return 0; - UINT_PTR* pDictionarySlots = (UINT_PTR*)GetPerInstInfo()[GetGenericsDictInfo()->m_wNumDicts - 1].GetValue(); - UINT_PTR* pSizeSlot = pDictionarySlots + GetGenericsDictInfo()->m_wNumTyPars; - return (DWORD)* pSizeSlot; + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)GetPerInstInfo()[GetGenericsDictInfo()->m_wNumDicts - 1].GetValue(); + ULONG_PTR* pSizeSlot = pDictionarySlots + GetGenericsDictInfo()->m_wNumTyPars; + return (DWORD)(*pSizeSlot); } #endif // DACCESS_COMPILE @@ -3991,7 +3991,7 @@ void MethodTable::PrepopulateDictionary(DataImage * image, BOOL nonExpansive) { STANDARD_VM_CONTRACT; - if (GetDictionary_Unsafe()) + if (GetDictionary()) { // We can only save elements of the dictionary if we are sure of its // layout, which means we must be either tightly-knit to the EEClass @@ -4003,7 +4003,7 @@ void MethodTable::PrepopulateDictionary(DataImage * image, BOOL nonExpansive) if (!IsCanonicalMethodTable() && image->CanEagerBindToMethodTable(GetCanonicalMethodTable())) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Prepopulating dictionary for MT %s\n", GetDebugClassName())); - GetDictionary_Unsafe()->PrepopulateDictionary(NULL, this, nonExpansive); + GetDictionary()->PrepopulateDictionary(NULL, this, nonExpansive); } } } @@ -4075,11 +4075,11 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) // Be careful about calling DictionaryLayout::Trim - strict conditions apply. // See note on that method. - if (GetDictionary_Unsafe() && - GetClass()->GetDictionaryLayout_Unsafe() && + if (GetDictionary() && + GetClass()->GetDictionaryLayout() && image->CanEagerBindToMethodTable(GetCanonicalMethodTable())) { - GetClass()->GetDictionaryLayout_Unsafe()->Trim(); + GetClass()->GetDictionaryLayout()->Trim(); } // Set the "restore" flags. They may not have been set yet. @@ -4241,7 +4241,7 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) image->BindPointer(GetPerInstInfo(), pPerInstInfoNode, sizeof(GenericsDictInfo)); } - Dictionary * pDictionary = GetDictionary_Unsafe(); + Dictionary * pDictionary = GetDictionary(); if (pDictionary != NULL) { BOOL fIsWriteable; @@ -4255,7 +4255,7 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) fIsWriteable = pDictionary->IsWriteable(image, canSaveSlots, GetNumGenericArgs(), GetModule(), - GetClass()->GetDictionaryLayout_Unsafe()); + GetClass()->GetDictionaryLayout()); } else { @@ -4265,11 +4265,11 @@ void MethodTable::Save(DataImage *image, DWORD profilingFlags) if (!fIsWriteable) { - image->StoreInternedStructure(pDictionary, GetInstAndDictSize_Unsafe(), DataImage::ITEM_DICTIONARY); + image->StoreInternedStructure(pDictionary, GetInstAndDictSize(), DataImage::ITEM_DICTIONARY); } else { - image->StoreStructure(pDictionary, GetInstAndDictSize_Unsafe(), DataImage::ITEM_DICTIONARY_WRITEABLE); + image->StoreStructure(pDictionary, GetInstAndDictSize(), DataImage::ITEM_DICTIONARY_WRITEABLE); } } @@ -4464,9 +4464,9 @@ BOOL MethodTable::ComputeNeedsRestoreWorker(DataImage *image, TypeHandleList *pV } // Now check if the dictionary (if any) owned by this methodtable needs a restore. - if (GetDictionary_Unsafe()) + if (GetDictionary()) { - if (GetDictionary_Unsafe()->ComputeNeedsRestore(image, pVisited, GetNumGenericArgs())) + if (GetDictionary()->ComputeNeedsRestore(image, pVisited, GetNumGenericArgs())) { UPDATE_RESTORE_REASON(GenericsDictionaryNeedsRestore); return TRUE; @@ -4988,7 +4988,7 @@ void MethodTable::Fixup(DataImage *image) // // Fixup instantiation+dictionary for this method table (if any) // - if (GetDictionary_Unsafe()) + if (GetDictionary()) { LOG((LF_JIT, LL_INFO10000, "GENERICS: Fixup dictionary for MT %s\n", GetDebugClassName())); @@ -4997,12 +4997,12 @@ void MethodTable::Fixup(DataImage *image) BOOL canSaveSlots = !IsCanonicalMethodTable() && (image->GetModule() == GetCanonicalMethodTable()->GetLoaderModule()); // See comment on Dictionary::Fixup - GetDictionary_Unsafe()->Fixup(image, + GetDictionary()->Fixup(image, TRUE, canSaveSlots, GetNumGenericArgs(), GetModule(), - GetClass()->GetDictionaryLayout_Unsafe()); + GetClass()->GetDictionaryLayout()); } // Fixup per-inst statics info @@ -6137,7 +6137,7 @@ BOOL MethodTable::IsWinRTObjectType() //========================================================================================== // Return a pointer to the dictionary for an instantiated type // Return NULL if not instantiated -PTR_Dictionary MethodTable::GetDictionary_Unsafe() +PTR_Dictionary MethodTable::GetDictionary() { LIMITED_METHOD_DAC_CONTRACT; @@ -9448,9 +9448,9 @@ MethodTable::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) DacEnumMemoryRegion(dac_cast(GetPerInstInfo()) - sizeof(GenericsDictInfo), GetPerInstInfoSize() + sizeof(GenericsDictInfo)); } - if (GetDictionary_Unsafe() != NULL) + if (GetDictionary() != NULL) { - DacEnumMemoryRegion(dac_cast(GetDictionary_Unsafe()), GetInstAndDictSize_Unsafe()); + DacEnumMemoryRegion(dac_cast(GetDictionary()), GetInstAndDictSize()); } VtableIndirectionSlotIterator it = IterateVtableIndirectionSlots(); diff --git a/src/vm/methodtable.h b/src/vm/methodtable.h index ccb2219f8f2b..51e47f6ce1e1 100644 --- a/src/vm/methodtable.h +++ b/src/vm/methodtable.h @@ -2907,7 +2907,7 @@ class MethodTable // Advantage: no need to deallocate when growing dictionaries. // Disadvantage: more indirections required at run-time.) // - // The layout of dictionaries is determined by GetClass()->GetDictionaryLayout_Unsafe() + // The layout of dictionaries is determined by GetClass()->GetDictionaryLayout() // Thus the layout can vary between incompatible instantiations. This is sometimes useful because individual type // parameters may or may not be shared. For example, consider a two parameter class Dict. In instantiations shared with // Dict any reference to K is known at JIT-compile-time (it's double) but any token containing D @@ -2973,7 +2973,7 @@ class MethodTable // If not instantiated, return NULL // This operation is not multi-threaded safe: other thread can update the // dictionary pointer during a dictionary size expansion. - PTR_Dictionary GetDictionary_Unsafe(); + PTR_Dictionary GetDictionary(); #ifdef FEATURE_PREJIT // @@ -4081,7 +4081,7 @@ public : // This operation is not multi-threaded safe: it uses the dictionary layout to compute // the size, and the dictionary layout can be updated by other threads in the case of a // generic dictionary size expansion. - inline DWORD GetInstAndDictSize_Unsafe(); + inline DWORD GetInstAndDictSize(); private: // Helper template to compute the offsets at compile time diff --git a/src/vm/methodtable.inl b/src/vm/methodtable.inl index f99d293e9d91..4eb6914627cb 100644 --- a/src/vm/methodtable.inl +++ b/src/vm/methodtable.inl @@ -1254,14 +1254,14 @@ inline DWORD MethodTable::GetInterfaceMapSize() //========================================================================================== // These are the generic dictionaries themselves and are come after // the interface map. In principle they need not be inline in the method table. -inline DWORD MethodTable::GetInstAndDictSize_Unsafe() +inline DWORD MethodTable::GetInstAndDictSize() { LIMITED_METHOD_DAC_CONTRACT; if (!HasInstantiation()) return 0; else - return DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericArgs(), GetClass()->GetDictionaryLayout_Unsafe()); + return DictionaryLayout::GetDictionarySizeFromLayout(GetNumGenericArgs(), GetClass()->GetDictionaryLayout()); } //========================================================================================== diff --git a/src/vm/methodtablebuilder.cpp b/src/vm/methodtablebuilder.cpp index 36be8ccf176a..253c596c8b34 100644 --- a/src/vm/methodtablebuilder.cpp +++ b/src/vm/methodtablebuilder.cpp @@ -10163,8 +10163,8 @@ MethodTableBuilder::SetupMethodTable2( EEClass *pClass = GetHalfBakedClass(); DWORD cbDict = bmtGenerics->HasInstantiation() - ? DictionaryLayout::GetFirstDictionaryBucketSize( - bmtGenerics->GetNumGenericArgs(), pClass->GetDictionaryLayout_Unsafe()) + ? DictionaryLayout::GetDictionarySizeFromLayout( + bmtGenerics->GetNumGenericArgs(), pClass->GetDictionaryLayout()) : 0; #ifdef FEATURE_COLLECTIBLE_TYPES @@ -10462,10 +10462,10 @@ MethodTableBuilder::SetupMethodTable2( pInstDest[j] = inst[j]; } - if (pClass->GetDictionaryLayout_Unsafe() != NULL && pClass->GetDictionaryLayout_Unsafe()->GetMaxSlots() > 0) + if (pClass->GetDictionaryLayout() != NULL && pClass->GetDictionaryLayout()->GetMaxSlots() > 0) { - UINT_PTR* pDictionarySlots = (UINT_PTR*)pMT->GetPerInstInfo()[bmtGenerics->numDicts - 1].GetValue(); - UINT_PTR* pSizeSlot = pDictionarySlots + bmtGenerics->GetNumGenericArgs(); + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[bmtGenerics->numDicts - 1].GetValue(); + ULONG_PTR* pSizeSlot = pDictionarySlots + bmtGenerics->GetNumGenericArgs(); *pSizeSlot = cbDict; } } From d59bb9b500a6e11815a4e59ecd3d8770ae501951 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Tue, 24 Sep 2019 11:46:24 -0700 Subject: [PATCH 12/21] Fix multithreaded race bug with InstantiatedMethodDescs The main problem was that we were publishing InstantiatedMethodDescs before recording them for dictionary expansions, making it possible for other threads to use old dictionary data with expanded slots, and therefore reading incorrect memory locations Fixes include: 1) Recording newly created InstantiatedMethodDescs for dictionary expansion before publishing them 2) Not adding multiple instances of the same method to the expansion hashtable 3) Use FastInterlockedExchange for dictionary pointer updates 4) Fixes around the "pAltMD == pRet" assert: use GetExistingWrappedMethodDesc instead of GetWrappedMethodDesc --- src/vm/ceeload.cpp | 95 ++++++++++++++++++++++++------------------ src/vm/genericdict.cpp | 6 --- src/vm/genmeth.cpp | 38 ++++++++++------- src/vm/method.cpp | 4 +- src/vm/method.hpp | 3 +- 5 files changed, 82 insertions(+), 64 deletions(-) diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index bb2bc9192fd0..b33baaa3b173 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -13239,6 +13239,34 @@ void ReflectionModule::ReleaseILData() Module::ReleaseILData(); } +TypeHandle* AllocateNewMethodDictionaryForExpansion(InstantiatedMethodDesc* pIMD, DWORD cbSize) +{ + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbSize)); + ZeroMemory(pInstOrPerInstInfo, cbSize); + + // Copy old dictionary entry contents + memcpy(pInstOrPerInstInfo, (const void*)pIMD->m_pPerInstInfo.GetValue(), pIMD->GetDictionarySlotsSize()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pIMD->GetNumGenericMethodArgs(); + *pSizeSlot = cbSize; + + return pInstOrPerInstInfo; +} + +TypeHandle* AllocateNewTypeDictionaryForExpansion(MethodTable* pMT, DWORD cbSize) +{ + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbSize)); + ZeroMemory(pInstOrPerInstInfo, cbSize); + + // Copy old dictionary entry contents + memcpy(pInstOrPerInstInfo, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), pMT->GetDictionarySlotsSize()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMT->GetNumGenericArgs(); + *pSizeSlot = cbSize; + + return pInstOrPerInstInfo; +} + void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParentMT, MethodTable* pDependencyMT) { CONTRACTL @@ -13265,17 +13293,11 @@ void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParent // Expand the dictionary slots here before finally adding the type to the hashtable. // - TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pDependencyMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout)); - ZeroMemory(pInstOrPerInstInfo, sizeFromDictLayout); - - // Copy old dictionary entry contents - memcpy(pInstOrPerInstInfo, (const void*)pDependencyMT->GetPerInstInfo()[pDependencyMT->GetNumDicts() - 1].GetValue(), pDependencyMT->GetDictionarySlotsSize()); - - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pDependencyMT->GetNumGenericArgs(); - *pSizeSlot = sizeFromDictLayout; + TypeHandle* pInstOrPerInstInfo = AllocateNewTypeDictionaryForExpansion(pDependencyMT, sizeFromDictLayout); // Publish the new dictionary slots to the type. - pDependencyMT->GetPerInstInfo()[pDependencyMT->GetNumDicts() - 1].SetValue((Dictionary*)pInstOrPerInstInfo); + TypeHandle** pPerInstInfo = (TypeHandle**)pDependencyMT->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + (pDependencyMT->GetNumDicts() - 1), pInstOrPerInstInfo); } } @@ -13309,20 +13331,15 @@ void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD // Expand the dictionary slots here before finally adding the method to the hashtable. // - TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pIMDDependency->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout)); - ZeroMemory(pInstOrPerInstInfo, sizeFromDictLayout); - - // Copy old dictionary entry contents - memcpy(pInstOrPerInstInfo, (const void*)pIMDDependency->m_pPerInstInfo.GetValue(), pIMDDependency->GetDictionarySlotsSize()); - - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pIMDDependency->GetNumGenericMethodArgs(); - *pSizeSlot = sizeFromDictLayout; + TypeHandle* pInstOrPerInstInfo = AllocateNewMethodDictionaryForExpansion(pIMDDependency, sizeFromDictLayout); - pIMDDependency->m_pPerInstInfo.SetValue((Dictionary*)pInstOrPerInstInfo); + FastInterlockExchangePointer((TypeHandle**)pIMDDependency->m_pPerInstInfo.GetValuePtr(), pInstOrPerInstInfo); } GCX_COOP(); - m_dynamicSlotsHashForMethods.Add(pDependencyMD->GetWrappedMethodDesc(), pDependencyMD, GetLoaderAllocator()); + + _ASSERTE(pDependencyMD->GetExistingWrappedMethodDesc() != NULL); + m_dynamicSlotsHashForMethods.Add(pDependencyMD->GetExistingWrappedMethodDesc(), pDependencyMD, GetLoaderAllocator()); } void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout) @@ -13372,14 +13389,7 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey); _ASSERTE(pMTToUpdate->GetDictionarySlotsSize() == oldInfoSize); - TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMTToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); - ZeroMemory(pInstOrPerInstInfo, newInfoSize); - - // Copy old dictionary entry contents - memcpy(pInstOrPerInstInfo, (const void*)pMTToUpdate->GetPerInstInfo()[pMTToUpdate->GetNumDicts() - 1].GetValue(), pMTToUpdate->GetDictionarySlotsSize()); - - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMTToUpdate->GetNumGenericArgs(); - *pSizeSlot = newInfoSize; + TypeHandle* pInstOrPerInstInfo = AllocateNewTypeDictionaryForExpansion(pMTToUpdate, newInfoSize); NewDictionary entry; entry.pMT = pMTToUpdate; @@ -13397,8 +13407,10 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p for (SArray::Iterator i = dictionaryUpdates.Begin(); i != dictionaryUpdates.End(); i++) { MethodTable* pMT = i->pMT; - pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].SetValue((Dictionary*)i->pDictSlots); + TypeHandle** pPerInstInfo = (TypeHandle**)pMT->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + (pMT->GetNumDicts() - 1), i->pDictSlots); _ASSERTE(pMT->GetDictionarySlotsSize() == newInfoSize); + _ASSERTE((TypeHandle*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue() == i->pDictSlots); } auto updateDependentDicts = [](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) @@ -13413,7 +13425,9 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p { DWORD dictToUpdate = pCurrentMT->GetNumDicts() - 1; Dictionary* pUpdatedParentDict = pCurrentMT->GetPerInstInfo()[dictToUpdate].GetValue(); - pMTToUpdate->GetPerInstInfo()[dictToUpdate].SetValue(pUpdatedParentDict); + TypeHandle** pPerInstInfo = (TypeHandle**)pMTToUpdate->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + dictToUpdate, (TypeHandle*)pUpdatedParentDict); + _ASSERTE(pMTToUpdate->GetPerInstInfo()[dictToUpdate].GetValue() == pUpdatedParentDict); return true; // Keep walking } @@ -13424,6 +13438,9 @@ void Module::ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* p }; m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, updateDependentDicts); + + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); } void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout) @@ -13449,7 +13466,8 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* // flushing write buffers // - MethodDesc* pCanonMD = pMD->IsInstantiatingStub() ? pMD->GetWrappedMethodDesc() : pMD; + MethodDesc* pCanonMD = pMD->IsInstantiatingStub() ? pMD->GetExistingWrappedMethodDesc() : pMD; + _ASSERTE(pCanonMD != NULL); DWORD oldInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pOldLayout); DWORD newInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pNewLayout); @@ -13467,19 +13485,12 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* #endif { // Update m_pPerInstInfo for the pMethodDesc being visited here - _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetWrappedMethodDesc() == pMDKey); + _ASSERTE(pMDToUpdate->IsInstantiatingStub() && pMDToUpdate->GetExistingWrappedMethodDesc() == pMDKey); InstantiatedMethodDesc* pInstantiatedMD = pMDToUpdate->AsInstantiatedMethodDesc(); _ASSERTE(pInstantiatedMD->GetDictionarySlotsSize() == oldInfoSize); - TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMDToUpdate->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(newInfoSize)); - ZeroMemory(pInstOrPerInstInfo, newInfoSize); - - // Copy old dictionary entry contents - memcpy(pInstOrPerInstInfo, (const void*)pInstantiatedMD->m_pPerInstInfo.GetValue(), pInstantiatedMD->GetDictionarySlotsSize()); - - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMDToUpdate->GetNumGenericMethodArgs(); - *pSizeSlot = newInfoSize; + TypeHandle* pInstOrPerInstInfo = AllocateNewMethodDictionaryForExpansion(pInstantiatedMD, newInfoSize); NewDictionary entry; entry.pIMD = pInstantiatedMD; @@ -13496,9 +13507,13 @@ void Module::ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* for (SArray::Iterator i = dictionaryUpdates.Begin(); i != dictionaryUpdates.End(); i++) { - i->pIMD->m_pPerInstInfo.SetValue((Dictionary*)i->pDictSlots); + FastInterlockExchangePointer((TypeHandle**)i->pIMD->m_pPerInstInfo.GetValuePtr(), i->pDictSlots); + _ASSERTE((TypeHandle*)i->pIMD->m_pPerInstInfo.GetValue() == i->pDictSlots); _ASSERTE(i->pIMD->GetDictionarySlotsSize() == newInfoSize); } + + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); } #endif // !CROSSGEN_COMPILE diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index ba268e8f5249..80f38d18ef7a 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -347,9 +347,6 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, // First, expand the PerInstInfo dictionaries on types that were using the dictionary layout that just got expanded, and expand their slots pMT->GetModule()->ExpandTypeDictionaries_Locked(pMT, pOldLayout, pNewLayout); - // Ensure no other thread uses old dictionary pointers - FlushProcessWriteBuffers(); - // Finally, update the dictionary layout pointer after all dictionaries of instantiated types have expanded, so that subsequent calls to // DictionaryLayout::FindToken can use this. It is important to update the dictionary layout at the very last step, otherwise some other threads // can start using newly added dictionary layout slots on types where the PerInstInfo hasn't expanded yet, and cause runtime failures. @@ -406,9 +403,6 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, // First, expand the PerInstInfo dictionaries on methods that were using the dictionary layout that just got expanded, and expand their slots pMD->GetModule()->ExpandMethodDictionaries_Locked(pMD, pOldLayout, pNewLayout); - // Ensure no other thread uses old dictionary pointers - FlushProcessWriteBuffers(); - // Finally, update the dictionary layout pointer after all dictionaries of instantiated methods have expanded, so that subsequent calls to // DictionaryLayout::FindToken can use this. It is important to update the dictionary layout at the very last step, otherwise some other threads // can start using newly added dictionary layout slots on methods where the PerInstInfo hasn't expanded yet, and cause runtime failures. diff --git a/src/vm/genmeth.cpp b/src/vm/genmeth.cpp index a568c5343caf..1d48618bb600 100644 --- a/src/vm/genmeth.cpp +++ b/src/vm/genmeth.cpp @@ -296,7 +296,8 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, MethodDesc* pGenericMDescInRepMT, MethodDesc* pWrappedMD, Instantiation methodInst, - BOOL getWrappedCode) + BOOL getWrappedCode, + BOOL recordForDictionaryExpansion) { CONTRACT(InstantiatedMethodDesc*) { @@ -372,6 +373,10 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, { if (pWrappedMD->IsSharedByGenericMethodInstantiations()) { + // It is ok to not take a lock here while reading the dictionary layout pointer. This is because + // when we reach the point of registering the newly created MethodDesc, we take the lock and + // check if the dictionary layout was expanded, and if so, we expand the dictionary of the method + // before recording it for future dictionary expansions and publishing it. pDL = pWrappedMD->AsInstantiatedMethodDesc()->GetDictLayoutRaw(); } } @@ -383,7 +388,7 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, SString name; TypeString::AppendMethodDebug(name, pGenericMDescInRepMT); LOG((LF_JIT, LL_INFO1000, "GENERICS: Created new dictionary layout for dictionary of size %d for %S\n", - DictionaryLayout::GetDictionarySizeFromLayout(pGenericMDescInRepMT->GetNumGenericMethodArgs(), pDL), name.GetUnicode())); + DictionaryLayout::GetDictionarySizeFromLayout(pGenericMDescInRepMT->GetNumGenericMethodArgs(), pDL), name.GetUnicode())); } #endif // _DEBUG } @@ -490,8 +495,8 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, const char* verb = "Created"; if (pWrappedMD) LOG((LF_CLASSLOADER, LL_INFO1000, - "GENERICS: %s instantiating-stub method desc %s with dictionary size %d\n", - verb, pDebugNameUTF8, infoSize)); + "GENERICS: %s instantiating-stub method desc %s with dictionary size %d\n", + verb, pDebugNameUTF8, infoSize)); else LOG((LF_CLASSLOADER, LL_INFO1000, "GENERICS: %s instantiated method desc %s\n", @@ -514,6 +519,15 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, // Verify that we are not creating redundant MethodDescs _ASSERTE(!pNewMD->IsTightlyBoundToMethodTable()); +#ifndef CROSSGEN_COMPILE + if (recordForDictionaryExpansion && pNewMD->HasMethodInstantiation()) + { + // Recording needs to happen before the MD gets published to the hashtable of InstantiatedMethodDescs + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); + pNewMD->GetModule()->RecordMethodForDictionaryExpansion_Locked(pNewMD); + } +#endif + // The method desc is fully set up; now add to the table InstMethodHashTable* pTable = pExactMDLoaderModule->GetInstMethodHashTable(); pTable->InsertMethodDesc(pNewMD); @@ -559,6 +573,7 @@ InstantiatedMethodDesc::FindOrCreateExactClassMethod(MethodTable *pExactMT, pCanonicalMD, pCanonicalMD, Instantiation(), + FALSE, FALSE); } @@ -1158,7 +1173,8 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, NULL, Instantiation(repInst, methodInst.GetNumArgs()), - TRUE); + TRUE, + FALSE); } } else if (getWrappedThenStub) @@ -1193,15 +1209,8 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, pWrappedMD, methodInst, - FALSE); - -#ifndef CROSSGEN_COMPILE - if (pInstMD->HasMethodInstantiation()) - { - CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - pInstMD->GetModule()->RecordMethodForDictionaryExpansion_Locked(pInstMD); - } -#endif + FALSE, + TRUE); } } else @@ -1226,6 +1235,7 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, NULL, methodInst, + FALSE, FALSE); } } diff --git a/src/vm/method.cpp b/src/vm/method.cpp index 2ba13a7c49df..af3b3ca1ba06 100644 --- a/src/vm/method.cpp +++ b/src/vm/method.cpp @@ -1594,9 +1594,7 @@ MethodDesc *MethodDesc::GetWrappedMethodDesc() this->GetMethodTable(), FALSE, /* no unboxing entrypoint */ this->GetMethodInstantiation(), - TRUE, /* get shared code */ - FALSE /* forceRemotableMethod */, - FALSE /* allowCreate */); + TRUE /* get shared code */ ); _ASSERTE(pAltMD == pRet); #endif // _DEBUG return pRet; diff --git a/src/vm/method.hpp b/src/vm/method.hpp index 7b113700109b..35f97143a5a9 100644 --- a/src/vm/method.hpp +++ b/src/vm/method.hpp @@ -3690,7 +3690,8 @@ class InstantiatedMethodDesc : public MethodDesc MethodDesc* pGenericMDescInRepMT, MethodDesc* pSharedMDescForStub, Instantiation methodInst, - BOOL getSharedNotStub); + BOOL getSharedNotStub, + BOOL recordForDictionaryExpansion); }; From cc30cf1008d26251f3197ab5cf23500ff4b5b736 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 2 Oct 2019 16:14:36 -0700 Subject: [PATCH 13/21] Add debug slot with pointer to old dictionary --- src/vm/ceeload.cpp | 14 ++++++++++++++ src/vm/genericdict.cpp | 15 +++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index b33baaa3b173..69e106aa0980 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -13250,6 +13250,11 @@ TypeHandle* AllocateNewMethodDictionaryForExpansion(InstantiatedMethodDesc* pIMD ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pIMD->GetNumGenericMethodArgs(); *pSizeSlot = cbSize; +#ifdef _DEBUG + TypeHandle** ppOldDictSlot = (TypeHandle**)pInstOrPerInstInfo + pIMD->GetNumGenericMethodArgs() + 1; + *ppOldDictSlot = (TypeHandle*)pIMD->m_pPerInstInfo.GetValue(); +#endif + return pInstOrPerInstInfo; } @@ -13264,6 +13269,11 @@ TypeHandle* AllocateNewTypeDictionaryForExpansion(MethodTable* pMT, DWORD cbSize ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMT->GetNumGenericArgs(); *pSizeSlot = cbSize; +#ifdef _DEBUG + TypeHandle** ppOldDictSlot = (TypeHandle**)pInstOrPerInstInfo + pMT->GetNumGenericArgs() + 1; + *ppOldDictSlot = (TypeHandle*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(); +#endif + return pInstOrPerInstInfo; } @@ -13298,6 +13308,8 @@ void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParent // Publish the new dictionary slots to the type. TypeHandle** pPerInstInfo = (TypeHandle**)pDependencyMT->GetPerInstInfo()->GetValuePtr(); FastInterlockExchangePointer(pPerInstInfo + (pDependencyMT->GetNumDicts() - 1), pInstOrPerInstInfo); + + FlushProcessWriteBuffers(); } } @@ -13334,6 +13346,8 @@ void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD TypeHandle* pInstOrPerInstInfo = AllocateNewMethodDictionaryForExpansion(pIMDDependency, sizeFromDictLayout); FastInterlockExchangePointer((TypeHandle**)pIMDDependency->m_pPerInstInfo.GetValuePtr(), pInstOrPerInstInfo); + + FlushProcessWriteBuffers(); } GCX_COOP(); diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index 80f38d18ef7a..5f7a7fbaa183 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -32,7 +32,13 @@ #include "sigbuilder.h" #include "compile.h" -#ifndef DACCESS_COMPILE +#ifdef _DEBUG +#define NUM_DEBUG_SLOTS 1 +#else +#define NUM_DEBUG_SLOTS 0 +#endif + +#ifndef DACCESS_COMPILE //--------------------------------------------------------------------------------------- // @@ -89,6 +95,7 @@ DictionaryLayout::GetDictionarySizeFromLayout( if (pDictLayout != NULL) { bytes += sizeof(ULONG_PTR*); // Slot for dictionary size + bytes += (sizeof(void*) * NUM_DEBUG_SLOTS); // Slots for debug purposes bytes += pDictLayout->m_numSlots * sizeof(void*); // Slots for dictionary slots based on a dictionary layout } @@ -136,8 +143,8 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat CONTRACTL_END // First slots contain the type parameters - _ASSERTE(FitsIn(numGenericArgs + 1 + scanFromSlot)); - WORD slot = static_cast(numGenericArgs + 1 + scanFromSlot); + _ASSERTE(FitsIn(numGenericArgs + 1 + NUM_DEBUG_SLOTS + scanFromSlot)); + WORD slot = static_cast(numGenericArgs + 1 + NUM_DEBUG_SLOTS + scanFromSlot); #if _DEBUG if (scanFromSlot > 0) @@ -286,7 +293,7 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* pNewDictionaryLayout->m_slots[iSlot] = pCurrentDictLayout->m_slots[iSlot]; WORD layoutSlotIndex = pCurrentDictLayout->m_numSlots; - WORD slot = static_cast(numGenericArgs) + 1 + layoutSlotIndex; + WORD slot = static_cast(numGenericArgs) + 1 + NUM_DEBUG_SLOTS + layoutSlotIndex; PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature)) = pResultSignature; From 59ea03ea523f92f84d3e73fb1a91a946fd772888 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 23 Oct 2019 14:53:51 -0700 Subject: [PATCH 14/21] Move type recording to just before the CLASS_LOAD_EXACTPARENTS flag is set. This fixes a race condition found with the final level of type loading, which does not use the typeloader lock used by the other load levels. Added some debug-only checks --- src/vm/ceeload.cpp | 32 ++++++++++++++++++++++++++++++++ src/vm/ceeload.h | 5 +++++ src/vm/class.cpp | 3 +++ src/vm/clsload.cpp | 1 - src/vm/genericdict.cpp | 7 +++++++ 5 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index 69e106aa0980..41ed9c723ac2 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -13277,6 +13277,38 @@ TypeHandle* AllocateNewTypeDictionaryForExpansion(MethodTable* pMT, DWORD cbSize return pInstOrPerInstInfo; } +#ifdef _DEBUG +void Module::EnsureTypeRecorded(MethodTable* pMT) +{ + _ASSERTE(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + + BOOL typeExistsInHashtable = FALSE; + auto lamda = [&typeExistsInHashtable, pMT](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTValue) + { + typeExistsInHashtable = (pMT == pMTValue); + return pMT != pMTValue; + }; + + m_dynamicSlotsHashForTypes.VisitValuesOfKey(pMT->GetCanonicalMethodTable(), lamda); + _ASSERTE(typeExistsInHashtable); +} + +void Module::EnsureMethodRecorded(MethodDesc* pMD) +{ + _ASSERTE(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + + BOOL methodExistsInHashtable = FALSE; + auto lamda = [&methodExistsInHashtable, pMD](OBJECTREF obj, MethodDesc* pMDKey, MethodDesc* pMDValue) + { + methodExistsInHashtable = (pMD== pMDValue); + return pMD != pMDValue; + }; + + m_dynamicSlotsHashForMethods.VisitValuesOfKey(pMD->GetExistingWrappedMethodDesc(), lamda); + _ASSERTE(methodExistsInHashtable); +} +#endif + void Module::RecordTypeForDictionaryExpansion_Locked(MethodTable* pGenericParentMT, MethodTable* pDependencyMT) { CONTRACTL diff --git a/src/vm/ceeload.h b/src/vm/ceeload.h index 25364f1c7c70..139fb144a65a 100644 --- a/src/vm/ceeload.h +++ b/src/vm/ceeload.h @@ -3261,6 +3261,11 @@ class Module void ExpandTypeDictionaries_Locked(MethodTable* pMT, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout); void ExpandMethodDictionaries_Locked(MethodDesc* pMD, DictionaryLayout* pOldLayout, DictionaryLayout* pNewLayout); + +#ifdef _DEBUG + void EnsureTypeRecorded(MethodTable* pMT); + void EnsureMethodRecorded(MethodDesc* pMD); +#endif #endif //!CROSSGEN_COMPILE }; diff --git a/src/vm/class.cpp b/src/vm/class.cpp index 01e789a626a2..b36ae737b1c0 100644 --- a/src/vm/class.cpp +++ b/src/vm/class.cpp @@ -967,6 +967,9 @@ void ClassLoader::LoadExactParents(MethodTable *pMT) MethodTableBuilder::CopyExactParentSlots(pMT, pApproxParentMT); + // Record this type for dynamic dictionary expansion (if applicable) + RecordDependenciesForDictionaryExpansion(pMT); + // We can now mark this type as having exact parents pMT->SetHasExactParent(); diff --git a/src/vm/clsload.cpp b/src/vm/clsload.cpp index ffd165ebe9ba..6dba0d256654 100644 --- a/src/vm/clsload.cpp +++ b/src/vm/clsload.cpp @@ -3219,7 +3219,6 @@ TypeHandle ClassLoader::DoIncrementalLoad(TypeKey *pTypeKey, TypeHandle typeHnd, if (!typeHnd.IsTypeDesc()) { LoadExactParents(typeHnd.AsMethodTable()); - RecordDependenciesForDictionaryExpansion(typeHnd.AsMethodTable()); } break; diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index 5f7a7fbaa183..ca670c5ce3e9 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -1389,6 +1389,13 @@ Dictionary::PopulateEntry( // Lock is needed because dictionary pointers can get updated during dictionary size expansion CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); +#if !defined(CROSSGEN_COMPILE) && defined(_DEBUG) + if (pMT != NULL) + pMT->GetModule()->EnsureTypeRecorded(pMT); + else + pMD->GetModule()->EnsureMethodRecorded(pMD); +#endif + Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary() : pMD->GetMethodDictionary(); *EnsureWritablePages(pDictionary->GetSlotAddr(0, slotIndex)) = result; From cca6f1b93173a3d7b62036d455174392f2ae1a0d Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 30 Oct 2019 14:41:15 -0700 Subject: [PATCH 15/21] Diagnostic pointer to dynamic dictionary precessor. --- src/vm/ceeload.cpp | 26 ++++++++++++-------------- src/vm/genericdict.cpp | 13 +++---------- 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index 41ed9c723ac2..75aae2d59370 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -13241,8 +13241,12 @@ void ReflectionModule::ReleaseILData() TypeHandle* AllocateNewMethodDictionaryForExpansion(InstantiatedMethodDesc* pIMD, DWORD cbSize) { - TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbSize)); - ZeroMemory(pInstOrPerInstInfo, cbSize); + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbSize + sizeof(void*))); + ZeroMemory(pInstOrPerInstInfo, cbSize + sizeof(void*)); + + // Slot[-1] points at previous dictionary to help with diagnostics when investigating crashes + *(byte**)pInstOrPerInstInfo = (byte*)pIMD->m_pPerInstInfo.GetValue() + 1; + pInstOrPerInstInfo++; // Copy old dictionary entry contents memcpy(pInstOrPerInstInfo, (const void*)pIMD->m_pPerInstInfo.GetValue(), pIMD->GetDictionarySlotsSize()); @@ -13250,18 +13254,17 @@ TypeHandle* AllocateNewMethodDictionaryForExpansion(InstantiatedMethodDesc* pIMD ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pIMD->GetNumGenericMethodArgs(); *pSizeSlot = cbSize; -#ifdef _DEBUG - TypeHandle** ppOldDictSlot = (TypeHandle**)pInstOrPerInstInfo + pIMD->GetNumGenericMethodArgs() + 1; - *ppOldDictSlot = (TypeHandle*)pIMD->m_pPerInstInfo.GetValue(); -#endif - return pInstOrPerInstInfo; } TypeHandle* AllocateNewTypeDictionaryForExpansion(MethodTable* pMT, DWORD cbSize) { - TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbSize)); - ZeroMemory(pInstOrPerInstInfo, cbSize); + TypeHandle* pInstOrPerInstInfo = (TypeHandle*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(cbSize + sizeof(void*))); + ZeroMemory(pInstOrPerInstInfo, cbSize + sizeof(void*)); + + // Slot[-1] points at previous dictionary to help with diagnostics when investigating crashes + *(byte**)pInstOrPerInstInfo = (byte*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue() + 1; + pInstOrPerInstInfo++; // Copy old dictionary entry contents memcpy(pInstOrPerInstInfo, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), pMT->GetDictionarySlotsSize()); @@ -13269,11 +13272,6 @@ TypeHandle* AllocateNewTypeDictionaryForExpansion(MethodTable* pMT, DWORD cbSize ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMT->GetNumGenericArgs(); *pSizeSlot = cbSize; -#ifdef _DEBUG - TypeHandle** ppOldDictSlot = (TypeHandle**)pInstOrPerInstInfo + pMT->GetNumGenericArgs() + 1; - *ppOldDictSlot = (TypeHandle*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(); -#endif - return pInstOrPerInstInfo; } diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index ca670c5ce3e9..44d313d52a3b 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -32,12 +32,6 @@ #include "sigbuilder.h" #include "compile.h" -#ifdef _DEBUG -#define NUM_DEBUG_SLOTS 1 -#else -#define NUM_DEBUG_SLOTS 0 -#endif - #ifndef DACCESS_COMPILE //--------------------------------------------------------------------------------------- @@ -95,7 +89,6 @@ DictionaryLayout::GetDictionarySizeFromLayout( if (pDictLayout != NULL) { bytes += sizeof(ULONG_PTR*); // Slot for dictionary size - bytes += (sizeof(void*) * NUM_DEBUG_SLOTS); // Slots for debug purposes bytes += pDictLayout->m_numSlots * sizeof(void*); // Slots for dictionary slots based on a dictionary layout } @@ -143,8 +136,8 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat CONTRACTL_END // First slots contain the type parameters - _ASSERTE(FitsIn(numGenericArgs + 1 + NUM_DEBUG_SLOTS + scanFromSlot)); - WORD slot = static_cast(numGenericArgs + 1 + NUM_DEBUG_SLOTS + scanFromSlot); + _ASSERTE(FitsIn(numGenericArgs + 1 + scanFromSlot)); + WORD slot = static_cast(numGenericArgs + 1 + scanFromSlot); #if _DEBUG if (scanFromSlot > 0) @@ -293,7 +286,7 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* pNewDictionaryLayout->m_slots[iSlot] = pCurrentDictLayout->m_slots[iSlot]; WORD layoutSlotIndex = pCurrentDictLayout->m_numSlots; - WORD slot = static_cast(numGenericArgs) + 1 + NUM_DEBUG_SLOTS + layoutSlotIndex; + WORD slot = static_cast(numGenericArgs) + 1 + layoutSlotIndex; PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature)) = pResultSignature; From 1028e724ff1c7141f7e1a253e5f8028e384cd2e7 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 30 Oct 2019 15:27:23 -0700 Subject: [PATCH 16/21] Fix merge issues --- src/vm/class.cpp | 1 - src/vm/genericdict.cpp | 15 ++++++--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/vm/class.cpp b/src/vm/class.cpp index b36ae737b1c0..9a730c5dcd24 100644 --- a/src/vm/class.cpp +++ b/src/vm/class.cpp @@ -1026,7 +1026,6 @@ void ClassLoader::RecordDependenciesForDictionaryExpansion(MethodTable* pMT) { if (pMT->GetPerInstInfo()[iDict].GetValueMaybeNull() != pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()) { - EnsureWritablePages(&pMT->GetPerInstInfo()[iDict]); pMT->GetPerInstInfo()[iDict].SetValueMaybeNull(pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()); } } diff --git a/src/vm/genericdict.cpp b/src/vm/genericdict.cpp index 44d313d52a3b..f121727dc92a 100644 --- a/src/vm/genericdict.cpp +++ b/src/vm/genericdict.cpp @@ -225,8 +225,8 @@ BOOL DictionaryLayout::FindTokenWorker(LoaderAllocator* pAllocat _ASSERT(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); - *EnsureWritablePages(&(pDictLayout->m_slots[iSlot].m_signature)) = pResultSignature; - *EnsureWritablePages(&(pDictLayout->m_slots[iSlot].m_signatureSource)) = signatureSource; + pDictLayout->m_slots[iSlot].m_signature = pResultSignature; + pDictLayout->m_slots[iSlot].m_signatureSource = signatureSource; pResult->signature = pDictLayout->m_slots[iSlot].m_signature; @@ -268,9 +268,6 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* // There shouldn't be any empty slots remaining in the current dictionary. _ASSERTE(pCurrentDictLayout->m_slots[pCurrentDictLayout->m_numSlots - 1].m_signature != NULL); - // Expand the dictionary layout to add more slots - DWORD newNumSlots = (DWORD)pCurrentDictLayout->m_numSlots * 2; - #ifdef _DEBUG // Stress debug mode by increasing size by only 1 slot if (!FitsIn((DWORD)pCurrentDictLayout->m_numSlots + 1)) @@ -289,8 +286,8 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* WORD slot = static_cast(numGenericArgs) + 1 + layoutSlotIndex; PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); - *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature)) = pResultSignature; - *EnsureWritablePages(&(pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signatureSource)) = signatureSource; + pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature = pResultSignature; + pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signatureSource = signatureSource; pResult->signature = pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature; @@ -527,7 +524,7 @@ DictionaryLayout::Trim() // Trim down the size to what's actually used DWORD dwSlots = GetNumUsedSlots(); _ASSERTE(FitsIn(dwSlots)); - *EnsureWritablePages(&m_numSlots) = static_cast(dwSlots); + m_numSlots = static_cast(dwSlots); } @@ -1391,7 +1388,7 @@ Dictionary::PopulateEntry( Dictionary* pDictionary = pMT != NULL ? pMT->GetDictionary() : pMD->GetMethodDictionary(); - *EnsureWritablePages(pDictionary->GetSlotAddr(0, slotIndex)) = result; + *(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); } } From 78c5b44fab12f42ecdcae640ed988b4ce8d67b9a Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 4 Nov 2019 11:35:19 -0800 Subject: [PATCH 17/21] Adding a chapter in the BOTR describing generic dictionaries --- Documentation/botr/shared-generics.md | 183 ++++++++++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 Documentation/botr/shared-generics.md diff --git a/Documentation/botr/shared-generics.md b/Documentation/botr/shared-generics.md new file mode 100644 index 000000000000..c1cc4b7b58bf --- /dev/null +++ b/Documentation/botr/shared-generics.md @@ -0,0 +1,183 @@ +Shared Generics Design +=== + +Author: Fadi Hanna - 2019 + +# Introduction + +Shared generics is a runtime+JIT feature aimed at reducing the amount of code the runtime generates for generic methods of various instantiations (supports methods on generic types and generic methods). The idea is that for certain instantiations, the generated code will almost be identical with the exception of a few instructions, so in order to reduce the memory footprint, and the amount of time we spend jitting these generic methods, the runtime will generate a single special canonical version of the code, which can be used by all compatible instantiations of the method. + +### Canonical Codegen and Generic Dictionaries + +Consider the following C# code sample: + +``` c# +string Func() +{ + return typeof(List).ToString(); +} +``` + +Without shared generics, the code for instantiations like `Func` or `Func` would look identical except for one single instruction: the one that loads the correct TypeHandle of type `List`: +``` + mov rcx, type handle of List or List + call ToString() + ret +``` + +With shared generics, the canonical code will not have any hard-coded versions of the type handle of List, but instead looks up the exact type handle either through a call to a runtime helper API, or by loading it up from the *generic dictionary* of the instantiation of Func that is executing. The code would look more like the following: +``` asm + mov rcx, generic context // MethodDesc of Func or Func + mov rcx, [rcx + offset of InstantiatedMethodDesc::m_pPerInstInfo] // This is the generic dictionary + mov rcx, [rcx + dictionary slot containing type handle of List] + call ToString() + ret +``` + +The generic context in this example is the InstantiatedMethodDesc of `Func` or `Func`. The generic dictionary is a data structure used by shared generic code to fetch instantiation-specific information. It is basically an array where the entries are instantiation-specific type handles, method handles, field handles, method entry points, etc... The "PerInstInfo" fields on MethodTable and InstantiatedMethodDesc structures point at the generic dictionary structure for a generic type and method respectively. + +In this example, the generic dictionary for Func will contain a slot with the type handle for type List, and the generic dictionary for Func will contain a slot with the type handle for type List. + +This feature is currently only supported for instantiations over reference types because they all have the same size/properties/layout/etc... For instantiations over primitive types or value types, the runtime will generate separate code bodies for each instantiation. + + +# Layouts and Algorithms + +### Dictionaries Pointers on Types and Methods + +The dictionary used by any given generic method is pointed at by the `m_pPerInstInfo` field on the `InstantiatedMethodDesc` structure of that method. It's a direct pointer to the contents of the generic dictionary data. + +On generic types, there's an extra level of indirection: the 'm_pPerInstInfo' field on the `MethodTable` structure is a pointer to a table of dictionaries, and each entry in that table is a pointer to the actual generic dictionary data. This is because types have inheritance, and derived generic types inherit the dictionary pointers of their base types. + +Here's an example: +```c# +class BaseClass { } + +class DerivedClass : BaseClass { } + +class AnotherDerivedClass : DerivedClass { } +``` + +The MethodTables of each of these types will look like the following: + +| **BaseClass[T]'s MethodTable** | +|--------------------------| +| ... | +| `m_PerInstInfo`: points at dictionary table below | +| ... | +| `dictionaryTable[0]`: points at dictionary data below | +| `BaseClass's dictionary data here` | + +| **DerivedClass[U]'s MethodTable ** | +|--------------------------| +| ... | +| `m_PerInstInfo`: points at dictionary table below | +| ... | +| `dictionaryTable[0]`: points at dictionary data of `BaseClass` | +| `dictionaryTable[1]`: points at dictionary data below | +| `DerivedClass's dictionary data here` | + +| **AnotherDerivedClass's MethodTable** | +|--------------------------| +| ... | +| `m_PerInstInfo`: points at dictionary table below | +| ... | +| `dictionaryTable[0]`: points at dictionary data of `BaseClass` | +| `dictionaryTable[1]`: points at dictionary data of `DerivedClass` | + +Note that `AnotherDerivedClass` doesn't have a dictionary of its own given that it is not a generic type, but inherits the dictionary pointers of its base types. + +### Dictionary Slots + +As described earlier, a generic dictionary is an array of multiple slots containing instantiation-specific information. When a dictionary is initially allocated for a certain generic type or method, all of its slots are initialized to NULL, and are lazily populated on demand as code executes (see: `Dictionary::PopulateEntry(...)`). + +The first N slots in an instantiation of N arguments are always going to be the type handles of the instantiation type arguments (this is kind of an optimization as well). The slots that follow contain instantiation-based information. + +For instance, here is an example of the contents of the generic dictionary for our `Func` example: + +| `Func's dicionary` | +|--------------------------| +| slot[0]: TypeHandle(`string`) | +| slot[1]: Total dictionary size | +| slot[2]: TypeHandle(`List`) | +| slot[3]: NULL (not used) | +| slot[4]: NULL (not used) | + +Note: the size slot is never used by generic code, and is part of the dynamic dictionary expansion feature. More on that below. + +When this dictionary is first allocated, only slot[0] is initialized because it contains the instantiation type arguments (and of course the size slot after the dictionary expansion feature), but the rest of the slots (example slot[2]) are NULL, and get lazily populated with values if we ever hit a code path that attempts to use them. + +When loading information from a slot that is still NULL, the generic code will call one of these runtime helper functions to populate the dictionary slot with a value: +- `JIT_GenericHandleClass`: Used to lookup a value in a generic type dictionary. This helper is used by all instance methods on generic types. +- `JIT_GenericHandleMethod`: Used to lookup a value in a generic method dictionary. This helper used by all generic methods, or non-generic static methods on generic types. + +When generating shared generic code, the JIT knows which slots to use for the various lookups, and the kind of information contained in each slot using the help of the `DictionaryLayout` implementation (https://github.com/dotnet/coreclr/blob/master/src/vm/genericdict.cpp). + +### Dictionary Layouts + +The `DictionaryLayout` structure is what tells the JIT which slot to use when performing a dictionary lookup. This `DictionaryLayout` structure has a couple of important properties: +- It is shared accross all compatible instantiations of a certain type of method. In other words, a dictionary layout is associated with the canonical instantiation of a type or a method. For instance, in our example above, `Func` and `Func` are compatible instantiations, each with their own **separate dictionaries**, however they all share the **same dictionary layout**, which is associated with the canonical instantiation `Func<__Canon>`. +- The dictionaries of generic types or methods have the same number of slots as their dictionary layouts. Note: historically before the introduction of the dynamic dictionary expansion feature, the generic dictionaries could be smaller than their layouts, meaning that for certain lookups, we had to use invoke some runtime helper APIs (slow path). + +When a generic type or method is first created, its dictionary layout contains 'unassigned' slots. Assignments happen as part of code generation, whenever the JIT needs to emit a dictionary lookup sequence. This assignment happens during the calls to the `DictionaryLayout::FindToken(...)` APIs. Once a slot has been assigned, it becomes associated with a certain signature, which describes the kind of value that will go in every instantiatied dictionary at that slot index. + +Given an input signature, slot assignment is performed with the following algorithm: + +``` +Begin with slot = 0 +Foreach entry in dictionary layout + If entry.signature != NULL + If entry.signature == inputSignature + return slot + EndIf + Else + entry.signature = inputSignature + return slot + EndIf + slot++ +EndForeach +``` + +So what happens when the above algorithm runs, but no existing slot with the same signature is found, and we're out of 'unassigned' slots? This is where the dynamic dictionary expansion kicks in to resize the layout by adding more slots to it, and resizing all dictionaries associated with this layout. + +# Dynamic Dictionary Expansion + +### History + +Before the dynamic dictionary expansion feature, dictionary layouts were organized into buckets (a linked list of fixed-size `DictionaryLayout` structures). The size of the initial layout bucket was always fixed to some number which was computed based on some heuristics for generic types, and always fixed to 4 slots for generic methods. The generic types and methods also had fixed-size generic dictionaries which could be used for lookups (also known as "fast lookup slots"). + +When a bucket gets filled with entries, we would just allocate a new `DictionaryLayout` bucket, and add it to the list. The problem however is that we couldn't resize the generic dictionaries of types or methods, because they have already been allocated with a fixed size, and the JIT does not support generating instructions that could indirect into a linked-list of dictionaries. Given that limitation, we could only lookup a generic dictionary for a fixed number of values (the ones associated with the entries of the first `DictionaryLayout` bucket), and were forced to go through a slower runtime helper for additional lookups. + +This was acceptable, until we introduced the [ReadyToRun](https://github.com/dotnet/coreclr/blob/master/Documentation/botr/readytorun-overview.md) and the Tiered Compilation technologies. Slots were getting assigned quickly when used by ReadyToRun code, and when the runtime decided re-jitted certain methods for better performance, it could not in some cases find any remaining "fast lookup slots", and was forced to generate code that goes through the slower runtime helpers. This ended up hurting performance in some scenarios, and a decision was made to not use the fast lookup slots for ReadyToRun code, and instead keep them reserved for re-jitted code. This decision however hurt the ReadyToRun performance, but it was a necessary compromise since we cared more about re-jitted code throughput over R2R throughput. + +For this reason, the dynamic dictionary expansion feature was introduced. + +### Description and Algorithms + +The feature is simple in concept: change dictionary layouts from a linked list of buckets into dynamically expandable arrays instead. Sounds simple, but great care had to be taken when impementing it, because: +- We can't just resize `DictionaryLayout` structures alone. If the size of the layout is larger than the size of the actual generic dictionary, this would cause the JIT to generate indirection instructions that do not match the size of the dictionary data, leading to access violations. +- We can't just resize generic dictionaries on types and methods: + - For types, the generic dictionary is part of the `MethodTable` structure, which can't be reallocated (already in use by managed code) + - For methods, the generic dictionary is not part of the `MethodDesc` structure, but can still be in use by some generic code. + - We can't have multiple MethodTables or MethodDescs for the same type or method anyways, so reallocations are not an option. +- We can't just resize the generic dictionary for a single instantiation. For instance, in our example above, let's say we wanted to expand the dictionary for `Func`. The resizing of the layout would have an impact on the shared canonical code that the JIT generates for `Func<__Canon>`. If we only resized the dictionary of `Func`, the shared generic code would work for that instantiation only, but when we attempt to use it with another instantiation like `Func`, the jitted instructions would no longer match the size of the dictionary structure, and would cause access violations. +- The runtime is multithreaded, which adds to the complexity. + +The first step in this feature is to insert all generic types and methods with dictionaries into a hashtable, where the key is the canonical instantiation. For instance, with our example, `Func` and `Func` would be added to the hashtable as values under the `Func<__Canon>` key. This ensures that if we ever need to resize the dictionary layout, we would have a way of finding all existing instantiations to resize their dictionaries as well (remember, a dictionary size has to match the size of the layout now). This is achieved by calls to the `Module::RecordTypeForDictionaryExpansion_Locked` and `Module::RecordMethodForDictionaryExpansion_Locked` APIs, every time a new generic type or method is created, just before they get published for usage by other threads. + +Resizing of the dictionary layouts takes place in `DictionaryLayout::ExpandDictionaryLayout`. A new `DictionaryLayout` structure is allocated with a larger size, and the contents of the old layout are copied over. At this point, we **cannot yet** associate that new layout with the canonical instantiation: we need to resize the dictionaries of all related instantiations (because of multi-threading). + +Resizing of the dictionaries of all related types or methods takes place in `Module::ExpandTypeDictionaries_Locked` and `Module::ExpandMethodDictionaries_Locked`. New dictionaries are allocated for each affected type or method, and the contents of their old dictionaries are copied over. These new dictionaries then get published on the corresponding `MethodTable` or `InstantiatedMethodDesc` structures (the "PerInstInfo" field). Great care is taken to perform all the dictionary allocations and initializations first before publishing them, with a call to `FlushProcessWriteBuffers()` in the middle to ensure correct ordering of read/write operations in multi-threading. + +Finally, after resizing all generic dictionaries, the last step is to publish the newly allocated `DictionaryLayout` structure by associating it with the canonical instantiation. + +### Diagnostics + +During feature development, an interesting set of bugs were hit, all having to do with multi-threaded executions. The main root cause behind these bugs was that some threads started to make use of newly allocated generic MethodTables or MethodDescs, and started to expand their dictionary layouts before we ever got a chance to insert these new types/methods into the hashtable to correctly track them for dictionary resizing. In other words, some thread was still in the process of constructing these MethodTables/MethodDescs, got them to a usable state and published them, making them available for other threads to start using, but did not yet reach the point of recording them into the hashtable of dictionary expansion tracking. The effect is that some shared generic code started accessing slots beyond the size limits of the generic dictionaries of these types/methods, causing access violations. + +The most useful piece of data that made it easy to diagnose these access violations was a pointer in each dynamically allocated dictionary to its predecessor. Tracking back dictionaries using these pointers led to the location in memory where the incorrect lookup value was loaded from, and helped root cause the bug. + +These predecessor pointers are allocated at the begining of each dynamically allocated dictionary, but are not part of the dictionary itself (so think of it as slot[-1]). + +The plan is to also add an SOS command that could help diagnose dictionary contents accross the chain of dynamically allocated dictionaries (dotnet/diagnostics#588). + From e06136eba50078ec9701335e76593d283418b62b Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 4 Nov 2019 11:50:40 -0800 Subject: [PATCH 18/21] Update shared-generics.md asm formatting --- Documentation/botr/shared-generics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/botr/shared-generics.md b/Documentation/botr/shared-generics.md index c1cc4b7b58bf..a9f0f5e29906 100644 --- a/Documentation/botr/shared-generics.md +++ b/Documentation/botr/shared-generics.md @@ -19,7 +19,7 @@ string Func() ``` Without shared generics, the code for instantiations like `Func` or `Func` would look identical except for one single instruction: the one that loads the correct TypeHandle of type `List`: -``` +``` asm mov rcx, type handle of List or List call ToString() ret From cd7a18fde813522211c750dfdb1b3bea2bd774ba Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 4 Nov 2019 11:59:07 -0800 Subject: [PATCH 19/21] Update shared-generics.md Note on old dictionaries not getting deallocated --- Documentation/botr/shared-generics.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/botr/shared-generics.md b/Documentation/botr/shared-generics.md index a9f0f5e29906..c2fa0a21e1fe 100644 --- a/Documentation/botr/shared-generics.md +++ b/Documentation/botr/shared-generics.md @@ -169,6 +169,8 @@ Resizing of the dictionary layouts takes place in `DictionaryLayout::ExpandDicti Resizing of the dictionaries of all related types or methods takes place in `Module::ExpandTypeDictionaries_Locked` and `Module::ExpandMethodDictionaries_Locked`. New dictionaries are allocated for each affected type or method, and the contents of their old dictionaries are copied over. These new dictionaries then get published on the corresponding `MethodTable` or `InstantiatedMethodDesc` structures (the "PerInstInfo" field). Great care is taken to perform all the dictionary allocations and initializations first before publishing them, with a call to `FlushProcessWriteBuffers()` in the middle to ensure correct ordering of read/write operations in multi-threading. +One thing to note is that old dictionaries are not deallocated, but once a new dictionary gets published on a MethodTable or MethodDesc, any subsequent dictionary lookup by generic code will make use of that newly allocated dictionary. Deallocating old dictionaries would be extremely complicated, especially in a multi-threaded environment, and won't give any useful benefit. + Finally, after resizing all generic dictionaries, the last step is to publish the newly allocated `DictionaryLayout` structure by associating it with the canonical instantiation. ### Diagnostics From 1e0439d77b65759ed4ecd00358f64250ddca813c Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Tue, 5 Nov 2019 14:37:38 -0800 Subject: [PATCH 20/21] Update shared-generics.md Note on thread synchronization --- Documentation/botr/shared-generics.md | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/Documentation/botr/shared-generics.md b/Documentation/botr/shared-generics.md index c2fa0a21e1fe..bcf8f22af01b 100644 --- a/Documentation/botr/shared-generics.md +++ b/Documentation/botr/shared-generics.md @@ -152,7 +152,7 @@ This was acceptable, until we introduced the [ReadyToRun](https://github.com/dot For this reason, the dynamic dictionary expansion feature was introduced. -### Description and Algorithms +### Description The feature is simple in concept: change dictionary layouts from a linked list of buckets into dynamically expandable arrays instead. Sounds simple, but great care had to be taken when impementing it, because: - We can't just resize `DictionaryLayout` structures alone. If the size of the layout is larger than the size of the actual generic dictionary, this would cause the JIT to generate indirection instructions that do not match the size of the dictionary data, leading to access violations. @@ -163,16 +163,38 @@ The feature is simple in concept: change dictionary layouts from a linked list o - We can't just resize the generic dictionary for a single instantiation. For instance, in our example above, let's say we wanted to expand the dictionary for `Func`. The resizing of the layout would have an impact on the shared canonical code that the JIT generates for `Func<__Canon>`. If we only resized the dictionary of `Func`, the shared generic code would work for that instantiation only, but when we attempt to use it with another instantiation like `Func`, the jitted instructions would no longer match the size of the dictionary structure, and would cause access violations. - The runtime is multithreaded, which adds to the complexity. +### Expansion Algorithm + +##### Step 1 - Type and Method Registration + The first step in this feature is to insert all generic types and methods with dictionaries into a hashtable, where the key is the canonical instantiation. For instance, with our example, `Func` and `Func` would be added to the hashtable as values under the `Func<__Canon>` key. This ensures that if we ever need to resize the dictionary layout, we would have a way of finding all existing instantiations to resize their dictionaries as well (remember, a dictionary size has to match the size of the layout now). This is achieved by calls to the `Module::RecordTypeForDictionaryExpansion_Locked` and `Module::RecordMethodForDictionaryExpansion_Locked` APIs, every time a new generic type or method is created, just before they get published for usage by other threads. +##### Step 2 - Dictionary Layout Expansion + Resizing of the dictionary layouts takes place in `DictionaryLayout::ExpandDictionaryLayout`. A new `DictionaryLayout` structure is allocated with a larger size, and the contents of the old layout are copied over. At this point, we **cannot yet** associate that new layout with the canonical instantiation: we need to resize the dictionaries of all related instantiations (because of multi-threading). +##### Step 3 - Type and Method Dictionaries Expansion + Resizing of the dictionaries of all related types or methods takes place in `Module::ExpandTypeDictionaries_Locked` and `Module::ExpandMethodDictionaries_Locked`. New dictionaries are allocated for each affected type or method, and the contents of their old dictionaries are copied over. These new dictionaries then get published on the corresponding `MethodTable` or `InstantiatedMethodDesc` structures (the "PerInstInfo" field). Great care is taken to perform all the dictionary allocations and initializations first before publishing them, with a call to `FlushProcessWriteBuffers()` in the middle to ensure correct ordering of read/write operations in multi-threading. One thing to note is that old dictionaries are not deallocated, but once a new dictionary gets published on a MethodTable or MethodDesc, any subsequent dictionary lookup by generic code will make use of that newly allocated dictionary. Deallocating old dictionaries would be extremely complicated, especially in a multi-threaded environment, and won't give any useful benefit. +##### Step 4 - Publishing New Dictionary Layout + Finally, after resizing all generic dictionaries, the last step is to publish the newly allocated `DictionaryLayout` structure by associating it with the canonical instantiation. +##### Note on Thread Synchronization + +Thread synchronization is done by protecting all places where dictionaries are read/written in a meaninful way using a critical section implementation: +``` c++ +CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); +``` + +Here is the list of places that require synchronization: +- Before recording any new generic type or method into hashtable to track them for dictionary expansions (`RecordTypeForDictionaryExpansion_Locked` and `RecordMethodForDictionaryExpansion_Locked`). This prevents any expansion from taking place on another thread before we get a chance to add the type/method into the hashtable and track them. +- Before any unassigned slot in a DictionaryLayout structure gets assigned a certain lookup kind/signature (see `DictionaryLayout::FindToken` implementations). No two threads are allowed to update a DictionaryLayout structure at the same time. +- Before any uninitialized dictionary slot gets populated with a value as a result of a dictionary lookup (see `Dictionary::PopulateEntry`). This is because during expansion, the contents of an existing dictionary are copied over to the newly allocated one, so the lock synchronizes read and write operations. + ### Diagnostics During feature development, an interesting set of bugs were hit, all having to do with multi-threaded executions. The main root cause behind these bugs was that some threads started to make use of newly allocated generic MethodTables or MethodDescs, and started to expand their dictionary layouts before we ever got a chance to insert these new types/methods into the hashtable to correctly track them for dictionary resizing. In other words, some thread was still in the process of constructing these MethodTables/MethodDescs, got them to a usable state and published them, making them available for other threads to start using, but did not yet reach the point of recording them into the hashtable of dictionary expansion tracking. The effect is that some shared generic code started accessing slots beyond the size limits of the generic dictionaries of these types/methods, causing access violations. From 11965aa50fed7fc93a16f11383432f02fab1cde5 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 6 Nov 2019 09:07:49 -0800 Subject: [PATCH 21/21] Update shared-generics.md Feedback from Jan --- Documentation/botr/shared-generics.md | 69 ++++++++++++++------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/Documentation/botr/shared-generics.md b/Documentation/botr/shared-generics.md index bcf8f22af01b..1fd37439b57d 100644 --- a/Documentation/botr/shared-generics.md +++ b/Documentation/botr/shared-generics.md @@ -7,6 +7,8 @@ Author: Fadi Hanna - 2019 Shared generics is a runtime+JIT feature aimed at reducing the amount of code the runtime generates for generic methods of various instantiations (supports methods on generic types and generic methods). The idea is that for certain instantiations, the generated code will almost be identical with the exception of a few instructions, so in order to reduce the memory footprint, and the amount of time we spend jitting these generic methods, the runtime will generate a single special canonical version of the code, which can be used by all compatible instantiations of the method. +More information on the design can be found in the original MSR paper at https://www.microsoft.com/en-us/research/publication/design-and-implementation-of-generics-for-the-net-common-language-runtime. + ### Canonical Codegen and Generic Dictionaries Consider the following C# code sample: @@ -60,30 +62,30 @@ class AnotherDerivedClass : DerivedClass { } The MethodTables of each of these types will look like the following: -| **BaseClass[T]'s MethodTable** | -|--------------------------| -| ... | -| `m_PerInstInfo`: points at dictionary table below | -| ... | -| `dictionaryTable[0]`: points at dictionary data below | -| `BaseClass's dictionary data here` | - -| **DerivedClass[U]'s MethodTable ** | -|--------------------------| -| ... | -| `m_PerInstInfo`: points at dictionary table below | -| ... | -| `dictionaryTable[0]`: points at dictionary data of `BaseClass` | -| `dictionaryTable[1]`: points at dictionary data below | -| `DerivedClass's dictionary data here` | - -| **AnotherDerivedClass's MethodTable** | -|--------------------------| -| ... | +| **BaseClass[T]'s MethodTable** | +|-------------------------------------------------------| +| ... | | `m_PerInstInfo`: points at dictionary table below | -| ... | -| `dictionaryTable[0]`: points at dictionary data of `BaseClass` | -| `dictionaryTable[1]`: points at dictionary data of `DerivedClass` | +| ... | +| `dictionaryTable[0]`: points at dictionary data below | +| `BaseClass's dictionary data here` | + +| **DerivedClass[U]'s MethodTable ** | +|----------------------------------------------------------------| +| ... | +| `m_PerInstInfo`: points at dictionary table below | +| ... | +| `dictionaryTable[0]`: points at dictionary data of `BaseClass` | +| `dictionaryTable[1]`: points at dictionary data below | +| `DerivedClass's dictionary data here` | + +| **AnotherDerivedClass's MethodTable** | +|-------------------------------------------------------------------| +| ... | +| `m_PerInstInfo`: points at dictionary table below | +| ... | +| `dictionaryTable[0]`: points at dictionary data of `BaseClass` | +| `dictionaryTable[1]`: points at dictionary data of `DerivedClass` | Note that `AnotherDerivedClass` doesn't have a dictionary of its own given that it is not a generic type, but inherits the dictionary pointers of its base types. @@ -95,13 +97,13 @@ The first N slots in an instantiation of N arguments are always going to be the For instance, here is an example of the contents of the generic dictionary for our `Func` example: -| `Func's dicionary` | -|--------------------------| -| slot[0]: TypeHandle(`string`) | -| slot[1]: Total dictionary size | +| `Func's dicionary` | +|--------------------------------------| +| slot[0]: TypeHandle(`string`) | +| slot[1]: Total dictionary size | | slot[2]: TypeHandle(`List`) | -| slot[3]: NULL (not used) | -| slot[4]: NULL (not used) | +| slot[3]: NULL (not used) | +| slot[4]: NULL (not used) | Note: the size slot is never used by generic code, and is part of the dynamic dictionary expansion feature. More on that below. @@ -146,9 +148,9 @@ So what happens when the above algorithm runs, but no existing slot with the sam Before the dynamic dictionary expansion feature, dictionary layouts were organized into buckets (a linked list of fixed-size `DictionaryLayout` structures). The size of the initial layout bucket was always fixed to some number which was computed based on some heuristics for generic types, and always fixed to 4 slots for generic methods. The generic types and methods also had fixed-size generic dictionaries which could be used for lookups (also known as "fast lookup slots"). -When a bucket gets filled with entries, we would just allocate a new `DictionaryLayout` bucket, and add it to the list. The problem however is that we couldn't resize the generic dictionaries of types or methods, because they have already been allocated with a fixed size, and the JIT does not support generating instructions that could indirect into a linked-list of dictionaries. Given that limitation, we could only lookup a generic dictionary for a fixed number of values (the ones associated with the entries of the first `DictionaryLayout` bucket), and were forced to go through a slower runtime helper for additional lookups. +When a bucket gets filled with entries, we would just allocate a new `DictionaryLayout` bucket, and add it to the list. The problem however is that we could not resize the generic dictionaries of types or methods, because they had already been allocated with a fixed size, and the JIT does not support generating instructions that could indirect into a linked-list of dictionaries. Given that limitation, we could only lookup a generic dictionary for a fixed number of values (the ones associated with the entries of the first `DictionaryLayout` bucket), and were forced to go through a slower runtime helper for additional lookups. This implementation design is from the time of fragile NGen images, and having expandable dictionary structures would have been very complicated. -This was acceptable, until we introduced the [ReadyToRun](https://github.com/dotnet/coreclr/blob/master/Documentation/botr/readytorun-overview.md) and the Tiered Compilation technologies. Slots were getting assigned quickly when used by ReadyToRun code, and when the runtime decided re-jitted certain methods for better performance, it could not in some cases find any remaining "fast lookup slots", and was forced to generate code that goes through the slower runtime helpers. This ended up hurting performance in some scenarios, and a decision was made to not use the fast lookup slots for ReadyToRun code, and instead keep them reserved for re-jitted code. This decision however hurt the ReadyToRun performance, but it was a necessary compromise since we cared more about re-jitted code throughput over R2R throughput. +When the [ReadyToRun](https://github.com/dotnet/coreclr/blob/master/Documentation/botr/readytorun-overview.md) and the Tiered Compilation technologies were introduced, some performance problems started to show. Dictionary slots were getting assigned quickly when used by ReadyToRun code, and when the runtime decided to re-jit certain methods for better performance, it could not in some cases find any remaining "fast lookup slots", and was forced to generate code that goes through the slower runtime helpers. This ended up hurting performance in some scenarios, and a decision was made to not use the fast lookup slots for ReadyToRun code, and instead keep them reserved for re-jitted code. This decision however hurt the ReadyToRun performance, but it was a necessary compromise since we cared more about re-jitted code throughput over R2R throughput. For this reason, the dynamic dictionary expansion feature was introduced. @@ -157,9 +159,8 @@ For this reason, the dynamic dictionary expansion feature was introduced. The feature is simple in concept: change dictionary layouts from a linked list of buckets into dynamically expandable arrays instead. Sounds simple, but great care had to be taken when impementing it, because: - We can't just resize `DictionaryLayout` structures alone. If the size of the layout is larger than the size of the actual generic dictionary, this would cause the JIT to generate indirection instructions that do not match the size of the dictionary data, leading to access violations. - We can't just resize generic dictionaries on types and methods: - - For types, the generic dictionary is part of the `MethodTable` structure, which can't be reallocated (already in use by managed code) - - For methods, the generic dictionary is not part of the `MethodDesc` structure, but can still be in use by some generic code. - - We can't have multiple MethodTables or MethodDescs for the same type or method anyways, so reallocations are not an option. + - They were already allocated with a fixed number of slots, and cannot be resized in place. + - They can be in use by some generic code. - We can't just resize the generic dictionary for a single instantiation. For instance, in our example above, let's say we wanted to expand the dictionary for `Func`. The resizing of the layout would have an impact on the shared canonical code that the JIT generates for `Func<__Canon>`. If we only resized the dictionary of `Func`, the shared generic code would work for that instantiation only, but when we attempt to use it with another instantiation like `Func`, the jitted instructions would no longer match the size of the dictionary structure, and would cause access violations. - The runtime is multithreaded, which adds to the complexity.