CR feedback

Author: Fadi Hanna

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<WORD>((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<WORD>(newSize))
         return NULL;
-    DictionaryLayout* pNewDictionaryLayout = Allocate(pCurrentDictLayout->m_numSlots + 1, pAllocator, NULL);
+    DictionaryLayout* pNewDictionaryLayout = Allocate((WORD)newSize, pAllocator, NULL);
 #else
     if (!FitsIn<WORD>((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);
         }
     }

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;
     }
 

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;
             }
         }

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);
     }
 

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;
         }
     }