From d711d6937b1125f06d99e06930960cbdf8ebcfb4 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 19 Dec 2019 15:52:08 -0800 Subject: [PATCH 01/11] Original implementation of the dynamic dictionary expansion feature --- docs/design/coreclr/botr/shared-generics.md | 185 ++++++++ src/coreclr/src/debug/daccess/nidump.cpp | 4 +- src/coreclr/src/vm/appdomain.cpp | 8 +- src/coreclr/src/vm/ceeload.cpp | 327 ++++++++++++- src/coreclr/src/vm/ceeload.h | 27 ++ src/coreclr/src/vm/class.cpp | 75 +++ src/coreclr/src/vm/clsload.cpp | 9 +- src/coreclr/src/vm/clsload.hpp | 2 + src/coreclr/src/vm/genericdict.cpp | 485 +++++++++++++------- src/coreclr/src/vm/genericdict.h | 86 ++-- src/coreclr/src/vm/generics.cpp | 12 + src/coreclr/src/vm/genmeth.cpp | 64 ++- src/coreclr/src/vm/jitinterface.cpp | 6 +- src/coreclr/src/vm/method.cpp | 2 +- src/coreclr/src/vm/method.hpp | 30 +- src/coreclr/src/vm/methodtable.cpp | 19 + src/coreclr/src/vm/methodtable.h | 3 + src/coreclr/src/vm/methodtable.inl | 2 +- src/coreclr/src/vm/methodtablebuilder.cpp | 9 +- src/coreclr/src/vm/prestub.cpp | 4 +- 20 files changed, 1120 insertions(+), 239 deletions(-) create mode 100644 docs/design/coreclr/botr/shared-generics.md diff --git a/docs/design/coreclr/botr/shared-generics.md b/docs/design/coreclr/botr/shared-generics.md new file mode 100644 index 0000000000000..c2fa0a21e1fe2 --- /dev/null +++ b/docs/design/coreclr/botr/shared-generics.md @@ -0,0 +1,185 @@ +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`: +``` asm + 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. + +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 + +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). + diff --git a/src/coreclr/src/debug/daccess/nidump.cpp b/src/coreclr/src/debug/daccess/nidump.cpp index 0b23633727570..8d2f216fa15a1 100644 --- a/src/coreclr/src/debug/daccess/nidump.cpp +++ b/src/coreclr/src/debug/daccess/nidump.cpp @@ -6957,7 +6957,7 @@ NativeImageDumper::DumpMethodTable( PTR_MethodTable mt, const char * name, //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, @@ -7983,7 +7983,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/coreclr/src/vm/appdomain.cpp b/src/coreclr/src/vm/appdomain.cpp index 44aa02dad8cfa..0eb122f950a70 100644 --- a/src/coreclr/src/vm/appdomain.cpp +++ b/src/coreclr/src/vm/appdomain.cpp @@ -2004,6 +2004,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); @@ -2019,10 +2023,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/coreclr/src/vm/ceeload.cpp b/src/coreclr/src/vm/ceeload.cpp index 3ff65d67432e5..55fe4b4e4ad58 100644 --- a/src/coreclr/src/vm/ceeload.cpp +++ b/src/coreclr/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()) { @@ -687,8 +688,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 @@ -13204,6 +13209,326 @@ void ReflectionModule::ResumeMetadataCapture() CaptureModuleMetaDataToMemory(); } +TypeHandle* AllocateNewMethodDictionaryForExpansion(InstantiatedMethodDesc* pIMD, DWORD 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()); + + 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 + 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()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMT->GetNumGenericArgs(); + *pSizeSlot = 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 + { + GC_TRIGGERS; + PRECONDITION(CheckPointer(pGenericParentMT) && CheckPointer(pDependencyMT)); + PRECONDITION(pGenericParentMT->HasInstantiation() && pGenericParentMT != pGenericParentMT->GetCanonicalMethodTable()); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END + + DictionaryLayout* pDictLayout = pDependencyMT->GetClass()->GetDictionaryLayout(); + if (pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0) + { + DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(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 = AllocateNewTypeDictionaryForExpansion(pDependencyMT, sizeFromDictLayout); + + // Publish the new dictionary slots to the type. + TypeHandle** pPerInstInfo = (TypeHandle**)pDependencyMT->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + (pDependencyMT->GetNumDicts() - 1), pInstOrPerInstInfo); + + FlushProcessWriteBuffers(); + } + } + + 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(pDependencyMD->GetDictionaryLayout() != NULL && pDependencyMD->GetDictionaryLayout()->GetMaxSlots() > 0); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END + + DictionaryLayout* pDictLayout = pDependencyMD->GetDictionaryLayout(); + InstantiatedMethodDesc* pIMDDependency = pDependencyMD->AsInstantiatedMethodDesc(); + + DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(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 = AllocateNewMethodDictionaryForExpansion(pIMDDependency, sizeFromDictLayout); + + FastInterlockExchangePointer((TypeHandle**)pIMDDependency->m_pPerInstInfo.GetValuePtr(), pInstOrPerInstInfo); + + FlushProcessWriteBuffers(); + } + + GCX_COOP(); + + _ASSERTE(pDependencyMD->GetExistingWrappedMethodDesc() != NULL); + m_dynamicSlotsHashForMethods.Add(pDependencyMD->GetExistingWrappedMethodDesc(), 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::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END + + GCX_COOP(); + + MethodTable* pCanonMT = pMT->GetCanonicalMethodTable(); + 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 + // 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) 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. + // + + struct NewDictionary + { + MethodTable* pMT; + TypeHandle* pDictSlots; + }; + 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; + + _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey); + _ASSERTE(pMTToUpdate->GetDictionarySlotsSize() == oldInfoSize); + + TypeHandle* pInstOrPerInstInfo = AllocateNewTypeDictionaryForExpansion(pMTToUpdate, 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; + 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) + { + 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(); + TypeHandle** pPerInstInfo = (TypeHandle**)pMTToUpdate->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + dictToUpdate, (TypeHandle*)pUpdatedParentDict); + _ASSERTE(pMTToUpdate->GetPerInstInfo()[dictToUpdate].GetValue() == pUpdatedParentDict); + + return true; // Keep walking + } + pCurrentMT = pCurrentMT->GetParentMethodTable(); + } + + UNREACHABLE(); + }; + + m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, updateDependentDicts); + + // Ensure no other thread uses old dictionary pointers + FlushProcessWriteBuffers(); +} + +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::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->GetExistingWrappedMethodDesc() : pMD; + _ASSERTE(pCanonMD != NULL); + DWORD oldInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pOldLayout); + DWORD newInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pNewLayout); + + struct NewDictionary + { + InstantiatedMethodDesc* pIMD; + TypeHandle* pDictSlots; + }; + 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->GetExistingWrappedMethodDesc() == pMDKey); + + InstantiatedMethodDesc* pInstantiatedMD = pMDToUpdate->AsInstantiatedMethodDesc(); + _ASSERTE(pInstantiatedMD->GetDictionarySlotsSize() == oldInfoSize); + + TypeHandle* pInstOrPerInstInfo = AllocateNewMethodDictionaryForExpansion(pInstantiatedMD, 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++) + { + 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 #endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/vm/ceeload.h b/src/coreclr/src/vm/ceeload.h index f4a0b05120dd9..276254d79742b 100644 --- a/src/coreclr/src/vm/ceeload.h +++ b/src/coreclr/src/vm/ceeload.h @@ -3234,6 +3234,33 @@ 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 { }; + 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); + +#ifdef _DEBUG + void EnsureTypeRecorded(MethodTable* pMT); + void EnsureMethodRecorded(MethodDesc* pMD); +#endif +#endif //!CROSSGEN_COMPILE }; // diff --git a/src/coreclr/src/vm/class.cpp b/src/coreclr/src/vm/class.cpp index 2b123af9d02df..0cfc73fec6f90 100644 --- a/src/coreclr/src/vm/class.cpp +++ b/src/coreclr/src/vm/class.cpp @@ -967,12 +967,87 @@ 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(); 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; + } + + { + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); + + MethodTable* pParentMT = pMT->GetParentMethodTable(); + if (pParentMT != NULL && pParentMT->HasPerInstInfo()) + { + // 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. + + DWORD nDicts = pParentMT->GetNumDicts(); + for (DWORD iDict = 0; iDict < nDicts; iDict++) + { + if (pMT->GetPerInstInfo()[iDict].GetValueMaybeNull() != pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()) + { + 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/coreclr/src/vm/clsload.cpp b/src/coreclr/src/vm/clsload.cpp index 7c1bac2ab8e36..c83da2654994a 100644 --- a/src/coreclr/src/vm/clsload.cpp +++ b/src/coreclr/src/vm/clsload.cpp @@ -3268,14 +3268,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/coreclr/src/vm/clsload.hpp b/src/coreclr/src/vm/clsload.hpp index 23a051f6aeef4..cae8b6c190c1a 100644 --- a/src/coreclr/src/vm/clsload.hpp +++ b/src/coreclr/src/vm/clsload.hpp @@ -971,6 +971,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/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index e573988c7982f..eb67af80a7b9b 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/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; @@ -76,21 +73,24 @@ 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; 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 dictionary slots based on a dictionary layout + } return bytes; } @@ -109,183 +109,334 @@ 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, + WORD* pSlotOut, + DWORD scanFromSlot /* = 0 */, + BOOL useEmptySlotIfFound /* = FALSE */) + { CONTRACTL { STANDARD_VM_CHECK; PRECONDITION(numGenericArgs > 0); + PRECONDITION(scanFromSlot >= 0 && scanFromSlot <= pDictLayout->m_numSlots); PRECONDITION(CheckPointer(pDictLayout)); - PRECONDITION(CheckPointer(pSlotOut)); + PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); 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 slots contain the type parameters + _ASSERTE(FitsIn(numGenericArgs + 1 + scanFromSlot)); + WORD slot = static_cast(numGenericArgs + 1 + scanFromSlot); - // First bucket also contains type parameters - _ASSERTE(FitsIn(numGenericArgs)); - WORD slot = static_cast(numGenericArgs); - for (;;) +#if _DEBUG + if (scanFromSlot > 0) { - for (DWORD iSlot = 0; iSlot < pDictLayout->m_numSlots; iSlot++) + _ASSERT(useEmptySlotIfFound); + + for (DWORD iSlot = 0; iSlot < scanFromSlot; iSlot++) { - RetryMatch: - BYTE * pCandidate = (BYTE *)pDictLayout->m_slots[iSlot].m_signature; - if (pCandidate != NULL) + // 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) { - 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); - } - - // 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) + 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; + _ASSERT(j != cbSig); } } - // If we hit an empty slot then there's no more so use it else { - { - BaseDomain::LockHolder lh(pAllocator->GetDomain()); + _ASSERT(pCandidate != pSig); + } + } + } +#endif - if (pDictLayout->m_slots[iSlot].m_signature != NULL) - goto RetryMatch; + for (DWORD iSlot = scanFromSlot; iSlot < pDictLayout->m_numSlots; iSlot++) + { + BYTE* pCandidate = (BYTE*)pDictLayout->m_slots[iSlot].m_signature; + if (pCandidate != NULL) + { + bool signaturesMatch = false; - PVOID pResultSignature = pSig; + if (pSigBuilder != NULL) + { + // JIT case: compare signatures by comparing the bytes in them. We exclude + // any ReadyToRun signatures from the JIT case. - if (pSigBuilder != NULL) + 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++) { - pSigBuilder->AppendData(isFirstBucket ? slot : 0); - - DWORD cbNewSig; - PVOID pNewSig = pSigBuilder->GetSignature(&cbNewSig); - - pResultSignature = pAllocator->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(cbNewSig)); - memcpy(pResultSignature, pNewSig, cbNewSig); + if (pCandidate[j] != pSig[j]) + break; } - - pDictLayout->m_slots[iSlot].m_signature = pResultSignature; - pDictLayout->m_slots[iSlot].m_signatureSource = signatureSource; + signaturesMatch = (j == cbSig); } + } + else + { + // 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; return TRUE; } - slot++; } + // If we hit an empty slot then there's no more so use it + 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()); - // 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); + PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); + pDictLayout->m_slots[iSlot].m_signature = pResultSignature; + pDictLayout->m_slots[iSlot].m_signatureSource = signatureSource; - pDictLayout = pDictLayout->m_pNext; - isFirstBucket = FALSE; + pResult->signature = pDictLayout->m_slots[iSlot].m_signature; + + _ASSERTE(FitsIn(nFirstOffset + 1)); + pResult->indirections = static_cast(nFirstOffset + 1); + pResult->offsets[nFirstOffset] = slot * sizeof(DictionaryEntry); + *pSlotOut = slot; + return TRUE; + } + + slot++; } -} // DictionaryLayout::FindToken + *pSlotOut = pDictLayout->m_numSlots; + 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, + WORD* pSlotOut) { - WRAPPER_NO_CONTRACT; + CONTRACTL + { + STANDARD_VM_CHECK; + INJECT_FAULT(ThrowOutOfMemory();); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + PRECONDITION(CheckPointer(pResult) && CheckPointer(pSlotOut)); + } + 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); + +#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]; - DWORD cbSig; - BYTE * pSig = (BYTE *)pSigBuilder->GetSignature(&cbSig); + WORD layoutSlotIndex = pCurrentDictLayout->m_numSlots; + WORD slot = static_cast(numGenericArgs) + 1 + layoutSlotIndex; - WORD slotDummy; - return FindTokenWorker(pAllocator, numGenericArgs, pDictLayout, pResult, pSigBuilder, pSig, cbSig, nFirstOffset, signatureSource, &slotDummy); + PVOID pResultSignature = pSigBuilder == NULL ? pSig : CreateSignatureWithSlotData(pSigBuilder, pAllocator, slot); + pNewDictionaryLayout->m_slots[layoutSlotIndex].m_signature = pResultSignature; + 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); + *pSlotOut = 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, + WORD* pSlotOut) { - WRAPPER_NO_CONTRACT; + 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, 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)) + return TRUE; + + +#ifndef CROSSGEN_COMPILE + DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout(); + 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); + return FALSE; + } - return FindTokenWorker(pAllocator, numGenericArgs, pDictLayout, pResult, NULL, signature, -1, nFirstOffset, signatureSource, pSlotOut); + // 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, + WORD* pSlotOut) +{ + 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, 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)) + return TRUE; + +#ifndef CROSSGEN_COMPILE + DictionaryLayout* pOldLayout = pMD->GetDictionaryLayout(); + 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); + 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 +498,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 +521,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); + m_numSlots = static_cast(dwSlots); } @@ -401,21 +539,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 +789,6 @@ Dictionary::PopulateEntry( } CONTRACTL_END; CORINFO_GENERIC_HANDLE result = NULL; - Dictionary * pDictionary = NULL; *ppSlot = NULL; bool isReadyToRunModule = (pModule != NULL && pModule->IsReadyToRun()); @@ -749,7 +879,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 +890,15 @@ Dictionary::PopulateEntry( { IfFailThrow(ptr.GetData(&dictionaryIndex)); } + +#if _DEBUG + // Lock is needed because dictionary pointers can get updated during dictionary size expansion + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); // MethodTable is expected to be normalized + Dictionary* pDictionary = pMT->GetDictionary(); _ASSERTE(pDictionary == pMT->GetPerInstInfo()[dictionaryIndex].GetValueMaybeNull()); - } - else - { - pDictionary = pMD->GetMethodDictionary(); +#endif } { @@ -1245,6 +1376,18 @@ Dictionary::PopulateEntry( if ((slotIndex != 0) && !IsCompilationProcess()) { + // 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(); + *(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); } diff --git a/src/coreclr/src/vm/genericdict.h b/src/coreclr/src/vm/genericdict.h index 44993886bbdf4..b3e7d73ba87f6 100644 --- a/src/coreclr/src/vm/genericdict.h +++ b/src/coreclr/src/vm/genericdict.h @@ -92,6 +92,13 @@ typedef DPTR(DictionaryEntryLayout) PTR_DictionaryEntryLayout; class DictionaryLayout; typedef DPTR(DictionaryLayout) PTR_DictionaryLayout; +// Number of slots to initially allocate in a generic method dictionary layout. +#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 class DictionaryLayout @@ -101,51 +108,63 @@ 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, + WORD* pSlotOut, + DWORD scanFromSlot, + BOOL useEmptySlotIfFound); + + + static DictionaryLayout* ExpandDictionaryLayout(LoaderAllocator* pAllocator, + DictionaryLayout* pCurrentDictLayout, + DWORD numGenericArgs, + SigBuilder* pSigBuilder, + BYTE* pSig, + int nFirstOffset, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut); + + static PVOID CreateSignatureWithSlotData(SigBuilder* pSigBuilder, LoaderAllocator* pAllocator, WORD slot); public: // 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 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 DWORD GetDictionarySizeFromLayout(DWORD numGenericArgs, PTR_DictionaryLayout pDictLayout); + + static BOOL FindToken(MethodTable* pMT, + LoaderAllocator* pAllocator, + int nFirstOffset, + SigBuilder* pSigBuilder, + BYTE* pSig, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut); + + static BOOL FindToken(MethodDesc* pMD, + LoaderAllocator* pAllocator, + int nFirstOffset, + SigBuilder* pSigBuilder, + BYTE* pSig, + DictionaryEntrySignatureSource signatureSource, + CORINFO_RUNTIME_LOOKUP* pResult, + WORD* pSlotOut); DWORD GetMaxSlots(); DWORD GetNumUsedSlots(); @@ -158,7 +177,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/coreclr/src/vm/generics.cpp b/src/coreclr/src/vm/generics.cpp index 8a8bc4cf68b31..3027be1847a90 100644 --- a/src/coreclr/src/vm/generics.cpp +++ b/src/coreclr/src/vm/generics.cpp @@ -278,6 +278,11 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( DWORD cbPerInst = sizeof(GenericsDictInfo) + pOldMT->GetPerInstInfoSize(); // Finally we need space for the instantiation/dictionary for this type + // 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(); // Allocate from the high frequence heap of the correct domain @@ -488,6 +493,13 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( pInstDest[iArg] = inst[iArg]; } + if (cbInstAndDict != 0 && pOldMT->GetClass()->GetDictionaryLayout() != NULL && pOldMT->GetClass()->GetDictionaryLayout()->GetMaxSlots() > 0) + { + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); + ULONG_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/coreclr/src/vm/genmeth.cpp b/src/coreclr/src/vm/genmeth.cpp index 71f1e308e35a3..9dc0c16ad56ea 100644 --- a/src/coreclr/src/vm/genmeth.cpp +++ b/src/coreclr/src/vm/genmeth.cpp @@ -297,7 +297,8 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, MethodDesc* pGenericMDescInRepMT, MethodDesc* pWrappedMD, Instantiation methodInst, - BOOL getWrappedCode) + BOOL getWrappedCode, + BOOL recordForDictionaryExpansion) { CONTRACT(InstantiatedMethodDesc*) { @@ -373,28 +374,41 @@ 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(); } } 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); 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); - pInstOrPerInstInfo = (TypeHandle *) (void*) amt.Track(pAllocator->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(infoSize))); + 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]; + + 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*))); + + ULONG_PTR* pDictSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + methodInst.GetNumArgs(); + *pDictSizeSlot = infoSize; + } } BOOL forComInterop = FALSE; @@ -482,8 +496,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", @@ -506,6 +520,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); @@ -551,6 +574,7 @@ InstantiatedMethodDesc::FindOrCreateExactClassMethod(MethodTable *pExactMT, pCanonicalMD, pCanonicalMD, Instantiation(), + FALSE, FALSE); } @@ -1150,7 +1174,8 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, NULL, Instantiation(repInst, methodInst.GetNumArgs()), - TRUE); + TRUE, + FALSE); } } else if (getWrappedThenStub) @@ -1185,7 +1210,8 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, pWrappedMD, methodInst, - FALSE); + FALSE, + TRUE); } } else @@ -1210,6 +1236,7 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, NULL, methodInst, + FALSE, FALSE); } } @@ -1705,4 +1732,19 @@ BOOL MethodDesc::SatisfiesMethodConstraints(TypeHandle thParent, BOOL fThrowIfNo return TRUE; } +DWORD InstantiatedMethodDesc::GetDictionarySlotsSize() +{ + CONTRACTL + { + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END + + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)IMD_GetMethodDictionary(); + if (pDictionarySlots == NULL) + return 0; + ULONG_PTR* pSizeSlot = pDictionarySlots + m_wNumGenericArgs; + return (DWORD)(*pSizeSlot); +} + #endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index d67f6fb49650a..bda1389074cca 100644 --- a/src/coreclr/src/vm/jitinterface.cpp +++ b/src/coreclr/src/vm/jitinterface.cpp @@ -3444,13 +3444,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->GetLoaderAllocator(), pContextMD->GetNumGenericMethodArgs(), pContextMD->GetDictionaryLayout(), pResult, &sigBuilder, 1, signatureSource)) + if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult, &dummySlot)) { pResult->testForNull = 1; pResult->testForFixup = 0; @@ -3468,7 +3470,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, &dummySlot)) { pResult->testForNull = 1; pResult->testForFixup = 0; diff --git a/src/coreclr/src/vm/method.cpp b/src/coreclr/src/vm/method.cpp index 8eb97e917cc3c..c1f05bf7b4112 100644 --- a/src/coreclr/src/vm/method.cpp +++ b/src/coreclr/src/vm/method.cpp @@ -2645,7 +2645,7 @@ void MethodDesc::Save(DataImage *image) if (GetMethodDictionary()) { - DWORD cBytes = DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericMethodArgs(), GetDictionaryLayout()); + DWORD cBytes = DictionaryLayout::GetDictionarySizeFromLayout(GetNumGenericMethodArgs(), GetDictionaryLayout()); void* pBytes = GetMethodDictionary()->AsPtr(); LOG((LF_ZAP, LL_INFO10000, " MethodDesc::Save dictionary size %d\n", cBytes)); diff --git a/src/coreclr/src/vm/method.hpp b/src/coreclr/src/vm/method.hpp index 18cfc1e0000c4..f2c7a01416287 100644 --- a/src/coreclr/src/vm/method.hpp +++ b/src/coreclr/src/vm/method.hpp @@ -3468,7 +3468,11 @@ class InstantiatedMethodDesc : public MethodDesc Instantiation IMD_GetMethodInstantiation() { LIMITED_METHOD_DAC_CONTRACT; - + + // 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 returns the new or old dictionaries, the + // values of the instantiation arguments will always be the same. return Instantiation(IMD_GetMethodDictionary()->GetInstantiation(), m_wNumGenericArgs); } @@ -3479,6 +3483,10 @@ class InstantiatedMethodDesc : public MethodDesc return ReadPointerMaybeNull(this, &InstantiatedMethodDesc::m_pPerInstInfo); } +#ifndef DACCESS_COMPILE + DWORD GetDictionarySlotsSize(); +#endif + PTR_Dictionary IMD_GetMethodDictionaryNonNull() { LIMITED_METHOD_DAC_CONTRACT; @@ -3574,12 +3582,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 @@ -3667,7 +3688,8 @@ class InstantiatedMethodDesc : public MethodDesc MethodDesc* pGenericMDescInRepMT, MethodDesc* pSharedMDescForStub, Instantiation methodInst, - BOOL getSharedNotStub); + BOOL getSharedNotStub, + BOOL recordForDictionaryExpansion); }; diff --git a/src/coreclr/src/vm/methodtable.cpp b/src/coreclr/src/vm/methodtable.cpp index a509eec929484..cc729de6b9561 100644 --- a/src/coreclr/src/vm/methodtable.cpp +++ b/src/coreclr/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() == NULL) + return 0; + + if (GetClass()->GetDictionaryLayout()->GetMaxSlots() == 0) + return 0; + + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)GetPerInstInfo()[GetGenericsDictInfo()->m_wNumDicts - 1].GetValue(); + ULONG_PTR* pSizeSlot = pDictionarySlots + GetGenericsDictInfo()->m_wNumTyPars; + return (DWORD)(*pSizeSlot); +} #endif // DACCESS_COMPILE //========================================================================================== diff --git a/src/coreclr/src/vm/methodtable.h b/src/coreclr/src/vm/methodtable.h index 0c70148927780..19b6befacbecc 100644 --- a/src/coreclr/src/vm/methodtable.h +++ b/src/coreclr/src/vm/methodtable.h @@ -2936,6 +2936,9 @@ class MethodTable pInfo->m_wNumDicts = numDicts; pInfo->m_wNumTyPars = numTyPars; } + + DWORD GetDictionarySlotsSize(); + #endif // !DACCESS_COMPILE PTR_GenericsDictInfo GetGenericsDictInfo() { diff --git a/src/coreclr/src/vm/methodtable.inl b/src/coreclr/src/vm/methodtable.inl index e3fd0a8d8ffcb..656a0994b9a68 100644 --- a/src/coreclr/src/vm/methodtable.inl +++ b/src/coreclr/src/vm/methodtable.inl @@ -1261,7 +1261,7 @@ inline DWORD MethodTable::GetInstAndDictSize() if (!HasInstantiation()) return 0; else - return DictionaryLayout::GetFirstDictionaryBucketSize(GetNumGenericArgs(), GetClass()->GetDictionaryLayout()); + return DictionaryLayout::GetDictionarySizeFromLayout(GetNumGenericArgs(), GetClass()->GetDictionaryLayout()); } //========================================================================================== diff --git a/src/coreclr/src/vm/methodtablebuilder.cpp b/src/coreclr/src/vm/methodtablebuilder.cpp index cf6000afae9a5..1dfde1645cefb 100644 --- a/src/coreclr/src/vm/methodtablebuilder.cpp +++ b/src/coreclr/src/vm/methodtablebuilder.cpp @@ -10163,7 +10163,7 @@ MethodTableBuilder::SetupMethodTable2( EEClass *pClass = GetHalfBakedClass(); DWORD cbDict = bmtGenerics->HasInstantiation() - ? DictionaryLayout::GetFirstDictionaryBucketSize( + ? DictionaryLayout::GetDictionarySizeFromLayout( bmtGenerics->GetNumGenericArgs(), pClass->GetDictionaryLayout()) : 0; @@ -10461,6 +10461,13 @@ MethodTableBuilder::SetupMethodTable2( { pInstDest[j] = inst[j]; } + + if (pClass->GetDictionaryLayout() != NULL && pClass->GetDictionaryLayout()->GetMaxSlots() > 0) + { + ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[bmtGenerics->numDicts - 1].GetValue(); + ULONG_PTR* pSizeSlot = pDictionarySlots + bmtGenerics->GetNumGenericArgs(); + *pSizeSlot = cbDict; + } } CorElementType normalizedType = ELEMENT_TYPE_CLASS; diff --git a/src/coreclr/src/vm/prestub.cpp b/src/coreclr/src/vm/prestub.cpp index 13b57411cd1f3..509c3ec298822 100644 --- a/src/coreclr/src/vm/prestub.cpp +++ b/src/coreclr/src/vm/prestub.cpp @@ -2971,7 +2971,7 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock 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, &dictionarySlot)) { pResult->testForNull = 1; @@ -2990,7 +2990,7 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock // 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, &dictionarySlot)) { pResult->testForNull = 1; From f2840b26bdac1a60cb52718efbffdc7bd0a6e8ca Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 26 Dec 2019 14:57:26 -0800 Subject: [PATCH 02/11] Feature simplifications 1) Separate dictionary layout expansion from dictionary expansions on types and methods 2) Dictionary expansions on types and methods done on-demand, after size checks by codegen (uses new JIT feature) 3) Update dependency tracking to only track derived types of base types with dictionaries 4) Update the R2R dictionary access stubs to perform size checks (and major cleanup there) 5) Fix loader allocator issue with the dependency tracking (bug in original implementation) 6) Add unit test 7) Update documentation --- docs/design/coreclr/botr/shared-generics.md | 54 ++- src/coreclr/src/inc/corinfo.h | 2 + src/coreclr/src/jit/compiler.h | 3 + src/coreclr/src/jit/importer.cpp | 12 +- src/coreclr/src/vm/amd64/cgenamd64.cpp | 117 ++++--- src/coreclr/src/vm/arm/stubs.cpp | 99 +++--- src/coreclr/src/vm/arm64/stubs.cpp | 73 +++- src/coreclr/src/vm/ceeload.cpp | 317 ++--------------- src/coreclr/src/vm/ceeload.h | 22 +- src/coreclr/src/vm/class.cpp | 4 +- src/coreclr/src/vm/genericdict.cpp | 153 +++++++-- src/coreclr/src/vm/genericdict.h | 52 +-- src/coreclr/src/vm/generics.cpp | 7 +- src/coreclr/src/vm/genmeth.cpp | 25 +- src/coreclr/src/vm/i386/cgenx86.cpp | 56 +-- src/coreclr/src/vm/jitinterface.cpp | 21 +- src/coreclr/src/vm/method.hpp | 3 +- src/coreclr/src/vm/prestub.cpp | 15 +- .../DictionaryExpansion.cs | 325 ++++++++++++++++++ .../DictionaryExpansion.csproj | 10 + 20 files changed, 822 insertions(+), 548 deletions(-) create mode 100644 src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs create mode 100644 src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.csproj diff --git a/docs/design/coreclr/botr/shared-generics.md b/docs/design/coreclr/botr/shared-generics.md index c2fa0a21e1fe2..a36ad64e4c8c1 100644 --- a/docs/design/coreclr/botr/shared-generics.md +++ b/docs/design/coreclr/botr/shared-generics.md @@ -47,7 +47,7 @@ This feature is currently only supported for instantiations over reference types 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. +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 dictionaries of their base types. Here's an example: ```c# @@ -103,23 +103,23 @@ For instance, here is an example of the contents of the generic dictionary for o | 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. +*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 this dictionary is first allocated, only slot[0] is initialized because it contains the instantiation type arguments (and of course the size slot is also initialized with 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). +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 ([genericdict.cpp](https://github.com/dotnet/runtime/blob/master/src/coreclr/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>`. +- It is shared across 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. +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 instantiated dictionary at that slot index. Given an input signature, slot assignment is performed with the following algorithm: @@ -146,7 +146,7 @@ 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 couldn't resize the generic dictionaries of types or methods, because they are already 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. @@ -154,32 +154,48 @@ 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: +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 implementing 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 runtime is multi-threaded, 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. +The current implementation expands the dictionary layout and the actual dictionaries separately to keep things simple: -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). + - Dictionary layouts are expanded when we are out of empty slots. See implementations of `DictionaryLayout::FindToken()` in [genericdict.cpp](https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/genericdict.cpp). + - Instantiated type and method dictionaries are expanded lazily on demand whenever any code is attempting to read the value of a slot beyond the size of the dictionary of that type or method. This is done through a call to the helper functions mentioned previously (`JIT_GenericHandleClass` and `JIT_GenericHandleMethod`). -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. +The dictionary access codegen is equivalent to the following (both in JITted code and ReadyToRun code): +``` c++ +void* pMethodDesc = ; // Input MethodDesc for the instantiated generic method +int requiredOffset = ; // Offset we need to access -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. +void* pDictionary = pMethodDesc->m_pPerInstInfo; -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 how we check for the dictionary size first before indirecting at 'requiredOffset' +if (pDictionary[sizeOffset] <= requiredOffset || pDictionary[requiredOffset] == NULL) + pResult = JIT_GenericHandleMethod(pMethodDesc, ); +else + pResult = pDictionary[requiredOffset]; +``` -### Diagnostics +This size check is **not** done unconditionally every time we need to read a value from the dictionary, otherwise this would cause a noticeable performance regression. When a dictionary layout is first allocated, we keep track of the initial number of slots that were allocated, and **only** perform the size checks if we are attempting to read the value of a slot beyond those initial number of slots. + +Dictionaries on types and methods are expanded by the `Dictionary::GetTypeDictionaryWithSizeCheck()` and `Dictionary::GetMethodDictionaryWithSizeCheck()` helper functions in [genericdict.cpp](https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/genericdict.cpp). + +One thing to note regarding types is that they can inherit dictionary pointers from their base types. This means that if we resize the generic dictionary on any given generic type, we will need to propagate the new dictionary pointer to all of its derived types. To do that, we keep track of such dependencies using a hash-table where the key is the MethodTable pointer of the generic base types with dictionaries, and the values are the MethodTable pointers of all types that derive from these base types. See: `Module::RecordSharedGenericTypeDependency()` and `Module::UpdateDictionaryOnSharedGenericTypeDependencies()` in [ceeload.cpp](https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/ceeload.cpp). -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. +Old dictionaries are not deallocated after resizing, 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. + + +### Diagnostics -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. +To help diagnose runtime failures caused by the expandable nature of generic dictionaries, each dynamically allocated dictionary will have a pointer to its predecessor. Tracking back dictionaries using these pointers can help in diagnosing memory access issues if for any reason a value is read from an old dictionary of smaller size. -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]). +These predecessor pointers are allocated at the beginning 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). +The plan is to also add an SOS command that could help diagnose dictionary contents across the chain of dynamically allocated dictionaries (dotnet/diagnostics#588). diff --git a/src/coreclr/src/inc/corinfo.h b/src/coreclr/src/inc/corinfo.h index 21640c73ca214..566355d4b3fb2 100644 --- a/src/coreclr/src/inc/corinfo.h +++ b/src/coreclr/src/inc/corinfo.h @@ -1275,6 +1275,7 @@ struct CORINFO_LOOKUP_KIND // #define CORINFO_MAXINDIRECTIONS 4 #define CORINFO_USEHELPER ((WORD) 0xffff) +#define CORINFO_SKIPSIZECHECK ((WORD) 0xffff) struct CORINFO_RUNTIME_LOOKUP { @@ -1298,6 +1299,7 @@ struct CORINFO_RUNTIME_LOOKUP bool testForFixup; SIZE_T offsets[CORINFO_MAXINDIRECTIONS]; + WORD sizeOffset; // If set, first offset is indirect. // 0 means that value stored at first offset (offsets[0]) from pointer is next pointer, to which the next offset diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 56d0f4a9047de..53b4be389d3b7 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -66,6 +66,9 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX // a verification block should be inserted #define SEH_VERIFICATION_EXCEPTION 0xe0564552 // VER +// Invalid offset marker value for dynamic dictionary expansions support +#define EXPRUNTIMELOOKUP_INVALID_OFFSET 0xFFFF + /***************************************************************************** * Forward declarations */ diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index c7c9d4d0d437f..f1301665d6a70 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -2082,14 +2082,12 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken if (pRuntimeLookup->offsets[i] != 0) { -// The last indirection could be subject to a size check (dynamic dictionary expansion feature) -#if 0 // Uncomment that block when you add sizeOffset field to pRuntimeLookup. - if (i == pRuntimeLookup->indirections - 1 && pRuntimeLookup->sizeOffset != 0xFFFF) + // The last indirection could be subject to a size check (dynamic dictionary expansion) + if (i == pRuntimeLookup->indirections - 1 && pRuntimeLookup->sizeOffset != EXPRUNTIMELOOKUP_INVALID_OFFSET) { lastIndOfTree = impCloneExpr(slotPtrTree, &slotPtrTree, NO_CLASS_HANDLE, (unsigned)CHECK_SPILL_ALL, nullptr DEBUGARG("impRuntimeLookup indirectOffset")); } -#endif // 0 slotPtrTree = gtNewOperNode(GT_ADD, TYP_I_IMPL, slotPtrTree, gtNewIconNode(pRuntimeLookup->offsets[i], TYP_I_IMPL)); @@ -2160,9 +2158,10 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken GenTree* result = nullptr; -#if 0 // Uncomment that block when you add sizeOffset field to pRuntimeLookup. - if (pRuntimeLookup->sizeOffset != 0xFFFF) // dynamic dictionary expansion feature + if (pRuntimeLookup->sizeOffset != EXPRUNTIMELOOKUP_INVALID_OFFSET) { + // Dynamic dictionary expansion support + assert((lastIndOfTree != nullptr) && (pRuntimeLookup->indirections > 0)); // sizeValue = dictionary[pRuntimeLookup->sizeOffset] @@ -2186,7 +2185,6 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken addExpRuntimeLookupCandidate(helperCall); } else -#endif // 0 { GenTreeColon* colonNullCheck = new (this, GT_COLON) GenTreeColon(TYP_I_IMPL, handleForResult, helperCall); result = gtNewQmarkNode(TYP_I_IMPL, nullCheck, colonNullCheck); diff --git a/src/coreclr/src/vm/amd64/cgenamd64.cpp b/src/coreclr/src/vm/amd64/cgenamd64.cpp index a1ca359f2cd40..9799eecb654b9 100644 --- a/src/coreclr/src/vm/amd64/cgenamd64.cpp +++ b/src/coreclr/src/vm/amd64/cgenamd64.cpp @@ -1145,6 +1145,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, { STANDARD_VM_CONTRACT; + _ASSERTE(!MethodTable::IsPerInstInfoRelative()); + PCODE helperAddress = (pLookup->helper == CORINFO_HELP_RUNTIMEHANDLE_METHOD ? GetEEFuncEntryPoint(JIT_GenericHandleMethodWithSlotAndModule) : GetEEFuncEntryPoint(JIT_GenericHandleClassWithSlotAndModule)); @@ -1154,6 +1156,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, pArgs->signature = pLookup->signature; pArgs->module = (CORINFO_MODULE_HANDLE)pModule; + WORD slotOffset = (WORD)(dictionaryIndexAndSlot & 0xFFFF) * sizeof(Dictionary*); + // It's available only via the run-time helper function if (pLookup->indirections == CORINFO_USEHELPER) { @@ -1172,59 +1176,79 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) indirectionsSize += (pLookup->offsets[i] >= 0x80 ? 7 : 4); - int codeSize = indirectionsSize + (pLookup->testForNull ? 30 : 4); + int codeSize = indirectionsSize + (pLookup->testForNull ? 21 : 1) + (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK ? 13 : 0); BEGIN_DYNAMIC_HELPER_EMIT(codeSize); - if (pLookup->testForNull) - { - // rcx/rdi contains the generic context parameter. Save a copy of it in the rax register -#ifdef UNIX_AMD64_ABI - *(UINT32*)p = 0x00f88948; p += 3; // mov rax,rdi -#else - *(UINT32*)p = 0x00c88948; p += 3; // mov rax,rcx -#endif - } + BYTE* pJLECall = NULL; for (WORD i = 0; i < pLookup->indirections; i++) { -#ifdef UNIX_AMD64_ABI - // mov rdi,qword ptr [rdi+offset] - if (pLookup->offsets[i] >= 0x80) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) { - *(UINT32*)p = 0x00bf8b48; p += 3; - *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; + _ASSERTE(pLookup->testForNull && i > 0); + + // cmp qword ptr[rax + sizeOffset],slotOffset + *(UINT32*)p = 0x00b88148; p += 3; + *(UINT32*)p = (UINT32)pLookup->sizeOffset; p += 4; + *(UINT32*)p = (UINT32)slotOffset; p += 4; + + // jle 'HELPER CALL' + *p++ = 0x7e; + pJLECall = p++; // Offset filled later } - else + + if (i == 0) { - *(UINT32*)p = 0x007f8b48; p += 3; - *p++ = (BYTE)pLookup->offsets[i]; - } + // Move from rcx|rdi if it's the first indirection, otherwise from rax +#ifdef UNIX_AMD64_ABI + // mov rax,qword ptr [rdi+offset] + if (pLookup->offsets[i] >= 0x80) + { + *(UINT32*)p = 0x00878b48; p += 3; + *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; + } + else + { + *(UINT32*)p = 0x00478b48; p += 3; + *p++ = (BYTE)pLookup->offsets[i]; + } #else - // mov rcx,qword ptr [rcx+offset] - if (pLookup->offsets[i] >= 0x80) - { - *(UINT32*)p = 0x00898b48; p += 3; - *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; + // mov rax,qword ptr [rcx+offset] + if (pLookup->offsets[i] >= 0x80) + { + *(UINT32*)p = 0x00818b48; p += 3; + *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; + } + else + { + *(UINT32*)p = 0x00418b48; p += 3; + *p++ = (BYTE)pLookup->offsets[i]; + } +#endif } else { - *(UINT32*)p = 0x00498b48; p += 3; - *p++ = (BYTE)pLookup->offsets[i]; + // mov rax,qword ptr [rax+offset] + if (pLookup->offsets[i] >= 0x80) + { + *(UINT32*)p = 0x00808b48; p += 3; + *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; + } + else + { + *(UINT32*)p = 0x00408b48; p += 3; + *p++ = (BYTE)pLookup->offsets[i]; + } } -#endif } // No null test required if (!pLookup->testForNull) { - // No fixups needed for R2R + _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); -#ifdef UNIX_AMD64_ABI - *(UINT32*)p = 0x00f88948; p += 3; // mov rax,rdi -#else - *(UINT32*)p = 0x00c88948; p += 3; // mov rax,rcx -#endif + // No fixups needed for R2R *p++ = 0xC3; // ret } else @@ -1233,32 +1257,21 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, _ASSERTE(pLookup->indirections != 0); -#ifdef UNIX_AMD64_ABI - *(UINT32*)p = 0x00ff8548; p += 3; // test rdi,rdi -#else - *(UINT32*)p = 0x00c98548; p += 3; // test rcx,rcx -#endif + *(UINT32*)p = 0x00c08548; p += 3; // test rax,rax - // je 'HELPER_CALL' (a jump of 4 bytes) - *(UINT16*)p = 0x0474; p += 2; + // je 'HELPER_CALL' (a jump of 1 byte) + *(UINT16*)p = 0x0174; p += 2; -#ifdef UNIX_AMD64_ABI - *(UINT32*)p = 0x00f88948; p += 3; // mov rax,rdi -#else - *(UINT32*)p = 0x00c88948; p += 3; // mov rax,rcx -#endif *p++ = 0xC3; // ret // 'HELPER_CALL' { - // Put the generic context back into rcx (was previously saved in rax) -#ifdef UNIX_AMD64_ABI - *(UINT32*)p = 0x00c78948; p += 3; // mov rdi,rax -#else - *(UINT32*)p = 0x00c18948; p += 3; // mov rcx,rax -#endif + if (pJLECall != NULL) + *pJLECall = (BYTE)(p - pJLECall - 1); + + // rcx|rdi already contains the generic context parameter - // mov rdx,pArgs + // mov rdx|rsi,pArgs // jmp helperAddress EmitHelperWithArg(p, pAllocator, (TADDR)pArgs, helperAddress); } diff --git a/src/coreclr/src/vm/arm/stubs.cpp b/src/coreclr/src/vm/arm/stubs.cpp index ca6eeaec2ffb6..f2901a19267cc 100644 --- a/src/coreclr/src/vm/arm/stubs.cpp +++ b/src/coreclr/src/vm/arm/stubs.cpp @@ -2769,6 +2769,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, { STANDARD_VM_CONTRACT; + _ASSERTE(!MethodTable::IsPerInstInfoRelative()); + PCODE helperAddress = (pLookup->helper == CORINFO_HELP_RUNTIMEHANDLE_METHOD ? GetEEFuncEntryPoint(JIT_GenericHandleMethodWithSlotAndModule) : GetEEFuncEntryPoint(JIT_GenericHandleClassWithSlotAndModule)); @@ -2778,6 +2780,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, pArgs->signature = pLookup->signature; pArgs->module = (CORINFO_MODULE_HANDLE)pModule; + WORD slotOffset = (WORD)(dictionaryIndexAndSlot & 0xFFFF) * sizeof(Dictionary*); + // It's available only via the run-time helper function, if (pLookup->indirections == CORINFO_USEHELPER) @@ -2791,17 +2795,14 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, else { int indirectionsSize = 0; + if (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + { + indirectionsSize += (pLookup->sizeOffset >= 0xFFF ? 10 : 4); + indirectionsSize += 12; + } for (WORD i = 0; i < pLookup->indirections; i++) { - if ((i == 0 && pLookup->indirectFirstOffset) || (i == 1 && pLookup->indirectSecondOffset)) - { - indirectionsSize += (pLookup->offsets[i] >= 0xFFF ? 10 : 2); - indirectionsSize += 4; - } - else - { - indirectionsSize += (pLookup->offsets[i] >= 0xFFF ? 10 : 4); - } + indirectionsSize += (pLookup->offsets[i] >= 0xFFF ? 10 : 4); } int codeSize = indirectionsSize + (pLookup->testForNull ? 26 : 2); @@ -2815,76 +2816,80 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, p += 2; } + BYTE* pBLECall = NULL; + for (WORD i = 0; i < pLookup->indirections; i++) { - if ((i == 0 && pLookup->indirectFirstOffset) || (i == 1 && pLookup->indirectSecondOffset)) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) { - if (pLookup->offsets[i] >= 0xFF) + _ASSERTE(pLookup->testForNull && i > 0); + + if (pLookup->sizeOffset >= 0xFFF) { // mov r2, offset - MovRegImm(p, 2, pLookup->offsets[i]); - p += 8; - - // add r0, r2 - *(WORD *)p = 0x4410; - p += 2; + MovRegImm(p, 2, pLookup->sizeOffset); p += 8; + // ldr r1, [r0, r2] + *(WORD*)p = 0x5881; p += 2; } else { - // add r0, - *(WORD *)p = (WORD)((WORD)0x3000 | (WORD)((0x00FF) & pLookup->offsets[i])); - p += 2; + // ldr r1, [r0 + offset] + *(WORD*)p = 0xF8D0; p += 2; + *(WORD*)p = (WORD)(0xFFF & pLookup->sizeOffset) | 0x1000; p += 2; } - // r0 is pointer + offset[0] - // ldr r2, [r0] - *(WORD *)p = 0x6802; - p += 2; + // mov r2, slotOffset + MovRegImm(p, 2, slotOffset); p += 8; + + // cmp r1,r2 + *(WORD*)p = 0x4291; p += 2; - // r2 is offset1 - // add r0, r2 - *(WORD *)p = 0x4410; + // ble 'CALL HELPER' + pBLECall = p; // Offset filled later + *(WORD*)p = 0xdd00; p += 2; + } + if (pLookup->offsets[i] >= 0xFFF) + { + // mov r2, offset + MovRegImm(p, 2, pLookup->offsets[i]); + p += 8; + + // ldr r0, [r0, r2] + *(WORD *)p = 0x5880; p += 2; } else { - if (pLookup->offsets[i] >= 0xFFF) - { - // mov r2, offset - MovRegImm(p, 2, pLookup->offsets[i]); - p += 8; - - // ldr r0, [r0, r2] - *(WORD *)p = 0x5880; - p += 2; - } - else - { - // ldr r0, [r0 + offset] - *(WORD *)p = 0xF8D0; - p += 2; - *(WORD *)p = (WORD)(0xFFF & pLookup->offsets[i]); - p += 2; - } + // ldr r0, [r0 + offset] + *(WORD *)p = 0xF8D0; + p += 2; + *(WORD *)p = (WORD)(0xFFF & pLookup->offsets[i]); + p += 2; } } // No null test required if (!pLookup->testForNull) { + _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); + // mov pc, lr *(WORD *)p = 0x46F7; p += 2; } else { - // cbz r0, nullvaluelabel + // cbz r0, 'CALL HELPER' *(WORD *)p = 0xB100; p += 2; // mov pc, lr *(WORD *)p = 0x46F7; p += 2; - // nullvaluelabel: + + // CALL HELPER: + if (pBLECall != NULL) + *(WORD*)pBLECall |= (((BYTE)(p - pBLECall) - 4) >> 1); + // mov r0, r3 *(WORD *)p = 0x4618; p += 2; diff --git a/src/coreclr/src/vm/arm64/stubs.cpp b/src/coreclr/src/vm/arm64/stubs.cpp index 0810e0abb30e5..75fe7216e4130 100644 --- a/src/coreclr/src/vm/arm64/stubs.cpp +++ b/src/coreclr/src/vm/arm64/stubs.cpp @@ -2109,6 +2109,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, { STANDARD_VM_CONTRACT; + _ASSERTE(!MethodTable::IsPerInstInfoRelative()); + PCODE helperAddress = (pLookup->helper == CORINFO_HELP_RUNTIMEHANDLE_METHOD ? GetEEFuncEntryPoint(JIT_GenericHandleMethodWithSlotAndModule) : GetEEFuncEntryPoint(JIT_GenericHandleClassWithSlotAndModule)); @@ -2118,6 +2120,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, pArgs->signature = pLookup->signature; pArgs->module = (CORINFO_MODULE_HANDLE)pModule; + WORD slotOffset = (WORD)(dictionaryIndexAndSlot & 0xFFFF) * sizeof(Dictionary*); + // It's available only via the run-time helper function if (pLookup->indirections == CORINFO_USEHELPER) { @@ -2135,7 +2139,15 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, { int indirectionsCodeSize = 0; int indirectionsDataSize = 0; - for (WORD i = 0; i < pLookup->indirections; i++) { + if (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + { + indirectionsCodeSize += (pLookup->sizeOffset > 32760 ? 8 : 4); + indirectionsDataSize += (pLookup->sizeOffset > 32760 ? 4 : 0); + indirectionsCodeSize += 12; + } + + for (WORD i = 0; i < pLookup->indirections; i++) + { indirectionsCodeSize += (pLookup->offsets[i] > 32760 ? 8 : 4); // if( > 32760) (8 code bytes) else 4 bytes for instruction with offset encoded in instruction indirectionsDataSize += (pLookup->offsets[i] > 32760 ? 4 : 0); // 4 bytes for storing indirection offset values } @@ -2143,8 +2155,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, int codeSize = indirectionsCodeSize; if(pLookup->testForNull) { - codeSize += 4; // mov - codeSize += 12; // cbz-ret-mov + codeSize += 16; // mov-cbz-ret-mov //padding for 8-byte align (required by EmitHelperWithArg) if((codeSize & 0x7) == 0) codeSize += 4; @@ -2159,17 +2170,53 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, BEGIN_DYNAMIC_HELPER_EMIT(codeSize); - if (pLookup->testForNull) + if (pLookup->testForNull || pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) { // mov x9, x0 *(DWORD*)p = 0x91000009; p += 4; } + BYTE* pBLECall = NULL; + // moving offset value wrt PC. Currently points to first indirection offset data. uint dataOffset = codeSize - indirectionsDataSize - (pLookup->testForNull ? 4 : 0); for (WORD i = 0; i < pLookup->indirections; i++) { + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + { + _ASSERTE(pLookup->testForNull && i > 0); + + if (pLookup->sizeOffset > 32760) + { + // ldr w10, [PC, #dataOffset] + *(DWORD*)p = 0x1800000a | ((dataOffset >> 2) << 5); p += 4; + // ldr x11, [x0, x10] + *(DWORD*)p = 0xf86a680b; p += 4; + + // move to next indirection offset data + dataOffset = dataOffset - 8 + 4; // subtract 8 as we have moved PC by 8 and add 4 as next data is at 4 bytes from previous data + } + else + { + // ldr x11, [x0, #(pLookup->sizeOffset)] + *(DWORD*)p = 0xf940000b | (((UINT32)pLookup->sizeOffset >> 3) << 10); p += 4; + dataOffset -= 4; // subtract 4 as we have moved PC by 4 + } + + // mov x10,slotOffset + *(DWORD*)p = 0xd280000a | ((UINT32)slotOffset << 5); p += 4; + dataOffset -= 4; + + // cmp x9,x10 + *(DWORD*)p = 0xeb0a017f; p += 4; + dataOffset -= 4; + + // ble 'CALL HELPER' + pBLECall = p; // Offset filled later + *(DWORD*)p = 0x5400000d; p += 4; + dataOffset -= 4; + } if(pLookup->offsets[i] > 32760) { // ldr w10, [PC, #dataOffset] @@ -2197,19 +2244,25 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // No null test required if (!pLookup->testForNull) { + _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); + // ret lr *(DWORD*)p = 0xd65f03c0; p += 4; } else { - // cbz x0, nullvaluelabel + // cbz x0, 'CALL HELPER' *(DWORD*)p = 0xb4000040; p += 4; // ret lr *(DWORD*)p = 0xd65f03c0; p += 4; - // nullvaluelabel: + + // CALL HELPER: + if(pBLECall != NULL) + *(DWORD*)pBLECall |= (((UINT32)(p - pBLECall) >> 2) << 5); + // mov x0, x9 *(DWORD*)p = 0x91000120; p += 4; @@ -2222,9 +2275,13 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // datalabel: for (WORD i = 0; i < pLookup->indirections; i++) { - if(pLookup->offsets[i] > 32760) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK && pLookup->sizeOffset > 32760) + { + *(UINT32*)p = (UINT32)pLookup->sizeOffset; + p += 4; + } + if (pLookup->offsets[i] > 32760) { - _ASSERTE((pLookup->offsets[i] & 0xffffffff00000000) == 0); *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; } diff --git a/src/coreclr/src/vm/ceeload.cpp b/src/coreclr/src/vm/ceeload.cpp index 55fe4b4e4ad58..304f45457e4f6 100644 --- a/src/coreclr/src/vm/ceeload.cpp +++ b/src/coreclr/src/vm/ceeload.cpp @@ -689,8 +689,7 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) #endif // defined (PROFILING_SUPPORTED) &&!defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) #ifndef CROSSGEN_COMPILE - m_dynamicSlotsHashForTypes.Init(GetLoaderAllocator()); - m_dynamicSlotsHashForMethods.Init(GetLoaderAllocator()); + m_sharedGenericTypeDependencies.Init(GetLoaderAllocator()); #endif LOG((LF_CLASSLOADER, LL_INFO10, "Loaded pModule: \"%ws\".\n", GetDebugName())); @@ -13209,326 +13208,62 @@ void ReflectionModule::ResumeMetadataCapture() CaptureModuleMetaDataToMemory(); } -TypeHandle* AllocateNewMethodDictionaryForExpansion(InstantiatedMethodDesc* pIMD, DWORD 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()); - - 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 + 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()); - - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + pMT->GetNumGenericArgs(); - *pSizeSlot = 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) +void Module::RecordSharedGenericTypeDependency(MethodTable* pMT, MethodTable* pDependencyMT) { CONTRACTL { GC_TRIGGERS; - PRECONDITION(CheckPointer(pGenericParentMT) && CheckPointer(pDependencyMT)); - PRECONDITION(pGenericParentMT->HasInstantiation() && pGenericParentMT != pGenericParentMT->GetCanonicalMethodTable()); + PRECONDITION(pMT != pDependencyMT); + PRECONDITION(CheckPointer(pMT) && CheckPointer(pDependencyMT)); + PRECONDITION(pMT->HasInstantiation() && pMT != pMT->GetCanonicalMethodTable()); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END - DictionaryLayout* pDictLayout = pDependencyMT->GetClass()->GetDictionaryLayout(); - if (pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0) +#if _DEBUG + BOOL isDerived = FALSE; + MethodTable* pCurrentMT = pDependencyMT; + while (pCurrentMT) { - DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pDependencyMT->GetNumGenericArgs(), pDictLayout); - if (pDependencyMT->GetDictionarySlotsSize() != sizeFromDictLayout) + if (pCurrentMT == pMT) { - _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 = AllocateNewTypeDictionaryForExpansion(pDependencyMT, sizeFromDictLayout); - - // Publish the new dictionary slots to the type. - TypeHandle** pPerInstInfo = (TypeHandle**)pDependencyMT->GetPerInstInfo()->GetValuePtr(); - FastInterlockExchangePointer(pPerInstInfo + (pDependencyMT->GetNumDicts() - 1), pInstOrPerInstInfo); - - FlushProcessWriteBuffers(); + isDerived = TRUE; + break; } + pCurrentMT = pCurrentMT->GetParentMethodTable(); } + _ASSERTE(isDerived == TRUE); +#endif GCX_COOP(); - m_dynamicSlotsHashForTypes.Add(pGenericParentMT->GetCanonicalMethodTable(), pDependencyMT, GetLoaderAllocator()); + m_sharedGenericTypeDependencies.Add(pMT, pDependencyMT, pDependencyMT->GetLoaderAllocator()); } -void Module::RecordMethodForDictionaryExpansion_Locked(MethodDesc* pDependencyMD) +void Module::UpdateDictionaryOnSharedGenericTypeDependencies(MethodTable* pMT, Dictionary* pDictionary, ULONG dictionaryIndex) { CONTRACTL { GC_TRIGGERS; - PRECONDITION(CheckPointer(pDependencyMD) && pDependencyMD->HasMethodInstantiation() && pDependencyMD->IsInstantiatingStub()); - PRECONDITION(pDependencyMD->GetDictionaryLayout() != NULL && pDependencyMD->GetDictionaryLayout()->GetMaxSlots() > 0); - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); - } - CONTRACTL_END - - DictionaryLayout* pDictLayout = pDependencyMD->GetDictionaryLayout(); - InstantiatedMethodDesc* pIMDDependency = pDependencyMD->AsInstantiatedMethodDesc(); - - DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(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 = AllocateNewMethodDictionaryForExpansion(pIMDDependency, sizeFromDictLayout); - - FastInterlockExchangePointer((TypeHandle**)pIMDDependency->m_pPerInstInfo.GetValuePtr(), pInstOrPerInstInfo); - - FlushProcessWriteBuffers(); - } - - GCX_COOP(); - - _ASSERTE(pDependencyMD->GetExistingWrappedMethodDesc() != NULL); - m_dynamicSlotsHashForMethods.Add(pDependencyMD->GetExistingWrappedMethodDesc(), 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(CheckPointer(pMT)); + PRECONDITION(pMT->HasInstantiation() && pMT != pMT->GetCanonicalMethodTable()); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END - GCX_COOP(); - - MethodTable* pCanonMT = pMT->GetCanonicalMethodTable(); - 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 - // 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) 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. - // - - struct NewDictionary + auto lambda = [pDictionary, dictionaryIndex](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) { - MethodTable* pMT; - TypeHandle* pDictSlots; - }; - StackSArray dictionaryUpdates; + _ASSERTE(!pMTToUpdate->HasSameTypeDefAs(pMTKey)); -#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; - - _ASSERTE(pMTToUpdate != pMTToUpdate->GetCanonicalMethodTable() && pMTToUpdate->GetCanonicalMethodTable() == pMTKey); - _ASSERTE(pMTToUpdate->GetDictionarySlotsSize() == oldInfoSize); - - TypeHandle* pInstOrPerInstInfo = AllocateNewTypeDictionaryForExpansion(pMTToUpdate, newInfoSize); - - NewDictionary entry; - entry.pMT = pMTToUpdate; - entry.pDictSlots = pInstOrPerInstInfo; - dictionaryUpdates.Append(entry); + TypeHandle** pPerInstInfo = (TypeHandle**)pMTToUpdate->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDictionary); + _ASSERTE(pMTToUpdate->GetPerInstInfo()[dictionaryIndex].GetValue() == pDictionary); - return true; // Keep walking + 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; - 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) - { - 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(); - TypeHandle** pPerInstInfo = (TypeHandle**)pMTToUpdate->GetPerInstInfo()->GetValuePtr(); - FastInterlockExchangePointer(pPerInstInfo + dictToUpdate, (TypeHandle*)pUpdatedParentDict); - _ASSERTE(pMTToUpdate->GetPerInstInfo()[dictToUpdate].GetValue() == pUpdatedParentDict); - - return true; // Keep walking - } - pCurrentMT = pCurrentMT->GetParentMethodTable(); - } - - UNREACHABLE(); - }; - - m_dynamicSlotsHashForTypes.VisitValuesOfKey(pCanonMT, updateDependentDicts); - - // Ensure no other thread uses old dictionary pointers - FlushProcessWriteBuffers(); + m_sharedGenericTypeDependencies.VisitValuesOfKey(pMT, lambda); } -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::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->GetExistingWrappedMethodDesc() : pMD; - _ASSERTE(pCanonMD != NULL); - DWORD oldInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pOldLayout); - DWORD newInfoSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pNewLayout); - - struct NewDictionary - { - InstantiatedMethodDesc* pIMD; - TypeHandle* pDictSlots; - }; - 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->GetExistingWrappedMethodDesc() == pMDKey); - - InstantiatedMethodDesc* pInstantiatedMD = pMDToUpdate->AsInstantiatedMethodDesc(); - _ASSERTE(pInstantiatedMD->GetDictionarySlotsSize() == oldInfoSize); - - TypeHandle* pInstOrPerInstInfo = AllocateNewMethodDictionaryForExpansion(pInstantiatedMD, 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++) - { - 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 #endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/vm/ceeload.h b/src/coreclr/src/vm/ceeload.h index 276254d79742b..8f95122ab5225 100644 --- a/src/coreclr/src/vm/ceeload.h +++ b/src/coreclr/src/vm/ceeload.h @@ -3240,26 +3240,14 @@ class Module #ifndef CROSSGEN_COMPILE private: - class DynamicDictSlotsForTypesTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; - typedef CrossLoaderAllocatorHash DynamicDictSlotsForTypesTrackerHash; + class SharedGenericTypeDependencyTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; + typedef CrossLoaderAllocatorHash SharedGenericTypeDependencyTrackerHash; - class DynamicDictSlotsForMethodsTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; - typedef CrossLoaderAllocatorHash DynamicDictSlotsForMethodsTrackerHash; - - DynamicDictSlotsForTypesTrackerHash m_dynamicSlotsHashForTypes; - DynamicDictSlotsForMethodsTrackerHash m_dynamicSlotsHashForMethods; + SharedGenericTypeDependencyTrackerHash m_sharedGenericTypeDependencies; 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); - -#ifdef _DEBUG - void EnsureTypeRecorded(MethodTable* pMT); - void EnsureMethodRecorded(MethodDesc* pMD); -#endif + void RecordSharedGenericTypeDependency(MethodTable* pMT, MethodTable* pDependencyMT); + void UpdateDictionaryOnSharedGenericTypeDependencies(MethodTable* pMT, Dictionary* pDictionary, ULONG dictionaryIndex); #endif //!CROSSGEN_COMPILE }; diff --git a/src/coreclr/src/vm/class.cpp b/src/coreclr/src/vm/class.cpp index 0cfc73fec6f90..41adeb67146f1 100644 --- a/src/coreclr/src/vm/class.cpp +++ b/src/coreclr/src/vm/class.cpp @@ -1037,8 +1037,8 @@ void ClassLoader::RecordDependenciesForDictionaryExpansion(MethodTable* pMT) MethodTable* pCurrentMT = pMT; while (pCurrentMT) { - if (pCurrentMT->HasInstantiation() && !pCurrentMT->IsCanonicalMethodTable()) - pCurrentMT->GetModule()->RecordTypeForDictionaryExpansion_Locked(pCurrentMT, pMT); + if (pCurrentMT != pMT && pCurrentMT->HasInstantiation() && !pCurrentMT->IsCanonicalMethodTable()) + pCurrentMT->GetLoaderModule()->RecordSharedGenericTypeDependency(pCurrentMT, pMT); pCurrentMT = pCurrentMT->GetParentMethodTable(); } diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index eb67af80a7b9b..d3352a47aa083 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -37,11 +37,9 @@ //--------------------------------------------------------------------------------------- // //static -DictionaryLayout * -DictionaryLayout::Allocate( - WORD numSlots, - LoaderAllocator * pAllocator, - AllocMemTracker * pamTracker) +DictionaryLayout* DictionaryLayout::Allocate(WORD numSlots, + LoaderAllocator * pAllocator, + AllocMemTracker * pamTracker) { CONTRACT(DictionaryLayout*) { @@ -65,9 +63,10 @@ DictionaryLayout::Allocate( // This is the number of slots excluding the type parameters pD->m_numSlots = numSlots; + pD->m_numInitialSlots = numSlots; RETURN pD; -} // DictionaryLayout::Allocate +} #endif //!DACCESS_COMPILE @@ -279,6 +278,8 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots * 2, pAllocator, NULL); #endif + pNewDictionaryLayout->m_numInitialSlots = pCurrentDictLayout->m_numInitialSlots; + for (DWORD iSlot = 0; iSlot < pCurrentDictLayout->m_numSlots; iSlot++) pNewDictionaryLayout->m_slots[iSlot] = pCurrentDictLayout->m_slots[iSlot]; @@ -318,7 +319,7 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, PRECONDITION(CheckPointer(pResult)); PRECONDITION(pMT->HasInstantiation()); } - CONTRACTL_END + CONTRACTL_END; DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; @@ -330,7 +331,6 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, // 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)) return TRUE; - #ifndef CROSSGEN_COMPILE DictionaryLayout* pOldLayout = pMT->GetClass()->GetDictionaryLayout(); @@ -341,12 +341,8 @@ BOOL DictionaryLayout::FindToken(MethodTable* pMT, 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. + // Update the dictionary layout pointer. Note that the expansion of the dictionaries of all instantiated types using this layout + // is done lazily, whenever we attempt to access a slot that is beyond the size of the existing dictionary on that type. pMT->GetClass()->SetDictionaryLayout(pNewLayout); return TRUE; @@ -375,7 +371,7 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, PRECONDITION(CheckPointer(pResult)); PRECONDITION(pMD->HasMethodInstantiation()); } - CONTRACTL_END + CONTRACTL_END; DWORD cbSig = -1; pSig = pSigBuilder != NULL ? (BYTE*)pSigBuilder->GetSignature(&cbSig) : pSig; @@ -397,12 +393,8 @@ BOOL DictionaryLayout::FindToken(MethodDesc* pMD, 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. + // Update the dictionary layout pointer. Note that the expansion of the dictionaries of all instantiated methods using this layout + // is done lazily, whenever we attempt to access a slot that is beyond the size of the existing dictionary on that method. pMD->AsInstantiatedMethodDesc()->IMD_SetDictionaryLayout(pNewLayout); return TRUE; @@ -441,13 +433,18 @@ PVOID DictionaryLayout::CreateSignatureWithSlotData(SigBuilder* pSigBuilder, Loa //--------------------------------------------------------------------------------------- // -DWORD -DictionaryLayout::GetMaxSlots() +DWORD DictionaryLayout::GetMaxSlots() { LIMITED_METHOD_CONTRACT; return m_numSlots; } +DWORD DictionaryLayout::GetNumInitialSlots() +{ + LIMITED_METHOD_CONTRACT; + return m_numInitialSlots; +} + //--------------------------------------------------------------------------------------- // DWORD @@ -771,6 +768,105 @@ Dictionary::Restore( } #endif // FEATURE_PREJIT +Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) +{ + CONTRACT(Dictionary*) + { + THROWS; + GC_TRIGGERS; + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + POSTCONDITION(CheckPointer(RETVAL)); + } + CONTRACT_END; + + Dictionary* pDictionary = pMD->GetMethodDictionary(); + +#if !defined(CROSSGEN_COMPILE) + DictionaryLayout* pDictLayout = pMD->GetDictionaryLayout(); + InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); + _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + + DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); + if (pIMD->GetDictionarySlotsSize() != sizeFromDictLayout) + { + _ASSERT(pIMD->GetDictionarySlotsSize() < sizeFromDictLayout); + + pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout + sizeof(void*))); + ZeroMemory(pDictionary, sizeFromDictLayout + sizeof(void*)); + + // Slot[-1] points at previous dictionary to help with diagnostics when investigating crashes + *(byte**)pDictionary = (byte*)pIMD->m_pPerInstInfo.GetValue() + 1; + pDictionary++; + + // Copy old dictionary entry contents + memcpy(pDictionary, (const void*)pIMD->m_pPerInstInfo.GetValue(), pIMD->GetDictionarySlotsSize()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pDictionary) + pIMD->GetNumGenericMethodArgs(); + *pSizeSlot = sizeFromDictLayout; + + // Flush any write buffers before publishing the new dictionary contents + FlushProcessWriteBuffers(); + + // Publish the new dictionary slots to the type. + FastInterlockExchangePointer((TypeHandle**)pIMD->m_pPerInstInfo.GetValuePtr(), (TypeHandle*)pDictionary); + } +#endif + + RETURN pDictionary; +} + +Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT) +{ + CONTRACT(Dictionary*) + { + THROWS; + GC_TRIGGERS; + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + POSTCONDITION(CheckPointer(RETVAL)); + } + CONTRACT_END; + + Dictionary* pDictionary = pMT->GetDictionary(); + +#if !defined(CROSSGEN_COMPILE) + DictionaryLayout* pDictLayout = pMT->GetClass()->GetDictionaryLayout(); + _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + + DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); + if (pMT->GetDictionarySlotsSize() != sizeFromDictLayout) + { + _ASSERTE(pMT->GetDictionarySlotsSize() < sizeFromDictLayout); + + // Expand type dictionary + pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout + sizeof(void*))); + ZeroMemory(pDictionary, sizeFromDictLayout + sizeof(void*)); + + // Slot[-1] points at previous dictionary to help with diagnostics when investigating crashes + *(byte**)pDictionary = (byte*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue() + 1; + pDictionary++; + + // Copy old dictionary entry contents + memcpy(pDictionary, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), pMT->GetDictionarySlotsSize()); + + ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pDictionary) + pMT->GetNumGenericArgs(); + *pSizeSlot = sizeFromDictLayout; + + // Flush any write buffers before publishing the new dictionary contents + FlushProcessWriteBuffers(); + + // Publish the new dictionary slots to the type. + ULONG dictionaryIndex = pMT->GetNumDicts() - 1; + TypeHandle** pPerInstInfo = (TypeHandle**)pMT->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDictionary); + + // Update dictionary pointer on derived types + pMT->GetLoaderModule()->UpdateDictionaryOnSharedGenericTypeDependencies(pMT, pDictionary, dictionaryIndex); + } +#endif + + RETURN pDictionary; +} + //--------------------------------------------------------------------------------------- // DictionaryEntry @@ -1379,14 +1475,9 @@ 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(); + Dictionary* pDictionary = (pMT != NULL) ? + GetTypeDictionaryWithSizeCheck(pMT) : + GetMethodDictionaryWithSizeCheck(pMD); *(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); diff --git a/src/coreclr/src/vm/genericdict.h b/src/coreclr/src/vm/genericdict.h index b3e7d73ba87f6..32cb9ad4bcb2e 100644 --- a/src/coreclr/src/vm/genericdict.h +++ b/src/coreclr/src/vm/genericdict.h @@ -111,6 +111,9 @@ class DictionaryLayout // Number of non-type-argument slots in this bucket WORD m_numSlots; + // Number of non-type-argument slots of the initial layout before any expansion + WORD m_numInitialSlots; + // m_numSlots of these DictionaryEntryLayout m_slots[1]; @@ -167,6 +170,7 @@ class DictionaryLayout WORD* pSlotOut); DWORD GetMaxSlots(); + DWORD GetNumInitialSlots(); DWORD GetNumUsedSlots(); PTR_DictionaryEntryLayout GetEntryLayout(DWORD i) @@ -212,7 +216,7 @@ class Dictionary #ifdef DACCESS_COMPILE friend class NativeImageDumper; #endif - private: +private: // First N entries are generic instantiations arguments. They are stored as FixupPointers // in NGen images. It means that the lowest bit is used to mark optional indirection (see code:FixupPointer). // The rest of the open array are normal pointers (no optional indirection). @@ -226,7 +230,7 @@ class Dictionary idx * sizeof(m_pEntries[0]); } - public: +public: inline DPTR(FixupPointer) GetInstantiation() { LIMITED_METHOD_CONTRACT; @@ -238,11 +242,11 @@ class Dictionary inline void* AsPtr() { LIMITED_METHOD_CONTRACT; - return (void*) m_pEntries; + return (void*)m_pEntries; } #endif // #ifndef DACCESS_COMPILE - private: +private: #ifndef DACCESS_COMPILE @@ -251,50 +255,50 @@ class Dictionary LIMITED_METHOD_CONTRACT; return *GetTypeHandleSlotAddr(numGenericArgs, i); } - inline MethodDesc *GetMethodDescSlot(DWORD numGenericArgs, DWORD i) + inline MethodDesc* GetMethodDescSlot(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return *GetMethodDescSlotAddr(numGenericArgs,i); + return *GetMethodDescSlotAddr(numGenericArgs, i); } - inline FieldDesc *GetFieldDescSlot(DWORD numGenericArgs, DWORD i) + inline FieldDesc* GetFieldDescSlot(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return *GetFieldDescSlotAddr(numGenericArgs,i); + return *GetFieldDescSlotAddr(numGenericArgs, i); } - inline TypeHandle *GetTypeHandleSlotAddr(DWORD numGenericArgs, DWORD i) + inline TypeHandle* GetTypeHandleSlotAddr(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return ((TypeHandle *) &m_pEntries[numGenericArgs + i]); + return ((TypeHandle*)&m_pEntries[numGenericArgs + i]); } - inline MethodDesc **GetMethodDescSlotAddr(DWORD numGenericArgs, DWORD i) + inline MethodDesc** GetMethodDescSlotAddr(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return ((MethodDesc **) &m_pEntries[numGenericArgs + i]); + return ((MethodDesc**)&m_pEntries[numGenericArgs + i]); } - inline FieldDesc **GetFieldDescSlotAddr(DWORD numGenericArgs, DWORD i) + inline FieldDesc** GetFieldDescSlotAddr(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return ((FieldDesc **) &m_pEntries[numGenericArgs + i]); + return ((FieldDesc**)&m_pEntries[numGenericArgs + i]); } - inline DictionaryEntry *GetSlotAddr(DWORD numGenericArgs, DWORD i) + inline DictionaryEntry* GetSlotAddr(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return ((void **) &m_pEntries[numGenericArgs + i]); + return ((void**)&m_pEntries[numGenericArgs + i]); } inline DictionaryEntry GetSlot(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return *GetSlotAddr(numGenericArgs,i); + return *GetSlotAddr(numGenericArgs, i); } inline BOOL IsSlotEmpty(DWORD numGenericArgs, DWORD i) { LIMITED_METHOD_CONTRACT; - return GetSlot(numGenericArgs,i) == NULL; + return GetSlot(numGenericArgs, i) == NULL; } #endif // #ifndef DACCESS_COMPILE - public: +public: #ifndef DACCESS_COMPILE @@ -310,9 +314,13 @@ class Dictionary MethodTable * pMT, BOOL nonExpansive); +private: + static Dictionary* GetTypeDictionaryWithSizeCheck(MethodTable* pMT); + static Dictionary* GetMethodDictionaryWithSizeCheck(MethodDesc* pMD); + #endif // #ifndef DACCESS_COMPILE - public: +public: #ifdef FEATURE_PREJIT @@ -330,13 +338,13 @@ class Dictionary BOOL canSaveInstantiation, BOOL canSaveSlots, DWORD numGenericArgs, // Must be non-zero - Module *pModule, // module of the generic code + Module *pModule, // module of the generic code DictionaryLayout *pDictLayout); // If NULL, then only type arguments are present BOOL IsWriteable(DataImage *image, BOOL canSaveSlots, DWORD numGenericArgs, // Must be non-zero - Module *pModule, // module of the generic code + Module *pModule, // module of the generic code DictionaryLayout *pDictLayout); // If NULL, then only type arguments are present BOOL ComputeNeedsRestore(DataImage *image, diff --git a/src/coreclr/src/vm/generics.cpp b/src/coreclr/src/vm/generics.cpp index 3027be1847a90..7621b08be53c4 100644 --- a/src/coreclr/src/vm/generics.cpp +++ b/src/coreclr/src/vm/generics.cpp @@ -279,10 +279,9 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( // Finally we need space for the instantiation/dictionary for this type // 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. + // and the dictionary layout can be updated by other threads during a dictionary size expansion. This is + // not a problem anyways because whenever we load a value from the dictionary after a certain index, we will + // always check the size of the dictionary and expand it if needed DWORD cbInstAndDict = pOldMT->GetInstAndDictSize(); // Allocate from the high frequence heap of the correct domain diff --git a/src/coreclr/src/vm/genmeth.cpp b/src/coreclr/src/vm/genmeth.cpp index 9dc0c16ad56ea..9006e51f1e36d 100644 --- a/src/coreclr/src/vm/genmeth.cpp +++ b/src/coreclr/src/vm/genmeth.cpp @@ -297,8 +297,7 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, MethodDesc* pGenericMDescInRepMT, MethodDesc* pWrappedMD, Instantiation methodInst, - BOOL getWrappedCode, - BOOL recordForDictionaryExpansion) + BOOL getWrappedCode) { CONTRACT(InstantiatedMethodDesc*) { @@ -375,9 +374,8 @@ 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. + // when we load values from a generic dictionary beyond a certain index, we will check the size of + // the dictionary and expand it if needed. pDL = pWrappedMD->AsInstantiatedMethodDesc()->GetDictLayoutRaw(); } } @@ -520,15 +518,6 @@ 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); @@ -574,7 +563,6 @@ InstantiatedMethodDesc::FindOrCreateExactClassMethod(MethodTable *pExactMT, pCanonicalMD, pCanonicalMD, Instantiation(), - FALSE, FALSE); } @@ -1174,8 +1162,7 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, NULL, Instantiation(repInst, methodInst.GetNumArgs()), - TRUE, - FALSE); + TRUE); } } else if (getWrappedThenStub) @@ -1210,8 +1197,7 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, pWrappedMD, methodInst, - FALSE, - TRUE); + FALSE); } } else @@ -1236,7 +1222,6 @@ MethodDesc::FindOrCreateAssociatedMethodDesc(MethodDesc* pDefMD, pMDescInCanonMT, NULL, methodInst, - FALSE, FALSE); } } diff --git a/src/coreclr/src/vm/i386/cgenx86.cpp b/src/coreclr/src/vm/i386/cgenx86.cpp index fb890e814e8e3..399ee056d7c69 100644 --- a/src/coreclr/src/vm/i386/cgenx86.cpp +++ b/src/coreclr/src/vm/i386/cgenx86.cpp @@ -1663,6 +1663,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, { STANDARD_VM_CONTRACT; + _ASSERTE(!MethodTable::IsPerInstInfoRelative()); + PCODE helperAddress = (pLookup->helper == CORINFO_HELP_RUNTIMEHANDLE_METHOD ? GetEEFuncEntryPoint(JIT_GenericHandleMethodWithSlotAndModule) : GetEEFuncEntryPoint(JIT_GenericHandleClassWithSlotAndModule)); @@ -1672,6 +1674,8 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, pArgs->signature = pLookup->signature; pArgs->module = (CORINFO_MODULE_HANDLE)pModule; + WORD slotOffset = (WORD)(dictionaryIndexAndSlot & 0xFFFF) * sizeof(Dictionary*); + // It's available only via the run-time helper function if (pLookup->indirections == CORINFO_USEHELPER) { @@ -1690,28 +1694,38 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) indirectionsSize += (pLookup->offsets[i] >= 0x80 ? 6 : 3); - int codeSize = indirectionsSize + (pLookup->testForNull ? 21 : 3); + int codeSize = indirectionsSize + (pLookup->testForNull ? 15 : 1) + (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK ? 12 : 0); BEGIN_DYNAMIC_HELPER_EMIT(codeSize); - if (pLookup->testForNull) - { - // ecx contains the generic context parameter. Save a copy of it in the eax register - // mov eax,ecx - *(UINT16*)p = 0xc889; p += 2; - } + BYTE* pJLECall = NULL; for (WORD i = 0; i < pLookup->indirections; i++) { - // mov ecx,qword ptr [ecx+offset] + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + { + _ASSERTE(pLookup->testForNull && i > 0); + + // cmp dword ptr[eax + sizeOffset],slotOffset + *(UINT16*)p = 0xb881; p += 2; + *(UINT32*)p = (UINT32)pLookup->sizeOffset; p += 4; + *(UINT32*)p = (UINT32)slotOffset; p += 4; + + // jle 'HELPER CALL' + *p++ = 0x7e; + pJLECall = p++; // Offset filled later + } + + // Move from ecx if it's the first indirection, otherwise from eax + // mov eax,dword ptr [ecx|eax + offset] if (pLookup->offsets[i] >= 0x80) { - *(UINT16*)p = 0x898b; p += 2; + *(UINT16*)p = (i == 0 ? 0x818b : 0x808b); p += 2; *(UINT32*)p = (UINT32)pLookup->offsets[i]; p += 4; } else { - *(UINT16*)p = 0x498b; p += 2; + *(UINT16*)p = (i == 0 ? 0x418b : 0x408b); p += 2; *p++ = (BYTE)pLookup->offsets[i]; } } @@ -1719,10 +1733,9 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // No null test required if (!pLookup->testForNull) { - // No fixups needed for R2R + _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); - // mov eax,ecx - *(UINT16*)p = 0xc889; p += 2; + // No fixups needed for R2R *p++ = 0xC3; // ret } else @@ -1731,21 +1744,20 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, _ASSERTE(pLookup->indirections != 0); - // test ecx,ecx - *(UINT16*)p = 0xc985; p += 2; + // test eax,eax + *(UINT16*)p = 0xc085; p += 2; - // je 'HELPER_CALL' (a jump of 3 bytes) - *(UINT16*)p = 0x0374; p += 2; + // je 'HELPER_CALL' (a jump of 1 byte) + *(UINT16*)p = 0x0174; p += 2; - // mov eax,ecx - *(UINT16*)p = 0xc889; p += 2; *p++ = 0xC3; // ret // 'HELPER_CALL' { - // Put the generic context back into rcx (was previously saved in eax) - // mov ecx,eax - *(UINT16*)p = 0xc189; p += 2; + if (pJLECall != NULL) + *pJLECall = (BYTE)(p - pJLECall - 1); + + // ecx already contains the generic context parameter // mov edx,pArgs // jmp helperAddress diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index bda1389074cca..28df651f06e49 100644 --- a/src/coreclr/src/vm/jitinterface.cpp +++ b/src/coreclr/src/vm/jitinterface.cpp @@ -3020,6 +3020,9 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr pResult->indirectFirstOffset = 0; pResult->indirectSecondOffset = 0; + // Dictionary size checks skipped by default, unless we decide otherwise + pResult->sizeOffset = CORINFO_SKIPSIZECHECK; + // Unless we decide otherwise, just do the lookup via a helper function pResult->indirections = CORINFO_USEHELPER; @@ -3444,7 +3447,7 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr DictionaryEntrySignatureSource signatureSource = (IsCompilationProcess() ? FromZapImage : FromJIT); - WORD dummySlot; + WORD slot; // It's a method dictionary lookup if (pResultLookup->lookupKind.runtimeLookupKind == CORINFO_LOOKUP_METHODPARAM) @@ -3452,10 +3455,16 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr _ASSERTE(pContextMD != NULL); _ASSERTE(pContextMD->HasMethodInstantiation()); - if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult, &dummySlot)) + if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult, &slot)) { pResult->testForNull = 1; pResult->testForFixup = 0; + int minDictSize = pContextMD->GetNumGenericMethodArgs() + 1 + pContextMD->GetDictionaryLayout()->GetNumInitialSlots(); + if (slot >= minDictSize) + { + // Dictionaries are guaranteed to have at least the number of slots allocated initially, so skip size check for smaller indexes + pResult->sizeOffset = (WORD)pContextMD->GetNumGenericMethodArgs() * sizeof(DictionaryEntry); + } // Indirect through dictionary table pointer in InstantiatedMethodDesc pResult->offsets[0] = offsetof(InstantiatedMethodDesc, m_pPerInstInfo); @@ -3470,10 +3479,16 @@ 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, &dummySlot)) + if (DictionaryLayout::FindToken(pContextMT, pContextMT->GetLoaderAllocator(), 2, &sigBuilder, NULL, signatureSource, pResult, &slot)) { pResult->testForNull = 1; pResult->testForFixup = 0; + int minDictSize = pContextMT->GetNumGenericArgs() + 1 + pContextMT->GetClass()->GetDictionaryLayout()->GetNumInitialSlots(); + if (slot >= minDictSize) + { + // Dictionaries are guaranteed to have at least the number of slots allocated initially, so skip size check for smaller indexes + pResult->sizeOffset = (WORD)pContextMT->GetNumGenericArgs() * sizeof(DictionaryEntry); + } // Indirect through dictionary table pointer in vtable pResult->offsets[0] = MethodTable::GetOffsetOfPerInstInfo(); diff --git a/src/coreclr/src/vm/method.hpp b/src/coreclr/src/vm/method.hpp index f2c7a01416287..83437268549ff 100644 --- a/src/coreclr/src/vm/method.hpp +++ b/src/coreclr/src/vm/method.hpp @@ -3688,8 +3688,7 @@ class InstantiatedMethodDesc : public MethodDesc MethodDesc* pGenericMDescInRepMT, MethodDesc* pSharedMDescForStub, Instantiation methodInst, - BOOL getSharedNotStub, - BOOL recordForDictionaryExpansion); + BOOL getSharedNotStub); }; diff --git a/src/coreclr/src/vm/prestub.cpp b/src/coreclr/src/vm/prestub.cpp index 509c3ec298822..fa3f3f78e408d 100644 --- a/src/coreclr/src/vm/prestub.cpp +++ b/src/coreclr/src/vm/prestub.cpp @@ -2877,7 +2877,8 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock pResult->indirectFirstOffset = 0; pResult->indirectSecondOffset = 0; - + // Dictionary size checks skipped by default, unless we decide otherwise + pResult->sizeOffset = CORINFO_SKIPSIZECHECK; pResult->indirections = CORINFO_USEHELPER; DWORD numGenericArgs = 0; @@ -2974,6 +2975,12 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock if (DictionaryLayout::FindToken(pContextMD, pModule->GetLoaderAllocator(), 1, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult, &dictionarySlot)) { pResult->testForNull = 1; + int minDictSize = pContextMD->GetNumGenericMethodArgs() + 1 + pContextMD->GetDictionaryLayout()->GetNumInitialSlots(); + if (dictionarySlot >= minDictSize) + { + // Dictionaries are guaranteed to have at least the number of slots allocated initially, so skip size check for smaller indexes + pResult->sizeOffset = (WORD)pContextMD->GetNumGenericMethodArgs() * sizeof(DictionaryEntry); + } // Indirect through dictionary table pointer in InstantiatedMethodDesc pResult->offsets[0] = offsetof(InstantiatedMethodDesc, m_pPerInstInfo); @@ -2993,6 +3000,12 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock if (DictionaryLayout::FindToken(pContextMT, pModule->GetLoaderAllocator(), 2, NULL, (BYTE*)pBlobStart, FromReadyToRunImage, pResult, &dictionarySlot)) { pResult->testForNull = 1; + int minDictSize = pContextMT->GetNumGenericArgs() + 1 + pContextMT->GetClass()->GetDictionaryLayout()->GetNumInitialSlots(); + if (dictionarySlot >= minDictSize) + { + // Dictionaries are guaranteed to have at least the number of slots allocated initially, so skip size check for smaller indexes + pResult->sizeOffset = (WORD)pContextMT->GetNumGenericArgs() * sizeof(DictionaryEntry); + } // Indirect through dictionary table pointer in vtable pResult->offsets[0] = MethodTable::GetOffsetOfPerInstInfo(); diff --git a/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs new file mode 100644 index 0000000000000..f846f5bd26cba --- /dev/null +++ b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs @@ -0,0 +1,325 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.IO; +using System.Reflection; +using System.Diagnostics; +using System.Collections.Generic; +using System.Reflection.Emit; +using System.Runtime.CompilerServices; +using TestLibrary; + +class TestType1 { } +class TestType2 { } +class TestType3 { } +class TestType4 { } +class TestType5 { } +class TestType6 { } +class TestType7 { } +class TestType8 { } +class TestType9 { } +class TestType10 { } +class TestType11 { } +class TestType12 { } +class TestType13 { } +class TestType14 { } +class TestType15 { } +class TestType16 { } +class TestType17 { } +class TestType18 { } +class TestType19 { } +class TestType20 { } +class TestType21 { } +class TestType22 { } +class TestType23 { } +class TestType24 { } +class TestType25 { } + +public class GenBase +{ + public virtual void VFunc() { } +} + +public class GenClass : GenBase +{ + [MethodImpl(MethodImplOptions.NoInlining)] + public Type FuncOnGenClass(int level) + { + switch (level) + { + case 0: return typeof(T); + case 1: return typeof(TestType1); + case 2: return typeof(TestType2); + case 3: return typeof(TestType3); + case 4: return typeof(TestType4); + case 5: return typeof(TestType5); + case 6: return typeof(TestType6); + case 7: return typeof(TestType7); + case 8: return typeof(TestType8); + case 9: return typeof(TestType9); + case 10: return typeof(TestType10); + case 11: return typeof(TestType11); + case 12: return typeof(TestType12); + case 13: return typeof(TestType13); + case 14: return typeof(TestType14); + case 15: return typeof(TestType15); + case 16: return typeof(TestType16); + case 17: return typeof(TestType17); + case 18: return typeof(TestType18); + case 19: return typeof(TestType19); + case 20: return typeof(TestType20); + case 21: return typeof(TestType21); + case 22: return typeof(TestType22); + case 23: return typeof(TestType23); + case 24: return typeof(TestType24); + case 25: default: return typeof(TestType25); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public Type FuncOnGenClass2(int level) + { + switch (level) + { + case 0: return typeof(T); + case 1: return typeof(TestType1); + case 2: return typeof(TestType2); + case 3: return typeof(TestType3); + case 4: return typeof(TestType4); + case 5: return typeof(TestType5); + case 6: return typeof(TestType6); + case 7: return typeof(TestType7); + case 8: return typeof(TestType8); + case 9: return typeof(TestType9); + case 10: return typeof(TestType10); + case 11: return typeof(TestType11); + case 12: return typeof(TestType12); + case 13: return typeof(TestType13); + case 14: return typeof(TestType14); + case 15: return typeof(TestType15); + case 16: return typeof(TestType16); + case 17: return typeof(TestType17); + case 18: return typeof(TestType18); + case 19: return typeof(TestType19); + case 20: return typeof(TestType20); + case 21: return typeof(TestType21); + case 22: return typeof(TestType22); + case 23: return typeof(TestType23); + case 24: return typeof(TestType24); + case 25: default: return typeof(TestType25); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void DoTest_Inner(int max, GenClass o1, GenClass o2, GenClass o3) + { + Console.WriteLine("TEST: FuncOnGenClass<{0}>", typeof(T1).Name); + for (int i = 0; i < max; i++) + Assert.AreEqual(o1.FuncOnGenClass(i).ToString(), i == 0 ? $"{typeof(T1)}" : $"TestType{i}`1[{typeof(T1)}]"); + + Console.WriteLine("TEST: FuncOnGenClass<{0}>", typeof(T2).Name); + for (int i = 0; i < max; i++) + Assert.AreEqual(o2.FuncOnGenClass(i).ToString(), i == 0 ? $"{typeof(T2)}" : $"TestType{i}`1[{typeof(T2)}]"); + + Console.WriteLine("TEST: FuncOnGenClass2<{0}>", typeof(T2).Name); + for (int i = 0; i < max; i++) + Assert.AreEqual(o2.FuncOnGenClass2(i).ToString(), i == 0 ? $"{typeof(T2)}" : $"TestType{i}`1[{typeof(T2)}]"); + + Console.WriteLine("TEST: FuncOnGenClass<{0}>", typeof(T3).Name); + for (int i = 0; i < max; i++) + Assert.AreEqual(o3.FuncOnGenClass(i).ToString(), i == 0 ? $"{typeof(T3)}" : $"TestType{i}`1[{typeof(T3)}]"); + } + + public static void DoTest_GenClass(int max) + { + DoTest_Inner(max, + new GenClass(), + new GenClass(), + new GenClass()); + } + + public static void DoTest_GenDerived(int max) + { + DoTest_Inner(max, + new GenDerived(), + new GenDerived(), + new GenDerived()); + } + + public static void DoTest_GenDerived2(int max) + { + DoTest_Inner(max, + new GenDerived2(), + new GenDerived2(), + new GenDerived2()); + } + + public static void DoTest_GenDerived3(int max) + { + DoTest_Inner(max, + new GenDerived3(), + new GenDerived3(), + new GenDerived3()); + } + + + [MethodImpl(MethodImplOptions.NoInlining)] + public override void VFunc() + { + Assert.AreEqual(typeof(KeyValuePair).ToString(), "System.Collections.Generic.KeyValuePair`2[System.Object,System.String]"); + Assert.AreEqual(typeof(KeyValuePair).ToString(), "System.Collections.Generic.KeyValuePair`2[System.Object,System.String]"); + } +} + +public class GenDerived : GenClass +{ +} + +public class GenDerived2 : GenDerived +{ +} + +public class GenDerived3 : GenDerived2 +{ +} + + +public class Test +{ + [MethodImpl(MethodImplOptions.NoInlining)] + public static Type GFunc(int level) + { + switch(level) + { + case 0: return typeof(T); + case 1: return typeof(TestType1); + case 2: return typeof(TestType2); + case 3: return typeof(TestType3); + case 4: return typeof(TestType4); + case 5: return typeof(TestType5); + case 6: return typeof(TestType6); + case 7: return typeof(TestType7); + case 8: return typeof(TestType8); + case 9: return typeof(TestType9); + case 10: return typeof(TestType10); + case 11: return typeof(TestType11); + case 12: return typeof(TestType12); + case 13: return typeof(TestType13); + case 14: return typeof(TestType14); + case 15: return typeof(TestType15); + case 16: return typeof(TestType16); + case 17: return typeof(TestType17); + case 18: return typeof(TestType18); + case 19: return typeof(TestType19); + case 20: return typeof(TestType20); + case 21: return typeof(TestType21); + case 22: return typeof(TestType22); + case 23: return typeof(TestType23); + case 24: return typeof(TestType24); + case 25: default: return typeof(TestType25); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static Type GFunc2(int level) + { + switch(level) + { + case 0: return typeof(T); + case 1: return typeof(TestType1); + case 2: return typeof(TestType2); + case 3: return typeof(TestType3); + case 4: return typeof(TestType4); + case 5: return typeof(TestType5); + case 6: return typeof(TestType6); + case 7: return typeof(TestType7); + case 8: return typeof(TestType8); + case 9: return typeof(TestType9); + case 10: return typeof(TestType10); + case 11: return typeof(TestType11); + case 12: return typeof(TestType12); + case 13: return typeof(TestType13); + case 14: return typeof(TestType14); + case 15: return typeof(TestType15); + case 16: return typeof(TestType16); + case 17: return typeof(TestType17); + case 18: return typeof(TestType18); + case 19: return typeof(TestType19); + case 20: return typeof(TestType20); + case 21: return typeof(TestType21); + case 22: return typeof(TestType22); + case 23: return typeof(TestType23); + case 24: return typeof(TestType24); + case 25: default: return typeof(TestType25); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void DoTest(int max) + { + Console.WriteLine("TEST: GFunc"); + for(int i = 0; i < max; i++) + Assert.AreEqual(GFunc(i).ToString(), i == 0 ? "System.String" : $"TestType{i}`1[System.String]"); + + Console.WriteLine("TEST: GFunc(i)"); + for (int i = 0; i < max; i++) + Assert.AreEqual(GFunc(i).ToString(), i == 0 ? "System.Object" : $"TestType{i}`1[System.Object]"); + + Console.WriteLine("TEST: GFunc2(i)"); + for (int i = 0; i < max; i++) + Assert.AreEqual(GFunc2(i).ToString(), i == 0 ? "System.Object" : $"TestType{i}`1[System.Object]"); + + Console.WriteLine("TEST: GFunc(i)"); + for (int i = 0; i < max; i++) + Assert.AreEqual(GFunc(i).ToString(), i == 0 ? "Test" : $"TestType{i}`1[Test]"); + } + + public static int Main() + { + for(int i = 5; i <= 25; i += 5) + { + // Test for generic classes + switch(i % 4) + { + case 0: + GenClass.DoTest_GenClass(i); + break; + case 1: + GenClass.DoTest_GenDerived(i); + break; + case 2: + GenClass.DoTest_GenDerived2(i); + break; + case 3: + GenClass.DoTest_GenDerived3(i); + break; + + } + + // Test for generic methods + DoTest(i); + + { + AssemblyBuilder ab = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("CollectibleAsm"+i), AssemblyBuilderAccess.RunAndCollect); + var tb = ab.DefineDynamicModule("CollectibleMod" + i).DefineType("CollectibleGenDerived"+i, TypeAttributes.Public, typeof(GenDerived2)); + var t = tb.CreateType(); + GenBase col_b = (GenBase)Activator.CreateInstance(t); + col_b.VFunc(); + + ab = null; + tb = null; + t = null; + col_b = null; + for(int k = 0; k < 5; k++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + } + } + + return 100; + } +} diff --git a/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.csproj b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.csproj new file mode 100644 index 0000000000000..30b0e720dbfc5 --- /dev/null +++ b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.csproj @@ -0,0 +1,10 @@ + + + Exe + BuildAndRun + + + + + + From 72f65b655fefb35a0e8360ddd830dfe27e2741e0 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Fri, 14 Feb 2020 10:06:11 -0800 Subject: [PATCH 03/11] CR feedback --- src/coreclr/src/inc/corinfo.h | 4 ++-- .../src/tools/Common/JitInterface/CorInfoTypes.cs | 2 ++ .../JitInterface/CorInfoImpl.ReadyToRun.cs | 1 + src/coreclr/src/vm/amd64/cgenamd64.cpp | 6 +++--- src/coreclr/src/vm/arm/stubs.cpp | 6 +++--- src/coreclr/src/vm/arm64/stubs.cpp | 10 +++++----- src/coreclr/src/vm/i386/cgenx86.cpp | 6 +++--- src/coreclr/src/vm/jitinterface.cpp | 2 +- src/coreclr/src/vm/prestub.cpp | 2 +- 9 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/coreclr/src/inc/corinfo.h b/src/coreclr/src/inc/corinfo.h index 566355d4b3fb2..59b52ff3d564c 100644 --- a/src/coreclr/src/inc/corinfo.h +++ b/src/coreclr/src/inc/corinfo.h @@ -1275,7 +1275,7 @@ struct CORINFO_LOOKUP_KIND // #define CORINFO_MAXINDIRECTIONS 4 #define CORINFO_USEHELPER ((WORD) 0xffff) -#define CORINFO_SKIPSIZECHECK ((WORD) 0xffff) +#define CORINFO_NO_SIZE_CHECK ((WORD) 0xffff) struct CORINFO_RUNTIME_LOOKUP { @@ -1298,8 +1298,8 @@ struct CORINFO_RUNTIME_LOOKUP // If set, test the lowest bit and dereference if set (see code:FixupPointer) bool testForFixup; - SIZE_T offsets[CORINFO_MAXINDIRECTIONS]; WORD sizeOffset; + SIZE_T offsets[CORINFO_MAXINDIRECTIONS]; // If set, first offset is indirect. // 0 means that value stored at first offset (offsets[0]) from pointer is next pointer, to which the next offset diff --git a/src/coreclr/src/tools/Common/JitInterface/CorInfoTypes.cs b/src/coreclr/src/tools/Common/JitInterface/CorInfoTypes.cs index 1b3034ca3dd09..e5a47be05cc78 100644 --- a/src/coreclr/src/tools/Common/JitInterface/CorInfoTypes.cs +++ b/src/coreclr/src/tools/Common/JitInterface/CorInfoTypes.cs @@ -15,6 +15,7 @@ public static class CORINFO // This accounts for up to 2 indirections to get at a dictionary followed by a possible spill slot public const uint MAXINDIRECTIONS = 4; public const ushort USEHELPER = 0xffff; + public const ushort CORINFO_NO_SIZE_CHECK = 0xffff; } public struct CORINFO_METHOD_STRUCT_ @@ -248,6 +249,7 @@ public unsafe struct CORINFO_RUNTIME_LOOKUP public byte _testForFixup; public bool testForFixup { get { return _testForFixup != 0; } set { _testForFixup = value ? (byte)1 : (byte)0; } } + public ushort sizeOffset; public IntPtr offset0; public IntPtr offset1; public IntPtr offset2; diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs index 37f38184ba33c..42f853aecf250 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs @@ -1742,6 +1742,7 @@ private void ComputeRuntimeLookupForSharedGenericToken( // Unless we decide otherwise, just do the lookup via a helper function pResult.indirections = CORINFO.USEHELPER; + pResult.sizeOffset = CORINFO.CORINFO_NO_SIZE_CHECK; MethodDesc contextMethod = methodFromContext(pResolvedToken.tokenContext); TypeDesc contextType = typeFromContext(pResolvedToken.tokenContext); diff --git a/src/coreclr/src/vm/amd64/cgenamd64.cpp b/src/coreclr/src/vm/amd64/cgenamd64.cpp index 9799eecb654b9..35d00720264c8 100644 --- a/src/coreclr/src/vm/amd64/cgenamd64.cpp +++ b/src/coreclr/src/vm/amd64/cgenamd64.cpp @@ -1176,7 +1176,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) indirectionsSize += (pLookup->offsets[i] >= 0x80 ? 7 : 4); - int codeSize = indirectionsSize + (pLookup->testForNull ? 21 : 1) + (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK ? 13 : 0); + int codeSize = indirectionsSize + (pLookup->testForNull ? 21 : 1) + (pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK ? 13 : 0); BEGIN_DYNAMIC_HELPER_EMIT(codeSize); @@ -1184,7 +1184,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) { - if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { _ASSERTE(pLookup->testForNull && i > 0); @@ -1246,7 +1246,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // No null test required if (!pLookup->testForNull) { - _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); + _ASSERTE(pLookup->sizeOffset == CORINFO_NO_SIZE_CHECK); // No fixups needed for R2R *p++ = 0xC3; // ret diff --git a/src/coreclr/src/vm/arm/stubs.cpp b/src/coreclr/src/vm/arm/stubs.cpp index f2901a19267cc..25feee8fc6ef6 100644 --- a/src/coreclr/src/vm/arm/stubs.cpp +++ b/src/coreclr/src/vm/arm/stubs.cpp @@ -2795,7 +2795,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, else { int indirectionsSize = 0; - if (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { indirectionsSize += (pLookup->sizeOffset >= 0xFFF ? 10 : 4); indirectionsSize += 12; @@ -2820,7 +2820,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) { - if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { _ASSERTE(pLookup->testForNull && i > 0); @@ -2871,7 +2871,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // No null test required if (!pLookup->testForNull) { - _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); + _ASSERTE(pLookup->sizeOffset == CORINFO_NO_SIZE_CHECK); // mov pc, lr *(WORD *)p = 0x46F7; diff --git a/src/coreclr/src/vm/arm64/stubs.cpp b/src/coreclr/src/vm/arm64/stubs.cpp index 75fe7216e4130..9987da8e79ff7 100644 --- a/src/coreclr/src/vm/arm64/stubs.cpp +++ b/src/coreclr/src/vm/arm64/stubs.cpp @@ -2139,7 +2139,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, { int indirectionsCodeSize = 0; int indirectionsDataSize = 0; - if (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { indirectionsCodeSize += (pLookup->sizeOffset > 32760 ? 8 : 4); indirectionsDataSize += (pLookup->sizeOffset > 32760 ? 4 : 0); @@ -2170,7 +2170,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, BEGIN_DYNAMIC_HELPER_EMIT(codeSize); - if (pLookup->testForNull || pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (pLookup->testForNull || pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { // mov x9, x0 *(DWORD*)p = 0x91000009; @@ -2183,7 +2183,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, uint dataOffset = codeSize - indirectionsDataSize - (pLookup->testForNull ? 4 : 0); for (WORD i = 0; i < pLookup->indirections; i++) { - if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { _ASSERTE(pLookup->testForNull && i > 0); @@ -2244,7 +2244,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // No null test required if (!pLookup->testForNull) { - _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); + _ASSERTE(pLookup->sizeOffset == CORINFO_NO_SIZE_CHECK); // ret lr *(DWORD*)p = 0xd65f03c0; @@ -2275,7 +2275,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // datalabel: for (WORD i = 0; i < pLookup->indirections; i++) { - if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK && pLookup->sizeOffset > 32760) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK && pLookup->sizeOffset > 32760) { *(UINT32*)p = (UINT32)pLookup->sizeOffset; p += 4; diff --git a/src/coreclr/src/vm/i386/cgenx86.cpp b/src/coreclr/src/vm/i386/cgenx86.cpp index 399ee056d7c69..0de1ab4dfc734 100644 --- a/src/coreclr/src/vm/i386/cgenx86.cpp +++ b/src/coreclr/src/vm/i386/cgenx86.cpp @@ -1694,7 +1694,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) indirectionsSize += (pLookup->offsets[i] >= 0x80 ? 6 : 3); - int codeSize = indirectionsSize + (pLookup->testForNull ? 15 : 1) + (pLookup->sizeOffset != CORINFO_SKIPSIZECHECK ? 12 : 0); + int codeSize = indirectionsSize + (pLookup->testForNull ? 15 : 1) + (pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK ? 12 : 0); BEGIN_DYNAMIC_HELPER_EMIT(codeSize); @@ -1702,7 +1702,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, for (WORD i = 0; i < pLookup->indirections; i++) { - if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_SKIPSIZECHECK) + if (i == pLookup->indirections - 1 && pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { _ASSERTE(pLookup->testForNull && i > 0); @@ -1733,7 +1733,7 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator, // No null test required if (!pLookup->testForNull) { - _ASSERTE(pLookup->sizeOffset == CORINFO_SKIPSIZECHECK); + _ASSERTE(pLookup->sizeOffset == CORINFO_NO_SIZE_CHECK); // No fixups needed for R2R *p++ = 0xC3; // ret diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index 28df651f06e49..dded642a90381 100644 --- a/src/coreclr/src/vm/jitinterface.cpp +++ b/src/coreclr/src/vm/jitinterface.cpp @@ -3021,7 +3021,7 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr pResult->indirectSecondOffset = 0; // Dictionary size checks skipped by default, unless we decide otherwise - pResult->sizeOffset = CORINFO_SKIPSIZECHECK; + pResult->sizeOffset = CORINFO_NO_SIZE_CHECK; // Unless we decide otherwise, just do the lookup via a helper function pResult->indirections = CORINFO_USEHELPER; diff --git a/src/coreclr/src/vm/prestub.cpp b/src/coreclr/src/vm/prestub.cpp index fa3f3f78e408d..5a4c86c7390e7 100644 --- a/src/coreclr/src/vm/prestub.cpp +++ b/src/coreclr/src/vm/prestub.cpp @@ -2878,7 +2878,7 @@ void ProcessDynamicDictionaryLookup(TransitionBlock * pTransitionBlock pResult->indirectFirstOffset = 0; pResult->indirectSecondOffset = 0; // Dictionary size checks skipped by default, unless we decide otherwise - pResult->sizeOffset = CORINFO_SKIPSIZECHECK; + pResult->sizeOffset = CORINFO_NO_SIZE_CHECK; pResult->indirections = CORINFO_USEHELPER; DWORD numGenericArgs = 0; From eb8d5172cf631cba72057abcaf0858ff15fedb86 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 19 Feb 2020 12:25:02 -0800 Subject: [PATCH 04/11] CR feedback --- src/coreclr/src/jit/compiler.h | 3 - src/coreclr/src/jit/importer.cpp | 4 +- src/coreclr/src/vm/genericdict.cpp | 73 +++++++++++++------ src/coreclr/src/vm/genericdict.h | 7 +- src/coreclr/src/vm/generics.cpp | 3 +- src/coreclr/src/vm/genmeth.cpp | 15 ---- src/coreclr/src/vm/method.hpp | 4 - src/coreclr/src/vm/methodtable.cpp | 19 ----- src/coreclr/src/vm/methodtable.h | 2 - .../DictionaryExpansion.cs | 10 +++ 10 files changed, 68 insertions(+), 72 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 53b4be389d3b7..56d0f4a9047de 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -66,9 +66,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX // a verification block should be inserted #define SEH_VERIFICATION_EXCEPTION 0xe0564552 // VER -// Invalid offset marker value for dynamic dictionary expansions support -#define EXPRUNTIMELOOKUP_INVALID_OFFSET 0xFFFF - /***************************************************************************** * Forward declarations */ diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index f1301665d6a70..6d1079e2275c8 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -2083,7 +2083,7 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken if (pRuntimeLookup->offsets[i] != 0) { // The last indirection could be subject to a size check (dynamic dictionary expansion) - if (i == pRuntimeLookup->indirections - 1 && pRuntimeLookup->sizeOffset != EXPRUNTIMELOOKUP_INVALID_OFFSET) + if (i == pRuntimeLookup->indirections - 1 && pRuntimeLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { lastIndOfTree = impCloneExpr(slotPtrTree, &slotPtrTree, NO_CLASS_HANDLE, (unsigned)CHECK_SPILL_ALL, nullptr DEBUGARG("impRuntimeLookup indirectOffset")); @@ -2158,7 +2158,7 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken GenTree* result = nullptr; - if (pRuntimeLookup->sizeOffset != EXPRUNTIMELOOKUP_INVALID_OFFSET) + if (pRuntimeLookup->sizeOffset != CORINFO_NO_SIZE_CHECK) { // Dynamic dictionary expansion support diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index d3352a47aa083..eebd822c40052 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -768,6 +768,37 @@ Dictionary::Restore( } #endif // FEATURE_PREJIT +DWORD Dictionary::GetDictionarySlotsSizeForType(MethodTable* pMT) +{ + CONTRACTL + { + PRECONDITION(pMT->HasPerInstInfo()); + PRECONDITION(pMT->GetGenericsDictInfo()->m_wNumTyPars > 0); + PRECONDITION(pMT->GetClass()->GetDictionaryLayout() != NULL); + PRECONDITION(pMT->GetClass()->GetDictionaryLayout()->GetMaxSlots() > 0); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END; + + TADDR* pDictionarySlots = (TADDR*)pMT->GetPerInstInfo()[pMT->GetGenericsDictInfo()->m_wNumDicts - 1].GetValue(); + TADDR* pSizeSlot = pDictionarySlots + pMT->GetGenericsDictInfo()->m_wNumTyPars; + return *(DWORD*)pSizeSlot; +} + +DWORD Dictionary::GetDictionarySlotsSizeForMethod(InstantiatedMethodDesc* pIMD) +{ + CONTRACTL + { + PRECONDITION(pIMD->IMD_GetMethodDictionary() != NULL); + PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); + } + CONTRACTL_END; + + TADDR* pDictionarySlots = (TADDR*)pIMD->IMD_GetMethodDictionary(); + TADDR* pSizeSlot = pDictionarySlots + pIMD->GetNumGenericMethodArgs(); + return *(DWORD*)pSizeSlot; +} + Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) { CONTRACT(Dictionary*) @@ -786,23 +817,20 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); - DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); - if (pIMD->GetDictionarySlotsSize() != sizeFromDictLayout) + DWORD currentDictLayoutSize = GetDictionarySlotsSizeForMethod(pIMD); + DWORD expectedDictLayoutSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); + if (currentDictLayoutSize != expectedDictLayoutSize) { - _ASSERT(pIMD->GetDictionarySlotsSize() < sizeFromDictLayout); + _ASSERT(currentDictLayoutSize < expectedDictLayoutSize); - pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout + sizeof(void*))); - ZeroMemory(pDictionary, sizeFromDictLayout + sizeof(void*)); - - // Slot[-1] points at previous dictionary to help with diagnostics when investigating crashes - *(byte**)pDictionary = (byte*)pIMD->m_pPerInstInfo.GetValue() + 1; - pDictionary++; + pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictLayoutSize)); + ZeroMemory(pDictionary, expectedDictLayoutSize); // Copy old dictionary entry contents - memcpy(pDictionary, (const void*)pIMD->m_pPerInstInfo.GetValue(), pIMD->GetDictionarySlotsSize()); + memcpy(pDictionary, (const void*)pIMD->m_pPerInstInfo.GetValue(), currentDictLayoutSize); - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pDictionary) + pIMD->GetNumGenericMethodArgs(); - *pSizeSlot = sizeFromDictLayout; + DWORD* pSizeSlot = (DWORD*)(pDictionary + pIMD->GetNumGenericMethodArgs()); + *pSizeSlot = expectedDictLayoutSize; // Flush any write buffers before publishing the new dictionary contents FlushProcessWriteBuffers(); @@ -832,24 +860,21 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT) DictionaryLayout* pDictLayout = pMT->GetClass()->GetDictionaryLayout(); _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); - DWORD sizeFromDictLayout = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); - if (pMT->GetDictionarySlotsSize() != sizeFromDictLayout) + DWORD currentDictLayoutSize = GetDictionarySlotsSizeForType(pMT); + DWORD expectedDictLayoutSize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); + if (currentDictLayoutSize != expectedDictLayoutSize) { - _ASSERTE(pMT->GetDictionarySlotsSize() < sizeFromDictLayout); + _ASSERT(currentDictLayoutSize < expectedDictLayoutSize); // Expand type dictionary - pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(sizeFromDictLayout + sizeof(void*))); - ZeroMemory(pDictionary, sizeFromDictLayout + sizeof(void*)); - - // Slot[-1] points at previous dictionary to help with diagnostics when investigating crashes - *(byte**)pDictionary = (byte*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue() + 1; - pDictionary++; + pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictLayoutSize)); + ZeroMemory(pDictionary, expectedDictLayoutSize); // Copy old dictionary entry contents - memcpy(pDictionary, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), pMT->GetDictionarySlotsSize()); + memcpy(pDictionary, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), currentDictLayoutSize); - ULONG_PTR* pSizeSlot = ((ULONG_PTR*)pDictionary) + pMT->GetNumGenericArgs(); - *pSizeSlot = sizeFromDictLayout; + DWORD* pSizeSlot = (DWORD*)(pDictionary + pMT->GetNumGenericArgs()); + *pSizeSlot = expectedDictLayoutSize; // Flush any write buffers before publishing the new dictionary contents FlushProcessWriteBuffers(); diff --git a/src/coreclr/src/vm/genericdict.h b/src/coreclr/src/vm/genericdict.h index 32cb9ad4bcb2e..71331cdb30d31 100644 --- a/src/coreclr/src/vm/genericdict.h +++ b/src/coreclr/src/vm/genericdict.h @@ -108,7 +108,7 @@ class DictionaryLayout friend class NativeImageDumper; #endif private: - // Number of non-type-argument slots in this bucket + // Current number of non-type-argument slots WORD m_numSlots; // Number of non-type-argument slots of the initial layout before any expansion @@ -144,7 +144,7 @@ class DictionaryLayout static PVOID CreateSignatureWithSlotData(SigBuilder* pSigBuilder, LoaderAllocator* pAllocator, WORD slot); public: - // Create an initial dictionary layout with a single bucket containing numSlots slots + // Create an initial dictionary layout containing numSlots slots static DictionaryLayout* Allocate(WORD numSlots, LoaderAllocator *pAllocator, AllocMemTracker *pamTracker); // Bytes used for this dictionary, which might be stored inline in @@ -315,6 +315,9 @@ class Dictionary BOOL nonExpansive); private: + static DWORD GetDictionarySlotsSizeForType(MethodTable* pMT); + static DWORD GetDictionarySlotsSizeForMethod(InstantiatedMethodDesc* pIMD); + static Dictionary* GetTypeDictionaryWithSizeCheck(MethodTable* pMT); static Dictionary* GetMethodDictionaryWithSizeCheck(MethodDesc* pMD); diff --git a/src/coreclr/src/vm/generics.cpp b/src/coreclr/src/vm/generics.cpp index 7621b08be53c4..50819398a3b2e 100644 --- a/src/coreclr/src/vm/generics.cpp +++ b/src/coreclr/src/vm/generics.cpp @@ -492,7 +492,8 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( pInstDest[iArg] = inst[iArg]; } - if (cbInstAndDict != 0 && pOldMT->GetClass()->GetDictionaryLayout() != NULL && pOldMT->GetClass()->GetDictionaryLayout()->GetMaxSlots() > 0) + PTR_DictionaryLayout pLayout = pOldMT->GetClass()->GetDictionaryLayout(); + if (pLayout != NULL && pLayout->GetMaxSlots() > 0) { ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); ULONG_PTR* pSizeSlot = pDictionarySlots + ntypars; diff --git a/src/coreclr/src/vm/genmeth.cpp b/src/coreclr/src/vm/genmeth.cpp index 9006e51f1e36d..0ea124508c5de 100644 --- a/src/coreclr/src/vm/genmeth.cpp +++ b/src/coreclr/src/vm/genmeth.cpp @@ -1717,19 +1717,4 @@ BOOL MethodDesc::SatisfiesMethodConstraints(TypeHandle thParent, BOOL fThrowIfNo return TRUE; } -DWORD InstantiatedMethodDesc::GetDictionarySlotsSize() -{ - CONTRACTL - { - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); - } - CONTRACTL_END - - ULONG_PTR* pDictionarySlots = (ULONG_PTR*)IMD_GetMethodDictionary(); - if (pDictionarySlots == NULL) - return 0; - ULONG_PTR* pSizeSlot = pDictionarySlots + m_wNumGenericArgs; - return (DWORD)(*pSizeSlot); -} - #endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/vm/method.hpp b/src/coreclr/src/vm/method.hpp index 83437268549ff..22cdabe8bb1f0 100644 --- a/src/coreclr/src/vm/method.hpp +++ b/src/coreclr/src/vm/method.hpp @@ -3483,10 +3483,6 @@ class InstantiatedMethodDesc : public MethodDesc return ReadPointerMaybeNull(this, &InstantiatedMethodDesc::m_pPerInstInfo); } -#ifndef DACCESS_COMPILE - DWORD GetDictionarySlotsSize(); -#endif - PTR_Dictionary IMD_GetMethodDictionaryNonNull() { LIMITED_METHOD_DAC_CONTRACT; diff --git a/src/coreclr/src/vm/methodtable.cpp b/src/coreclr/src/vm/methodtable.cpp index cc729de6b9561..a509eec929484 100644 --- a/src/coreclr/src/vm/methodtable.cpp +++ b/src/coreclr/src/vm/methodtable.cpp @@ -475,25 +475,6 @@ 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() == NULL) - return 0; - - if (GetClass()->GetDictionaryLayout()->GetMaxSlots() == 0) - return 0; - - ULONG_PTR* pDictionarySlots = (ULONG_PTR*)GetPerInstInfo()[GetGenericsDictInfo()->m_wNumDicts - 1].GetValue(); - ULONG_PTR* pSizeSlot = pDictionarySlots + GetGenericsDictInfo()->m_wNumTyPars; - return (DWORD)(*pSizeSlot); -} #endif // DACCESS_COMPILE //========================================================================================== diff --git a/src/coreclr/src/vm/methodtable.h b/src/coreclr/src/vm/methodtable.h index 19b6befacbecc..52457393f4ae2 100644 --- a/src/coreclr/src/vm/methodtable.h +++ b/src/coreclr/src/vm/methodtable.h @@ -2937,8 +2937,6 @@ class MethodTable pInfo->m_wNumTyPars = numTyPars; } - DWORD GetDictionarySlotsSize(); - #endif // !DACCESS_COMPILE PTR_GenericsDictInfo GetGenericsDictInfo() { diff --git a/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs index f846f5bd26cba..ef860f02f395b 100644 --- a/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs +++ b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs @@ -185,6 +185,9 @@ public class GenDerived3 : GenDerived2 { } +public class GenDerived4 : GenDerived3 +{ +} public class Test { @@ -278,6 +281,8 @@ public static void DoTest(int max) public static int Main() { + GenBase deriv4 = new GenDerived4(); + for(int i = 5; i <= 25; i += 5) { // Test for generic classes @@ -319,6 +324,11 @@ public static int Main() } } } + + // After all expansions to existing dictionaries, use GenDerived4. GenDerived4 was allocated before any of its + // base type dictionaries were expanded. + Debugger.Break(); + deriv4.VFunc(); return 100; } From 570beab016fda2bfcf9785d9d15d6159bf0c0217 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 19 Feb 2020 13:31:44 -0800 Subject: [PATCH 05/11] Make dictionary pointer updates on derived types to be lazy Update JIT interface guid --- src/coreclr/src/inc/corinfo.h | 10 +-- .../crossgen2/jitinterface/jitwrapper.cpp | 10 +-- src/coreclr/src/vm/ceeload.cpp | 60 --------------- src/coreclr/src/vm/ceeload.h | 14 +--- src/coreclr/src/vm/class.cpp | 75 ------------------- src/coreclr/src/vm/clsload.hpp | 2 - src/coreclr/src/vm/genericdict.cpp | 3 - src/coreclr/src/vm/jithelpers.cpp | 19 ++++- src/coreclr/src/vm/method.hpp | 2 +- src/coreclr/src/vm/methodtable.h | 1 - .../DictionaryExpansion.cs | 4 +- 11 files changed, 30 insertions(+), 170 deletions(-) diff --git a/src/coreclr/src/inc/corinfo.h b/src/coreclr/src/inc/corinfo.h index 59b52ff3d564c..3d209089237df 100644 --- a/src/coreclr/src/inc/corinfo.h +++ b/src/coreclr/src/inc/corinfo.h @@ -217,11 +217,11 @@ TODO: Talk about initializing strutures before use #endif #endif -SELECTANY const GUID JITEEVersionIdentifier = { /* 9C412381-94A6-4F35-B2B6-60AFB2495B72 */ - 0x9c412381, - 0x94a6, - 0x4f35, - { 0xb2, 0xb6, 0x60, 0xaf, 0xb2, 0x49, 0x5b, 0x72 } +SELECTANY const GUID JITEEVersionIdentifier = { /* b2e40020-6125-41e4-a0fc-821127ec192a */ + 0xb2e40020, + 0x6125, + 0x41e4, + {0xa0, 0xfc, 0x82, 0x11, 0x27, 0xec, 0x19, 0x2a} }; ////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/coreclr/src/tools/crossgen2/jitinterface/jitwrapper.cpp b/src/coreclr/src/tools/crossgen2/jitinterface/jitwrapper.cpp index 26731de7f6aa2..8a4b1f91c10e7 100644 --- a/src/coreclr/src/tools/crossgen2/jitinterface/jitwrapper.cpp +++ b/src/coreclr/src/tools/crossgen2/jitinterface/jitwrapper.cpp @@ -27,11 +27,11 @@ class CORJIT_FLAGS uint64_t corJitFlags; }; -static const GUID JITEEVersionIdentifier = { /* 9C412381-94A6-4F35-B2B6-60AFB2495B72 */ - 0x9c412381, - 0x94a6, - 0x4f35, - { 0xb2, 0xb6, 0x60, 0xaf, 0xb2, 0x49, 0x5b, 0x72 } +static const GUID JITEEVersionIdentifier = { /* b2e40020-6125-41e4-a0fc-821127ec192a */ + 0xb2e40020, + 0x6125, + 0x41e4, + {0xa0, 0xfc, 0x82, 0x11, 0x27, 0xec, 0x19, 0x2a} }; class Jit diff --git a/src/coreclr/src/vm/ceeload.cpp b/src/coreclr/src/vm/ceeload.cpp index 304f45457e4f6..149d44a74a43e 100644 --- a/src/coreclr/src/vm/ceeload.cpp +++ b/src/coreclr/src/vm/ceeload.cpp @@ -688,10 +688,6 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) } #endif // defined (PROFILING_SUPPORTED) &&!defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) -#ifndef CROSSGEN_COMPILE - m_sharedGenericTypeDependencies.Init(GetLoaderAllocator()); -#endif - LOG((LF_CLASSLOADER, LL_INFO10, "Loaded pModule: \"%ws\".\n", GetDebugName())); } @@ -13208,62 +13204,6 @@ void ReflectionModule::ResumeMetadataCapture() CaptureModuleMetaDataToMemory(); } -void Module::RecordSharedGenericTypeDependency(MethodTable* pMT, MethodTable* pDependencyMT) -{ - CONTRACTL - { - GC_TRIGGERS; - PRECONDITION(pMT != pDependencyMT); - PRECONDITION(CheckPointer(pMT) && CheckPointer(pDependencyMT)); - PRECONDITION(pMT->HasInstantiation() && pMT != pMT->GetCanonicalMethodTable()); - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); - } - CONTRACTL_END - -#if _DEBUG - BOOL isDerived = FALSE; - MethodTable* pCurrentMT = pDependencyMT; - while (pCurrentMT) - { - if (pCurrentMT == pMT) - { - isDerived = TRUE; - break; - } - pCurrentMT = pCurrentMT->GetParentMethodTable(); - } - _ASSERTE(isDerived == TRUE); -#endif - - GCX_COOP(); - m_sharedGenericTypeDependencies.Add(pMT, pDependencyMT, pDependencyMT->GetLoaderAllocator()); -} - -void Module::UpdateDictionaryOnSharedGenericTypeDependencies(MethodTable* pMT, Dictionary* pDictionary, ULONG dictionaryIndex) -{ - CONTRACTL - { - GC_TRIGGERS; - PRECONDITION(CheckPointer(pMT)); - PRECONDITION(pMT->HasInstantiation() && pMT != pMT->GetCanonicalMethodTable()); - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); - } - CONTRACTL_END - - auto lambda = [pDictionary, dictionaryIndex](OBJECTREF obj, MethodTable* pMTKey, MethodTable* pMTToUpdate) - { - _ASSERTE(!pMTToUpdate->HasSameTypeDefAs(pMTKey)); - - TypeHandle** pPerInstInfo = (TypeHandle**)pMTToUpdate->GetPerInstInfo()->GetValuePtr(); - FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDictionary); - _ASSERTE(pMTToUpdate->GetPerInstInfo()[dictionaryIndex].GetValue() == pDictionary); - - return true; // Keep walking - }; - - m_sharedGenericTypeDependencies.VisitValuesOfKey(pMT, lambda); -} - #endif // !CROSSGEN_COMPILE #endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/vm/ceeload.h b/src/coreclr/src/vm/ceeload.h index 8f95122ab5225..86b3fe347a22d 100644 --- a/src/coreclr/src/vm/ceeload.h +++ b/src/coreclr/src/vm/ceeload.h @@ -3235,20 +3235,8 @@ 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 + // For protecting dictionary layout slot expansions CrstExplicitInit m_DictionaryCrst; - -#ifndef CROSSGEN_COMPILE -private: - class SharedGenericTypeDependencyTrackerHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { }; - typedef CrossLoaderAllocatorHash SharedGenericTypeDependencyTrackerHash; - - SharedGenericTypeDependencyTrackerHash m_sharedGenericTypeDependencies; - -public: - void RecordSharedGenericTypeDependency(MethodTable* pMT, MethodTable* pDependencyMT); - void UpdateDictionaryOnSharedGenericTypeDependencies(MethodTable* pMT, Dictionary* pDictionary, ULONG dictionaryIndex); -#endif //!CROSSGEN_COMPILE }; // diff --git a/src/coreclr/src/vm/class.cpp b/src/coreclr/src/vm/class.cpp index 41adeb67146f1..2b123af9d02df 100644 --- a/src/coreclr/src/vm/class.cpp +++ b/src/coreclr/src/vm/class.cpp @@ -967,87 +967,12 @@ 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(); 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; - } - - { - CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - - MethodTable* pParentMT = pMT->GetParentMethodTable(); - if (pParentMT != NULL && pParentMT->HasPerInstInfo()) - { - // 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. - - DWORD nDicts = pParentMT->GetNumDicts(); - for (DWORD iDict = 0; iDict < nDicts; iDict++) - { - if (pMT->GetPerInstInfo()[iDict].GetValueMaybeNull() != pParentMT->GetPerInstInfo()[iDict].GetValueMaybeNull()) - { - 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 != pMT && pCurrentMT->HasInstantiation() && !pCurrentMT->IsCanonicalMethodTable()) - pCurrentMT->GetLoaderModule()->RecordSharedGenericTypeDependency(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/coreclr/src/vm/clsload.hpp b/src/coreclr/src/vm/clsload.hpp index cae8b6c190c1a..23a051f6aeef4 100644 --- a/src/coreclr/src/vm/clsload.hpp +++ b/src/coreclr/src/vm/clsload.hpp @@ -971,8 +971,6 @@ 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/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index eebd822c40052..85affd440284c 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -883,9 +883,6 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT) ULONG dictionaryIndex = pMT->GetNumDicts() - 1; TypeHandle** pPerInstInfo = (TypeHandle**)pMT->GetPerInstInfo()->GetValuePtr(); FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDictionary); - - // Update dictionary pointer on derived types - pMT->GetLoaderModule()->UpdateDictionaryOnSharedGenericTypeDependencies(pMT, pDictionary, dictionaryIndex); } #endif diff --git a/src/coreclr/src/vm/jithelpers.cpp b/src/coreclr/src/vm/jithelpers.cpp index 57c314fe0feeb..263a29d00832d 100644 --- a/src/coreclr/src/vm/jithelpers.cpp +++ b/src/coreclr/src/vm/jithelpers.cpp @@ -3307,12 +3307,11 @@ CORINFO_GENERIC_HANDLE JIT_GenericHandleWorker(MethodDesc * pMD, MethodTable * p GC_TRIGGERS; } CONTRACTL_END; - MethodTable * pDeclaringMT = NULL; + ULONG dictionaryIndex = 0; + MethodTable * pDeclaringMT = NULL; if (pMT != NULL) { - ULONG dictionaryIndex = 0; - if (pModule != NULL) { #ifdef _DEBUG @@ -3387,6 +3386,20 @@ CORINFO_GENERIC_HANDLE JIT_GenericHandleWorker(MethodDesc * pMD, MethodTable * p AddToGenericHandleCache(&key, (HashDatum)result); } + if (pMT != NULL && pDeclaringMT != pMT) + { + // If the dictionary on the base type got expanded, update the current type's base type dictionary + // pointer to use the new one on the base type. + + Dictionary* pMTDictionary = pMT->GetPerInstInfo()[dictionaryIndex].GetValue(); + Dictionary* pDeclaringMTDictionary = pDeclaringMT->GetPerInstInfo()[dictionaryIndex].GetValue(); + if (pMTDictionary != pDeclaringMTDictionary) + { + TypeHandle** pPerInstInfo = (TypeHandle**)pMT->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDeclaringMTDictionary); + } + } + return result; } // JIT_GenericHandleWorker diff --git a/src/coreclr/src/vm/method.hpp b/src/coreclr/src/vm/method.hpp index 22cdabe8bb1f0..ad778a8235fb1 100644 --- a/src/coreclr/src/vm/method.hpp +++ b/src/coreclr/src/vm/method.hpp @@ -3468,7 +3468,7 @@ class InstantiatedMethodDesc : public MethodDesc Instantiation IMD_GetMethodInstantiation() { LIMITED_METHOD_DAC_CONTRACT; - + // 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 returns the new or old dictionaries, the diff --git a/src/coreclr/src/vm/methodtable.h b/src/coreclr/src/vm/methodtable.h index 52457393f4ae2..0c70148927780 100644 --- a/src/coreclr/src/vm/methodtable.h +++ b/src/coreclr/src/vm/methodtable.h @@ -2936,7 +2936,6 @@ class MethodTable pInfo->m_wNumDicts = numDicts; pInfo->m_wNumTyPars = numTyPars; } - #endif // !DACCESS_COMPILE PTR_GenericsDictInfo GetGenericsDictInfo() { diff --git a/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs index ef860f02f395b..551a146366ba1 100644 --- a/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs +++ b/src/coreclr/tests/src/Loader/classloader/DictionaryExpansion/DictionaryExpansion.cs @@ -327,8 +327,8 @@ public static int Main() // After all expansions to existing dictionaries, use GenDerived4. GenDerived4 was allocated before any of its // base type dictionaries were expanded. - Debugger.Break(); - deriv4.VFunc(); + for(int i = 0; i < 5; i++) + deriv4.VFunc(); return 100; } From 11b058f9f02b649b462be2919a73d34217f8c0bf Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 19 Feb 2020 13:41:07 -0800 Subject: [PATCH 06/11] Update documentation --- docs/design/coreclr/botr/shared-generics.md | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/docs/design/coreclr/botr/shared-generics.md b/docs/design/coreclr/botr/shared-generics.md index a36ad64e4c8c1..046a39af6c7d4 100644 --- a/docs/design/coreclr/botr/shared-generics.md +++ b/docs/design/coreclr/botr/shared-generics.md @@ -186,16 +186,6 @@ This size check is **not** done unconditionally every time we need to read a val Dictionaries on types and methods are expanded by the `Dictionary::GetTypeDictionaryWithSizeCheck()` and `Dictionary::GetMethodDictionaryWithSizeCheck()` helper functions in [genericdict.cpp](https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/genericdict.cpp). -One thing to note regarding types is that they can inherit dictionary pointers from their base types. This means that if we resize the generic dictionary on any given generic type, we will need to propagate the new dictionary pointer to all of its derived types. To do that, we keep track of such dependencies using a hash-table where the key is the MethodTable pointer of the generic base types with dictionaries, and the values are the MethodTable pointers of all types that derive from these base types. See: `Module::RecordSharedGenericTypeDependency()` and `Module::UpdateDictionaryOnSharedGenericTypeDependencies()` in [ceeload.cpp](https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/ceeload.cpp). +One thing to note regarding types is that they can inherit dictionary pointers from their base types. This means that if we resize the generic dictionary on any given generic type, we will need to propagate the new dictionary pointer to all of its derived types. This propagation is also done in a lazy way whenever the code calls into the `JIT_GenericHandleWorker` helper function with a derived type MethodTable pointer. In that helper, if we find that the dictionary pointer on the base type has been updated, we copy it to the derived type. Old dictionaries are not deallocated after resizing, 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. - - -### Diagnostics - -To help diagnose runtime failures caused by the expandable nature of generic dictionaries, each dynamically allocated dictionary will have a pointer to its predecessor. Tracking back dictionaries using these pointers can help in diagnosing memory access issues if for any reason a value is read from an old dictionary of smaller size. - -These predecessor pointers are allocated at the beginning 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 across the chain of dynamically allocated dictionaries (dotnet/diagnostics#588). - From 7ad02f89f501a75f40b94c076ebef2b5d845117d Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Wed, 19 Feb 2020 14:19:26 -0800 Subject: [PATCH 07/11] Fix build error --- src/coreclr/src/vm/genericdict.cpp | 7 ++++--- src/coreclr/src/vm/genericdict.h | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index 85affd440284c..c3f0ad0baafc4 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -785,15 +785,16 @@ DWORD Dictionary::GetDictionarySlotsSizeForType(MethodTable* pMT) return *(DWORD*)pSizeSlot; } -DWORD Dictionary::GetDictionarySlotsSizeForMethod(InstantiatedMethodDesc* pIMD) +DWORD Dictionary::GetDictionarySlotsSizeForMethod(MethodDesc* pMD) { CONTRACTL { - PRECONDITION(pIMD->IMD_GetMethodDictionary() != NULL); + PRECONDITION(pMD->AsInstantiatedMethodDesc()->IMD_GetMethodDictionary() != NULL); PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END; + InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); TADDR* pDictionarySlots = (TADDR*)pIMD->IMD_GetMethodDictionary(); TADDR* pSizeSlot = pDictionarySlots + pIMD->GetNumGenericMethodArgs(); return *(DWORD*)pSizeSlot; @@ -817,7 +818,7 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); - DWORD currentDictLayoutSize = GetDictionarySlotsSizeForMethod(pIMD); + DWORD currentDictLayoutSize = GetDictionarySlotsSizeForMethod(pMD); DWORD expectedDictLayoutSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); if (currentDictLayoutSize != expectedDictLayoutSize) { diff --git a/src/coreclr/src/vm/genericdict.h b/src/coreclr/src/vm/genericdict.h index 71331cdb30d31..8b45f7901dfac 100644 --- a/src/coreclr/src/vm/genericdict.h +++ b/src/coreclr/src/vm/genericdict.h @@ -316,7 +316,7 @@ class Dictionary private: static DWORD GetDictionarySlotsSizeForType(MethodTable* pMT); - static DWORD GetDictionarySlotsSizeForMethod(InstantiatedMethodDesc* pIMD); + static DWORD GetDictionarySlotsSizeForMethod(MethodDesc* pMD); static Dictionary* GetTypeDictionaryWithSizeCheck(MethodTable* pMT); static Dictionary* GetMethodDictionaryWithSizeCheck(MethodDesc* pMD); From 51966dd84c0c672446df5b91648d733f2d026413 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Thu, 20 Feb 2020 10:08:33 -0800 Subject: [PATCH 08/11] Expand dictionary only when slot needed is beyond dictionary size --- src/coreclr/src/vm/genericdict.cpp | 60 +++++++++++++++--------------- src/coreclr/src/vm/genericdict.h | 4 +- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index c3f0ad0baafc4..2af4b000be2e5 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -800,7 +800,7 @@ DWORD Dictionary::GetDictionarySlotsSizeForMethod(MethodDesc* pMD) return *(DWORD*)pSizeSlot; } -Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) +Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD, ULONG slotIndex) { CONTRACT(Dictionary*) { @@ -814,27 +814,26 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) Dictionary* pDictionary = pMD->GetMethodDictionary(); #if !defined(CROSSGEN_COMPILE) - DictionaryLayout* pDictLayout = pMD->GetDictionaryLayout(); - InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); - _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + DWORD currentDictionarySize = GetDictionarySlotsSizeForMethod(pMD); - DWORD currentDictLayoutSize = GetDictionarySlotsSizeForMethod(pMD); - DWORD expectedDictLayoutSize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); - if (currentDictLayoutSize != expectedDictLayoutSize) + if (currentDictionarySize <= (slotIndex * sizeof(DictionaryEntry))) { - _ASSERT(currentDictLayoutSize < expectedDictLayoutSize); + // Only expand the dictionary if the current slot we're trying to use is beyond the size of the dictionary + + DictionaryLayout* pDictLayout = pMD->GetDictionaryLayout(); + InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); + _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); - pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictLayoutSize)); - ZeroMemory(pDictionary, expectedDictLayoutSize); + DWORD expectedDictionarySize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); + _ASSERT(currentDictionarySize < expectedDictionarySize); + + pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictionarySize)); // Copy old dictionary entry contents - memcpy(pDictionary, (const void*)pIMD->m_pPerInstInfo.GetValue(), currentDictLayoutSize); + memcpy(pDictionary, (const void*)pIMD->m_pPerInstInfo.GetValue(), currentDictionarySize); DWORD* pSizeSlot = (DWORD*)(pDictionary + pIMD->GetNumGenericMethodArgs()); - *pSizeSlot = expectedDictLayoutSize; - - // Flush any write buffers before publishing the new dictionary contents - FlushProcessWriteBuffers(); + *pSizeSlot = expectedDictionarySize; // Publish the new dictionary slots to the type. FastInterlockExchangePointer((TypeHandle**)pIMD->m_pPerInstInfo.GetValuePtr(), (TypeHandle*)pDictionary); @@ -844,7 +843,7 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD) RETURN pDictionary; } -Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT) +Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT, ULONG slotIndex) { CONTRACT(Dictionary*) { @@ -858,27 +857,26 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT) Dictionary* pDictionary = pMT->GetDictionary(); #if !defined(CROSSGEN_COMPILE) - DictionaryLayout* pDictLayout = pMT->GetClass()->GetDictionaryLayout(); - _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + DWORD currentDictionarySize = GetDictionarySlotsSizeForType(pMT); - DWORD currentDictLayoutSize = GetDictionarySlotsSizeForType(pMT); - DWORD expectedDictLayoutSize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); - if (currentDictLayoutSize != expectedDictLayoutSize) + if (currentDictionarySize <= (slotIndex * sizeof(DictionaryEntry))) { - _ASSERT(currentDictLayoutSize < expectedDictLayoutSize); + // Only expand the dictionary if the current slot we're trying to use is beyond the size of the dictionary + + DictionaryLayout* pDictLayout = pMT->GetClass()->GetDictionaryLayout(); + _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + + DWORD expectedDictionarySize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); + _ASSERT(currentDictionarySize < expectedDictionarySize); // Expand type dictionary - pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictLayoutSize)); - ZeroMemory(pDictionary, expectedDictLayoutSize); + pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictionarySize)); // Copy old dictionary entry contents - memcpy(pDictionary, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), currentDictLayoutSize); + memcpy(pDictionary, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), currentDictionarySize); DWORD* pSizeSlot = (DWORD*)(pDictionary + pMT->GetNumGenericArgs()); - *pSizeSlot = expectedDictLayoutSize; - - // Flush any write buffers before publishing the new dictionary contents - FlushProcessWriteBuffers(); + *pSizeSlot = expectedDictionarySize; // Publish the new dictionary slots to the type. ULONG dictionaryIndex = pMT->GetNumDicts() - 1; @@ -1499,8 +1497,8 @@ Dictionary::PopulateEntry( CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); Dictionary* pDictionary = (pMT != NULL) ? - GetTypeDictionaryWithSizeCheck(pMT) : - GetMethodDictionaryWithSizeCheck(pMD); + GetTypeDictionaryWithSizeCheck(pMT, slotIndex) : + GetMethodDictionaryWithSizeCheck(pMD, slotIndex); *(pDictionary->GetSlotAddr(0, slotIndex)) = result; *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); diff --git a/src/coreclr/src/vm/genericdict.h b/src/coreclr/src/vm/genericdict.h index 8b45f7901dfac..75d4554879d27 100644 --- a/src/coreclr/src/vm/genericdict.h +++ b/src/coreclr/src/vm/genericdict.h @@ -318,8 +318,8 @@ class Dictionary static DWORD GetDictionarySlotsSizeForType(MethodTable* pMT); static DWORD GetDictionarySlotsSizeForMethod(MethodDesc* pMD); - static Dictionary* GetTypeDictionaryWithSizeCheck(MethodTable* pMT); - static Dictionary* GetMethodDictionaryWithSizeCheck(MethodDesc* pMD); + static Dictionary* GetTypeDictionaryWithSizeCheck(MethodTable* pMT, ULONG slotIndex); + static Dictionary* GetMethodDictionaryWithSizeCheck(MethodDesc* pMD, ULONG slotIndex); #endif // #ifndef DACCESS_COMPILE From 447ea44af2d6382d274948b300813ca6232158df Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 24 Feb 2020 11:37:31 -0800 Subject: [PATCH 09/11] CR feedback --- src/coreclr/src/vm/genericdict.cpp | 98 ++++++++++++++--------- src/coreclr/src/vm/generics.cpp | 15 ++-- src/coreclr/src/vm/genmeth.cpp | 11 +-- src/coreclr/src/vm/method.hpp | 7 +- src/coreclr/src/vm/methodtablebuilder.cpp | 7 +- 5 files changed, 82 insertions(+), 56 deletions(-) diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index 2af4b000be2e5..6ad04618524e5 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -268,10 +268,11 @@ DictionaryLayout* DictionaryLayout::ExpandDictionaryLayout(LoaderAllocator* _ASSERTE(pCurrentDictLayout->m_slots[pCurrentDictLayout->m_numSlots - 1].m_signature != NULL); #ifdef _DEBUG - // Stress debug mode by increasing size by only 1 slot - if (!FitsIn((DWORD)pCurrentDictLayout->m_numSlots + 1)) + // Stress debug mode by increasing size by only 1 slot for the first 10 slots. + DWORD newSize = pCurrentDictLayout->m_numSlots > 10 ? (DWORD)pCurrentDictLayout->m_numSlots * 2 : pCurrentDictLayout->m_numSlots + 1; + if (!FitsIn(newSize)) return NULL; - DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots + 1, pAllocator, NULL); + DictionaryLayout* pNewDictionaryLayout = Allocate((WORD)newSize, pAllocator, NULL); #else if (!FitsIn((DWORD)pCurrentDictLayout->m_numSlots * 2)) return NULL; @@ -776,7 +777,6 @@ DWORD Dictionary::GetDictionarySlotsSizeForType(MethodTable* pMT) PRECONDITION(pMT->GetGenericsDictInfo()->m_wNumTyPars > 0); PRECONDITION(pMT->GetClass()->GetDictionaryLayout() != NULL); PRECONDITION(pMT->GetClass()->GetDictionaryLayout()->GetMaxSlots() > 0); - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END; @@ -790,7 +790,6 @@ DWORD Dictionary::GetDictionarySlotsSizeForMethod(MethodDesc* pMD) CONTRACTL { PRECONDITION(pMD->AsInstantiatedMethodDesc()->IMD_GetMethodDictionary() != NULL); - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); } CONTRACTL_END; @@ -806,7 +805,6 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD, ULONG { THROWS; GC_TRIGGERS; - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); POSTCONDITION(CheckPointer(RETVAL)); } CONTRACT_END; @@ -820,23 +818,38 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD, ULONG { // Only expand the dictionary if the current slot we're trying to use is beyond the size of the dictionary - DictionaryLayout* pDictLayout = pMD->GetDictionaryLayout(); - InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); - _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + // Take lock and check for size again, just in case another thread already resized the dictionary + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - DWORD expectedDictionarySize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); - _ASSERT(currentDictionarySize < expectedDictionarySize); + pDictionary = pMD->GetMethodDictionary(); + currentDictionarySize = GetDictionarySlotsSizeForMethod(pMD); - pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictionarySize)); + if (currentDictionarySize <= (slotIndex * sizeof(DictionaryEntry))) + { + DictionaryLayout* pDictLayout = pMD->GetDictionaryLayout(); + InstantiatedMethodDesc* pIMD = pMD->AsInstantiatedMethodDesc(); + _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); - // Copy old dictionary entry contents - memcpy(pDictionary, (const void*)pIMD->m_pPerInstInfo.GetValue(), currentDictionarySize); + DWORD expectedDictionarySize = DictionaryLayout::GetDictionarySizeFromLayout(pMD->GetNumGenericMethodArgs(), pDictLayout); + _ASSERT(currentDictionarySize < expectedDictionarySize); + + pDictionary = (Dictionary*)(void*)pIMD->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictionarySize)); + + // Copy old dictionary entry contents + DictionaryEntry* pOldEntriesPtr = (DictionaryEntry*)pIMD->m_pPerInstInfo.GetValue(); + DictionaryEntry* pNewEntriesPtr = (DictionaryEntry*)pDictionary; + for (int i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) + { + DictionaryEntry pSlotValue = VolatileLoadWithoutBarrier(pOldEntriesPtr); + VolatileStoreWithoutBarrier(pNewEntriesPtr, pSlotValue); + } - DWORD* pSizeSlot = (DWORD*)(pDictionary + pIMD->GetNumGenericMethodArgs()); - *pSizeSlot = expectedDictionarySize; + DWORD* pSizeSlot = (DWORD*)(pDictionary + pIMD->GetNumGenericMethodArgs()); + *pSizeSlot = expectedDictionarySize; - // Publish the new dictionary slots to the type. - FastInterlockExchangePointer((TypeHandle**)pIMD->m_pPerInstInfo.GetValuePtr(), (TypeHandle*)pDictionary); + // Publish the new dictionary slots to the type. + FastInterlockExchangePointer((TypeHandle**)pIMD->m_pPerInstInfo.GetValuePtr(), (TypeHandle*)pDictionary); + } } #endif @@ -849,7 +862,6 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT, ULONG s { THROWS; GC_TRIGGERS; - PRECONDITION(SystemDomain::SystemModule()->m_DictionaryCrst.OwnedByCurrentThread()); POSTCONDITION(CheckPointer(RETVAL)); } CONTRACT_END; @@ -863,25 +875,40 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT, ULONG s { // Only expand the dictionary if the current slot we're trying to use is beyond the size of the dictionary - DictionaryLayout* pDictLayout = pMT->GetClass()->GetDictionaryLayout(); - _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); + // Take lock and check for size again, just in case another thread already resized the dictionary + CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - DWORD expectedDictionarySize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); - _ASSERT(currentDictionarySize < expectedDictionarySize); + pDictionary = pMT->GetDictionary(); + currentDictionarySize = GetDictionarySlotsSizeForType(pMT); - // Expand type dictionary - pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictionarySize)); + if (currentDictionarySize <= (slotIndex * sizeof(DictionaryEntry))) + { + DictionaryLayout* pDictLayout = pMT->GetClass()->GetDictionaryLayout(); + _ASSERTE(pDictLayout != NULL && pDictLayout->GetMaxSlots() > 0); - // Copy old dictionary entry contents - memcpy(pDictionary, (const void*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(), currentDictionarySize); + DWORD expectedDictionarySize = DictionaryLayout::GetDictionarySizeFromLayout(pMT->GetNumGenericArgs(), pDictLayout); + _ASSERT(currentDictionarySize < expectedDictionarySize); - DWORD* pSizeSlot = (DWORD*)(pDictionary + pMT->GetNumGenericArgs()); - *pSizeSlot = expectedDictionarySize; + // Expand type dictionary + pDictionary = (Dictionary*)(void*)pMT->GetLoaderAllocator()->GetHighFrequencyHeap()->AllocMem(S_SIZE_T(expectedDictionarySize)); - // Publish the new dictionary slots to the type. - ULONG dictionaryIndex = pMT->GetNumDicts() - 1; - TypeHandle** pPerInstInfo = (TypeHandle**)pMT->GetPerInstInfo()->GetValuePtr(); - FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDictionary); + // Copy old dictionary entry contents + DictionaryEntry* pOldEntriesPtr = (DictionaryEntry*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(); + DictionaryEntry* pNewEntriesPtr = (DictionaryEntry*)pDictionary; + for (int i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) + { + DictionaryEntry pSlotValue = VolatileLoadWithoutBarrier(pOldEntriesPtr); + VolatileStoreWithoutBarrier(pNewEntriesPtr, pSlotValue); + } + + DWORD* pSizeSlot = (DWORD*)(pDictionary + pMT->GetNumGenericArgs()); + *pSizeSlot = expectedDictionarySize; + + // Publish the new dictionary slots to the type. + ULONG dictionaryIndex = pMT->GetNumDicts() - 1; + TypeHandle** pPerInstInfo = (TypeHandle**)pMT->GetPerInstInfo()->GetValuePtr(); + FastInterlockExchangePointer(pPerInstInfo + dictionaryIndex, (TypeHandle*)pDictionary); + } } #endif @@ -1493,14 +1520,11 @@ Dictionary::PopulateEntry( if ((slotIndex != 0) && !IsCompilationProcess()) { - // Lock is needed because dictionary pointers can get updated during dictionary size expansion - CrstHolder ch(&SystemDomain::SystemModule()->m_DictionaryCrst); - Dictionary* pDictionary = (pMT != NULL) ? GetTypeDictionaryWithSizeCheck(pMT, slotIndex) : GetMethodDictionaryWithSizeCheck(pMD, slotIndex); - *(pDictionary->GetSlotAddr(0, slotIndex)) = result; + VolatileStoreWithoutBarrier(pDictionary->GetSlotAddr(0, slotIndex), (DictionaryEntry)result); *ppSlot = pDictionary->GetSlotAddr(0, slotIndex); } } diff --git a/src/coreclr/src/vm/generics.cpp b/src/coreclr/src/vm/generics.cpp index 50819398a3b2e..5a19aac5f9556 100644 --- a/src/coreclr/src/vm/generics.cpp +++ b/src/coreclr/src/vm/generics.cpp @@ -278,10 +278,10 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( DWORD cbPerInst = sizeof(GenericsDictInfo) + pOldMT->GetPerInstInfoSize(); // Finally we need space for the instantiation/dictionary for this type - // 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. This is - // not a problem anyways because whenever we load a value from the dictionary after a certain index, we will - // always check the size of the dictionary and expand it if needed + // Note that it is possible for the dictionary layout to be expanded in size by other threads while we're still + // creating this type. In other words: this type will have a smaller dictionary that its layout. This is not a + // problem however because whenever we need to load a value from the dictionary of this type beyond its size, we + // will expand the dictionary at that point. DWORD cbInstAndDict = pOldMT->GetInstAndDictSize(); // Allocate from the high frequence heap of the correct domain @@ -493,10 +493,11 @@ ClassLoader::CreateTypeHandleForNonCanonicalGenericInstantiation( } PTR_DictionaryLayout pLayout = pOldMT->GetClass()->GetDictionaryLayout(); - if (pLayout != NULL && pLayout->GetMaxSlots() > 0) + if (pLayout != NULL) { - ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); - ULONG_PTR* pSizeSlot = pDictionarySlots + ntypars; + _ASSERTE(pLayout->GetMaxSlots() > 0); + PTR_Dictionary pDictionarySlots = pMT->GetPerInstInfo()[pOldMT->GetNumDicts() - 1].GetValue(); + DWORD* pSizeSlot = (DWORD*)(pDictionarySlots + ntypars); *pSizeSlot = cbInstAndDict; } diff --git a/src/coreclr/src/vm/genmeth.cpp b/src/coreclr/src/vm/genmeth.cpp index 0ea124508c5de..4c2b3c5e8010e 100644 --- a/src/coreclr/src/vm/genmeth.cpp +++ b/src/coreclr/src/vm/genmeth.cpp @@ -373,9 +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 load values from a generic dictionary beyond a certain index, we will check the size of - // the dictionary and expand it if needed. + // Note that it is possible for the dictionary layout to be expanded in size by other threads while we're still + // creating this method. In other words: this method will have a smaller dictionary that its layout. This is not a + // problem however because whenever we need to load a value from the dictionary of this method beyond its size, we + // will expand the dictionary at that point. pDL = pWrappedMD->AsInstantiatedMethodDesc()->GetDictLayoutRaw(); } } @@ -402,9 +403,9 @@ InstantiatedMethodDesc::NewInstantiatedMethodDesc(MethodTable *pExactMT, { // 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*))); + _ASSERTE(infoSize > sizeof(TypeHandle*) * (methodInst.GetNumArgs() + 1)); - ULONG_PTR* pDictSizeSlot = ((ULONG_PTR*)pInstOrPerInstInfo) + methodInst.GetNumArgs(); + DWORD* pDictSizeSlot = (DWORD*)(pInstOrPerInstInfo + methodInst.GetNumArgs()); *pDictSizeSlot = infoSize; } } diff --git a/src/coreclr/src/vm/method.hpp b/src/coreclr/src/vm/method.hpp index ad778a8235fb1..53d581ade05ed 100644 --- a/src/coreclr/src/vm/method.hpp +++ b/src/coreclr/src/vm/method.hpp @@ -3469,10 +3469,9 @@ class InstantiatedMethodDesc : public MethodDesc { LIMITED_METHOD_DAC_CONTRACT; - // 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 returns the new or old dictionaries, the - // values of the instantiation arguments will always be the same. + // No lock needed here. 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. Whether we + // return the old or new dictionary here, the values of the instantiation arguments will always be the same. return Instantiation(IMD_GetMethodDictionary()->GetInstantiation(), m_wNumGenericArgs); } diff --git a/src/coreclr/src/vm/methodtablebuilder.cpp b/src/coreclr/src/vm/methodtablebuilder.cpp index 1dfde1645cefb..ada4aa25f8a8a 100644 --- a/src/coreclr/src/vm/methodtablebuilder.cpp +++ b/src/coreclr/src/vm/methodtablebuilder.cpp @@ -10462,10 +10462,11 @@ MethodTableBuilder::SetupMethodTable2( pInstDest[j] = inst[j]; } - if (pClass->GetDictionaryLayout() != NULL && pClass->GetDictionaryLayout()->GetMaxSlots() > 0) + PTR_DictionaryLayout pLayout = pClass->GetDictionaryLayout(); + if (pLayout != NULL && pLayout->GetMaxSlots() > 0) { - ULONG_PTR* pDictionarySlots = (ULONG_PTR*)pMT->GetPerInstInfo()[bmtGenerics->numDicts - 1].GetValue(); - ULONG_PTR* pSizeSlot = pDictionarySlots + bmtGenerics->GetNumGenericArgs(); + PTR_Dictionary pDictionarySlots = pMT->GetPerInstInfo()[bmtGenerics->numDicts - 1].GetValue(); + DWORD* pSizeSlot = (DWORD*)(pDictionarySlots + bmtGenerics->GetNumGenericArgs()); *pSizeSlot = cbDict; } } From 728d88f66cef4305e8f443a23d7659fcaeee65bc Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 24 Feb 2020 11:59:05 -0800 Subject: [PATCH 10/11] Nit --- src/coreclr/src/vm/genericdict.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index 6ad04618524e5..f92c3c964a82d 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -840,8 +840,7 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD, ULONG DictionaryEntry* pNewEntriesPtr = (DictionaryEntry*)pDictionary; for (int i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) { - DictionaryEntry pSlotValue = VolatileLoadWithoutBarrier(pOldEntriesPtr); - VolatileStoreWithoutBarrier(pNewEntriesPtr, pSlotValue); + *pNewEntriesPtr = VolatileLoadWithoutBarrier(pOldEntriesPtr); } DWORD* pSizeSlot = (DWORD*)(pDictionary + pIMD->GetNumGenericMethodArgs()); @@ -897,8 +896,7 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT, ULONG s DictionaryEntry* pNewEntriesPtr = (DictionaryEntry*)pDictionary; for (int i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) { - DictionaryEntry pSlotValue = VolatileLoadWithoutBarrier(pOldEntriesPtr); - VolatileStoreWithoutBarrier(pNewEntriesPtr, pSlotValue); + *pNewEntriesPtr = VolatileLoadWithoutBarrier(pOldEntriesPtr); } DWORD* pSizeSlot = (DWORD*)(pDictionary + pMT->GetNumGenericArgs()); From dd228013bde54fae6962ae9c28e7405e718ae9e5 Mon Sep 17 00:00:00 2001 From: Fadi Hanna Date: Mon, 24 Feb 2020 13:25:44 -0800 Subject: [PATCH 11/11] Fix build error on 32 bit platforms --- src/coreclr/src/vm/genericdict.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/genericdict.cpp b/src/coreclr/src/vm/genericdict.cpp index f92c3c964a82d..82016e1e7d2c1 100644 --- a/src/coreclr/src/vm/genericdict.cpp +++ b/src/coreclr/src/vm/genericdict.cpp @@ -838,7 +838,7 @@ Dictionary* Dictionary::GetMethodDictionaryWithSizeCheck(MethodDesc* pMD, ULONG // Copy old dictionary entry contents DictionaryEntry* pOldEntriesPtr = (DictionaryEntry*)pIMD->m_pPerInstInfo.GetValue(); DictionaryEntry* pNewEntriesPtr = (DictionaryEntry*)pDictionary; - for (int i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) + for (DWORD i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) { *pNewEntriesPtr = VolatileLoadWithoutBarrier(pOldEntriesPtr); } @@ -894,7 +894,7 @@ Dictionary* Dictionary::GetTypeDictionaryWithSizeCheck(MethodTable* pMT, ULONG s // Copy old dictionary entry contents DictionaryEntry* pOldEntriesPtr = (DictionaryEntry*)pMT->GetPerInstInfo()[pMT->GetNumDicts() - 1].GetValue(); DictionaryEntry* pNewEntriesPtr = (DictionaryEntry*)pDictionary; - for (int i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) + for (DWORD i = 0; i < currentDictionarySize / sizeof(DictionaryEntry); i++, pOldEntriesPtr++, pNewEntriesPtr++) { *pNewEntriesPtr = VolatileLoadWithoutBarrier(pOldEntriesPtr); }