From dffeab9eacd47d41e21fbcea3e774de8dd57ff8b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 4 Nov 2021 16:49:06 -0700 Subject: [PATCH 1/7] use HashTypeKey(), remove ComputeHash() --- src/coreclr/vm/pendingload.cpp | 8 +++++--- src/coreclr/vm/typehash.cpp | 2 +- src/coreclr/vm/typehash.h | 2 ++ src/coreclr/vm/typekey.h | 28 ---------------------------- 4 files changed, 8 insertions(+), 32 deletions(-) diff --git a/src/coreclr/vm/pendingload.cpp b/src/coreclr/vm/pendingload.cpp index 597a895b3317a..78c468f8c7274 100644 --- a/src/coreclr/vm/pendingload.cpp +++ b/src/coreclr/vm/pendingload.cpp @@ -114,7 +114,7 @@ BOOL PendingTypeLoadTable::InsertValue(PendingTypeLoadEntry *pData) _ASSERTE(m_dwNumBuckets != 0); - DWORD dwHash = pData->GetTypeKey().ComputeHash(); + DWORD dwHash = HashTypeKey(&pData->GetTypeKey()); DWORD dwBucket = dwHash % m_dwNumBuckets; PendingTypeLoadTable::TableEntry * pNewEntry = AllocNewEntry(); if (pNewEntry == NULL) @@ -130,6 +130,7 @@ BOOL PendingTypeLoadTable::InsertValue(PendingTypeLoadEntry *pData) return TRUE; } +static int CollisionCount; BOOL PendingTypeLoadTable::DeleteValue(TypeKey *pKey) { @@ -145,7 +146,7 @@ BOOL PendingTypeLoadTable::DeleteValue(TypeKey *pKey) _ASSERTE(m_dwNumBuckets != 0); - DWORD dwHash = pKey->ComputeHash(); + DWORD dwHash = HashTypeKey(pKey); DWORD dwBucket = dwHash % m_dwNumBuckets; PendingTypeLoadTable::TableEntry * pSearch; PendingTypeLoadTable::TableEntry **ppPrev = &m_pBuckets[dwBucket]; @@ -161,6 +162,7 @@ BOOL PendingTypeLoadTable::DeleteValue(TypeKey *pKey) } ppPrev = &pSearch->pNext; + printf("CollisionCount: %i \n", ++CollisionCount); } return FALSE; @@ -182,7 +184,7 @@ PendingTypeLoadTable::TableEntry *PendingTypeLoadTable::FindItem(TypeKey *pKey) _ASSERTE(m_dwNumBuckets != 0); - DWORD dwHash = pKey->ComputeHash(); + DWORD dwHash = HashTypeKey(pKey); DWORD dwBucket = dwHash % m_dwNumBuckets; PendingTypeLoadTable::TableEntry * pSearch; diff --git a/src/coreclr/vm/typehash.cpp b/src/coreclr/vm/typehash.cpp index e994e797477fc..ff8cff8cfe2b6 100644 --- a/src/coreclr/vm/typehash.cpp +++ b/src/coreclr/vm/typehash.cpp @@ -251,7 +251,7 @@ static DWORD HashTypeHandle(DWORD level, TypeHandle t) } // Calculate hash value from key -static DWORD HashTypeKey(TypeKey* pKey) +DWORD HashTypeKey(TypeKey* pKey) { CONTRACTL { diff --git a/src/coreclr/vm/typehash.h b/src/coreclr/vm/typehash.h index 5df8dec8682cd..b705bd2a7801a 100644 --- a/src/coreclr/vm/typehash.h +++ b/src/coreclr/vm/typehash.h @@ -26,6 +26,8 @@ // //======================================================================================== +DWORD HashTypeKey(TypeKey* pKey); + // One of these is present for each element in the table // It simply chains together (hash,data) pairs typedef DPTR(struct EETypeHashEntry) PTR_EETypeHashEntry; diff --git a/src/coreclr/vm/typekey.h b/src/coreclr/vm/typekey.h index 45b186748f3bc..57044cc5f7aaa 100644 --- a/src/coreclr/vm/typekey.h +++ b/src/coreclr/vm/typekey.h @@ -213,7 +213,6 @@ class TypeKey return TypeKey::Equals(this, pKey); } - // Comparison and hashing static BOOL Equals(const TypeKey *pKey1, const TypeKey *pKey2) { WRAPPER_NO_CONTRACT; @@ -257,33 +256,6 @@ class TypeKey return TRUE; } } - - DWORD ComputeHash() const - { - LIMITED_METHOD_CONTRACT; - DWORD_PTR hashLarge; - - if (m_kind == ELEMENT_TYPE_CLASS) - { - hashLarge = ((DWORD_PTR)u.asClass.m_pModule ^ (DWORD_PTR)u.asClass.m_numGenericArgs ^ (DWORD_PTR)u.asClass.m_typeDef); - } - else if (CorTypeInfo::IsModifier_NoThrow(m_kind) || m_kind == ELEMENT_TYPE_VALUETYPE) - { - hashLarge = (u.asParamType.m_paramType ^ (DWORD_PTR) u.asParamType.m_rank); - } - else hashLarge = 0; - -#if POINTER_BITS == 32 - return hashLarge; -#else - DWORD hash = *(DWORD *)&hashLarge; - for (unsigned i = 1; i < POINTER_BITS / 32; i++) - { - hash ^= ((DWORD *)&hashLarge)[i]; - } - return hash; -#endif - } }; From d60393e7ed2d16c4a74f2c06aa17e80aaa0186e1 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 4 Nov 2021 17:00:28 -0700 Subject: [PATCH 2/7] nonrecursive HashTypeHandle --- src/coreclr/vm/typehash.cpp | 47 ++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/src/coreclr/vm/typehash.cpp b/src/coreclr/vm/typehash.cpp index ff8cff8cfe2b6..12b146064c8c2 100644 --- a/src/coreclr/vm/typehash.cpp +++ b/src/coreclr/vm/typehash.cpp @@ -141,10 +141,10 @@ DWORD EETypeHashTable::GetCount() return BaseGetElementCount(); } -static DWORD HashTypeHandle(DWORD level, TypeHandle t); +static DWORD HashTypeHandle(TypeHandle t); // Calculate hash value for a type def or instantiated type def -static DWORD HashPossiblyInstantiatedType(DWORD level, mdTypeDef token, Instantiation inst) +static DWORD HashPossiblyInstantiatedType(mdTypeDef token, Instantiation inst) { CONTRACTL { @@ -163,15 +163,10 @@ static DWORD HashPossiblyInstantiatedType(DWORD level, mdTypeDef token, Instanti { dwHash = ((dwHash << 5) + dwHash) ^ inst.GetNumArgs(); - // Hash two levels of the hiearchy. A simple nesting of generics instantiations is - // pretty common in generic collections, e.g.: ICollection> - if (level < 2) + // Hash n type parameters + for (DWORD i = 0; i < inst.GetNumArgs(); i++) { - // Hash n type parameters - for (DWORD i = 0; i < inst.GetNumArgs(); i++) - { - dwHash = ((dwHash << 5) + dwHash) ^ HashTypeHandle(level+1, inst[i]); - } + dwHash = ((dwHash << 5) + dwHash) ^ inst[i].AsTAddr(); } } @@ -179,7 +174,7 @@ static DWORD HashPossiblyInstantiatedType(DWORD level, mdTypeDef token, Instanti } // Calculate hash value for a function pointer type -static DWORD HashFnPtrType(DWORD level, BYTE callConv, DWORD numArgs, TypeHandle *retAndArgTypes) +static DWORD HashFnPtrType(BYTE callConv, DWORD numArgs, TypeHandle *retAndArgTypes) { WRAPPER_NO_CONTRACT; SUPPORTS_DAC; @@ -188,31 +183,29 @@ static DWORD HashFnPtrType(DWORD level, BYTE callConv, DWORD numArgs, TypeHandle dwHash = ((dwHash << 5) + dwHash) ^ ELEMENT_TYPE_FNPTR; dwHash = ((dwHash << 5) + dwHash) ^ callConv; dwHash = ((dwHash << 5) + dwHash) ^ numArgs; - if (level < 1) + + for (DWORD i = 0; i <= numArgs; i++) { - for (DWORD i = 0; i <= numArgs; i++) - { - dwHash = ((dwHash << 5) + dwHash) ^ HashTypeHandle(level+1, retAndArgTypes[i]); - } + dwHash = ((dwHash << 5) + dwHash) ^ retAndArgTypes[i].AsTAddr(); } return dwHash; } // Calculate hash value for an array/pointer/byref type -static DWORD HashParamType(DWORD level, CorElementType kind, TypeHandle typeParam) +static DWORD HashParamType(CorElementType kind, TypeHandle typeParam) { WRAPPER_NO_CONTRACT; INT_PTR dwHash = 5381; dwHash = ((dwHash << 5) + dwHash) ^ kind; - dwHash = ((dwHash << 5) + dwHash) ^ HashTypeHandle(level, typeParam); + dwHash = ((dwHash << 5) + dwHash) ^ typeParam.AsTAddr(); return dwHash; } // Calculate hash value from type handle -static DWORD HashTypeHandle(DWORD level, TypeHandle t) +static DWORD HashTypeHandle(TypeHandle t) { CONTRACTL { @@ -229,7 +222,7 @@ static DWORD HashTypeHandle(DWORD level, TypeHandle t) if (t.HasTypeParam()) { - retVal = HashParamType(level, t.GetInternalCorElementType(), t.GetTypeParam()); + retVal = HashParamType(t.GetInternalCorElementType(), t.GetTypeParam()); } else if (t.IsGenericVariable()) { @@ -237,15 +230,15 @@ static DWORD HashTypeHandle(DWORD level, TypeHandle t) } else if (t.HasInstantiation()) { - retVal = HashPossiblyInstantiatedType(level, t.GetCl(), t.GetInstantiation()); + retVal = HashPossiblyInstantiatedType(t.GetCl(), t.GetInstantiation()); } else if (t.IsFnPtrType()) { FnPtrTypeDesc* pTD = t.AsFnPtrType(); - retVal = HashFnPtrType(level, pTD->GetCallConv(), pTD->GetNumArgs(), pTD->GetRetAndArgTypesPointer()); + retVal = HashFnPtrType(pTD->GetCallConv(), pTD->GetNumArgs(), pTD->GetRetAndArgTypesPointer()); } else - retVal = HashPossiblyInstantiatedType(level, t.GetCl(), Instantiation()); + retVal = HashPossiblyInstantiatedType(t.GetCl(), Instantiation()); return retVal; } @@ -265,15 +258,15 @@ DWORD HashTypeKey(TypeKey* pKey) if (pKey->GetKind() == ELEMENT_TYPE_CLASS) { - return HashPossiblyInstantiatedType(0, pKey->GetTypeToken(), pKey->GetInstantiation()); + return HashPossiblyInstantiatedType(pKey->GetTypeToken(), pKey->GetInstantiation()); } else if (pKey->GetKind() == ELEMENT_TYPE_FNPTR) { - return HashFnPtrType(0, pKey->GetCallConv(), pKey->GetNumArgs(), pKey->GetRetAndArgTypes()); + return HashFnPtrType(pKey->GetCallConv(), pKey->GetNumArgs(), pKey->GetRetAndArgTypes()); } else { - return HashParamType(0, pKey->GetKind(), pKey->GetElementType()); + return HashParamType(pKey->GetKind(), pKey->GetElementType()); } } @@ -552,7 +545,7 @@ VOID EETypeHashTable::InsertValue(TypeHandle data) pNewEntry->SetTypeHandle(data); - BaseInsertEntry(HashTypeHandle(0, data), pNewEntry); + BaseInsertEntry(HashTypeHandle(data), pNewEntry); } #endif // #ifndef DACCESS_COMPILE From c3d2961decc84d8cea22c02d89450ebd55ccaf1a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 4 Nov 2021 17:11:45 -0700 Subject: [PATCH 3/7] remove instrumentation from pendingload.cpp --- src/coreclr/vm/pendingload.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/vm/pendingload.cpp b/src/coreclr/vm/pendingload.cpp index 78c468f8c7274..1bc384befab56 100644 --- a/src/coreclr/vm/pendingload.cpp +++ b/src/coreclr/vm/pendingload.cpp @@ -130,8 +130,6 @@ BOOL PendingTypeLoadTable::InsertValue(PendingTypeLoadEntry *pData) return TRUE; } -static int CollisionCount; - BOOL PendingTypeLoadTable::DeleteValue(TypeKey *pKey) { CONTRACTL @@ -162,7 +160,6 @@ BOOL PendingTypeLoadTable::DeleteValue(TypeKey *pKey) } ppPrev = &pSearch->pNext; - printf("CollisionCount: %i \n", ++CollisionCount); } return FALSE; From d7b34042fc962848b537164ccd98919acf17b83c Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 4 Nov 2021 17:44:54 -0700 Subject: [PATCH 4/7] two more uses of `ComputeHash()` --- src/coreclr/vm/clsload.cpp | 2 +- src/coreclr/vm/gdbjit.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/clsload.cpp b/src/coreclr/vm/clsload.cpp index 9392f6c11d93e..02505194ffd06 100644 --- a/src/coreclr/vm/clsload.cpp +++ b/src/coreclr/vm/clsload.cpp @@ -3634,7 +3634,7 @@ ClassLoader::LoadTypeHandleForTypeKey_Body( } EX_HOOK { - LOG((LF_CLASSLOADER, LL_INFO10, "Caught an exception loading: %x, %0x (Module)\n", pTypeKey->IsConstructed() ? pTypeKey->ComputeHash() : pTypeKey->GetTypeToken(), pTypeKey->GetModule())); + LOG((LF_CLASSLOADER, LL_INFO10, "Caught an exception loading: %x, %0x (Module)\n", pTypeKey->IsConstructed() ? HashTypeKey(pTypeKey) : pTypeKey->GetTypeToken(), pTypeKey->GetModule())); if (!GetThread()->HasThreadStateNC(Thread::TSNC_LoadsTypeViolation)) { diff --git a/src/coreclr/vm/gdbjit.h b/src/coreclr/vm/gdbjit.h index 355e5ee831c5f..95f7abe79eeac 100644 --- a/src/coreclr/vm/gdbjit.h +++ b/src/coreclr/vm/gdbjit.h @@ -387,7 +387,7 @@ class NotifyGdb static count_t Hash(key_t k) { LIMITED_METHOD_CONTRACT; - return k->ComputeHash(); + return HashTypeKey(k); } static const element_t Null() { LIMITED_METHOD_CONTRACT; return element_t(key_t(),VALUE()); } From b9c3c6e2c18974947f30afadc84811ba3b93e980 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 4 Nov 2021 18:20:14 -0700 Subject: [PATCH 5/7] Make GCC happy --- src/coreclr/vm/pendingload.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/pendingload.h b/src/coreclr/vm/pendingload.h index be8cf6c521b06..2094d76f86330 100644 --- a/src/coreclr/vm/pendingload.h +++ b/src/coreclr/vm/pendingload.h @@ -166,7 +166,7 @@ class PendingTypeLoadEntry } #endif //DACCESS_COMPILE - TypeKey GetTypeKey() + TypeKey& GetTypeKey() { LIMITED_METHOD_CONTRACT; return m_typeKey; From 2769526763914d7031eb4bbdea8af544fc037c2a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 4 Nov 2021 22:59:57 -0700 Subject: [PATCH 6/7] couple tweaks --- src/coreclr/vm/typehash.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/typehash.cpp b/src/coreclr/vm/typehash.cpp index 12b146064c8c2..4453163ee5236 100644 --- a/src/coreclr/vm/typehash.cpp +++ b/src/coreclr/vm/typehash.cpp @@ -161,8 +161,6 @@ static DWORD HashPossiblyInstantiatedType(mdTypeDef token, Instantiation inst) dwHash = ((dwHash << 5) + dwHash) ^ token; if (!inst.IsEmpty()) { - dwHash = ((dwHash << 5) + dwHash) ^ inst.GetNumArgs(); - // Hash n type parameters for (DWORD i = 0; i < inst.GetNumArgs(); i++) { @@ -224,10 +222,6 @@ static DWORD HashTypeHandle(TypeHandle t) { retVal = HashParamType(t.GetInternalCorElementType(), t.GetTypeParam()); } - else if (t.IsGenericVariable()) - { - retVal = (dac_cast(t.AsTypeDesc())->GetToken()); - } else if (t.HasInstantiation()) { retVal = HashPossiblyInstantiatedType(t.GetCl(), t.GetInstantiation()); @@ -237,6 +231,11 @@ static DWORD HashTypeHandle(TypeHandle t) FnPtrTypeDesc* pTD = t.AsFnPtrType(); retVal = HashFnPtrType(pTD->GetCallConv(), pTD->GetNumArgs(), pTD->GetRetAndArgTypesPointer()); } + else if (t.IsGenericVariable()) + { + _ASSERTE(!"Generic variables are unexpected here."); + retVal = t.AsTAddr(); + } else retVal = HashPossiblyInstantiatedType(t.GetCl(), Instantiation()); From da655bd4cfc54aeed4942fcf94832e04871ee786 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 7 Nov 2021 09:12:23 -0800 Subject: [PATCH 7/7] unnecessary full fences. --- src/coreclr/vm/dacenumerablehash.inl | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/dacenumerablehash.inl b/src/coreclr/vm/dacenumerablehash.inl index a0a37381f8802..4faddb9ff558c 100644 --- a/src/coreclr/vm/dacenumerablehash.inl +++ b/src/coreclr/vm/dacenumerablehash.inl @@ -135,11 +135,9 @@ void DacEnumerableHashTable::BaseInsertEntry(DacEnumerableHa // Prepare to link the new entry at the head of the bucket chain. pVolatileEntry->m_pNextEntry = (GetBuckets())[dwBucket]; - // Make sure that all writes to the entry are visible before publishing the entry. - MemoryBarrier(); - // Publish the entry by pointing the bucket at it. - (GetBuckets())[dwBucket] = pVolatileEntry; + // Make sure that all writes to the entry are visible before publishing the entry. + VolatileStore(&(GetBuckets())[dwBucket], pVolatileEntry); m_cEntries++; @@ -207,15 +205,13 @@ void DacEnumerableHashTable::GrowTable() } // Make sure that all writes are visible before publishing the new array. - MemoryBarrier(); - m_pBuckets = pNewBuckets; + VolatileStore(&m_pBuckets, pNewBuckets); // The new number of buckets has to be published last (prior to this readers may miscalculate a bucket // index, but the result will always be in range and they'll simply walk the wrong chain and get a miss, // prompting a retry under the lock). If we let the count become visible unordered wrt to the bucket array // itself a reader could potentially read buckets from beyond the end of the old bucket list). - MemoryBarrier(); - m_cBuckets = cNewBuckets; + VolatileStore(&m_cBuckets, cNewBuckets); } // Returns the next prime larger (or equal to) than the number given.