From 229b9bdc115766c668681d64fc7eb20ca627922d Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 2 Mar 2022 13:16:40 -0800 Subject: [PATCH 1/7] Change `CrossLoaderAllocatorHash` to not use the GC `GCHeapHash`: - Replaced `GCHeapHash` usage with `SHash` - Added `s_supports_autoremove` to `SHash` to simulate `GCHeapHash`'s ability to remove elements while iterating them. When enabled, `SHash` calls `ShouldDelete()` on elements it walks during various operations and removes them if requested - Added an optional on-remove cleanup action for elements in `SHash` to ease cleanup of heap-allocated elements `CrossLoaderAllocatorHash`: - Added formal native types for internal data structures and hash table elements. The layout of the internal data structures is mostly the same as before. - Trackers are ref-counted since they may be inserted into multiple hash tables, and get deleted once their ref count reaches zero - Translated the current implementation. Used holders where necessary to handle OOMs gracefully. Miscellaneous: - Removed cooperative GC mode usage in the relevant places in slot backpatching and inline tracking, and removed usage of the forbid-suspend-for-debugger region in those places - Fixed a potential debugger suspend deadlock when entering cooperative GC mode inside a forbid-suspend-for-debugger region (which is only used in one place after this change) - When deleting call counters, reordered such that the runtime is suspended before the slot backpatching lock is acquired, to prevent the suspension code from suspending for the debugger while the lock is held and leading to FuncEval deadlocks Testing: - Memory usage is roughly the same as before, if anything it seems to be a bit lower - Perf is similar to before, no significant change - Ran a slot backpatching test including collectible assemblies in stress mode to look for issues --- .../System.Private.CoreLib.csproj | 2 - .../CrossLoaderAllocatorHashHelpers.cs | 35 - .../Runtime/CompilerServices/GCHeapHash.cs | 19 - src/coreclr/inc/CrstTypes.def | 6 +- src/coreclr/inc/crsttypes.h | 6 +- src/coreclr/inc/shash.h | 104 +- src/coreclr/inc/shash.inl | 114 +- src/coreclr/vm/appdomain.cpp | 2 - src/coreclr/vm/appdomain.hpp | 3 +- src/coreclr/vm/callcounting.cpp | 30 +- src/coreclr/vm/ceemain.cpp | 1 + src/coreclr/vm/codeversion.cpp | 37 +- src/coreclr/vm/corelib.h | 22 - src/coreclr/vm/crossloaderallocatorhash.h | 354 ++++- src/coreclr/vm/crossloaderallocatorhash.inl | 1314 +++++++---------- src/coreclr/vm/crst.h | 10 + src/coreclr/vm/fptrstubs.cpp | 2 +- src/coreclr/vm/gcheaphashtable.h | 141 -- src/coreclr/vm/gcheaphashtable.inl | 529 ------- src/coreclr/vm/inlinetracking.cpp | 16 +- src/coreclr/vm/inlinetracking.h | 15 +- src/coreclr/vm/loaderallocator.cpp | 73 + src/coreclr/vm/loaderallocator.hpp | 20 + src/coreclr/vm/method.cpp | 2 +- src/coreclr/vm/methoddescbackpatchinfo.cpp | 6 +- src/coreclr/vm/methoddescbackpatchinfo.h | 47 +- src/coreclr/vm/object.cpp | 17 - src/coreclr/vm/object.h | 129 -- src/coreclr/vm/prestub.cpp | 3 +- src/coreclr/vm/rejit.cpp | 2 - src/coreclr/vm/threadsuspend.cpp | 20 +- src/coreclr/vm/tieredcompilation.cpp | 17 +- 32 files changed, 1168 insertions(+), 1930 deletions(-) delete mode 100644 src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs delete mode 100644 src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/GCHeapHash.cs delete mode 100644 src/coreclr/vm/gcheaphashtable.h delete mode 100644 src/coreclr/vm/gcheaphashtable.inl diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj index c9c3bca531be0..99a4a8ce520d8 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -201,8 +201,6 @@ - - diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs deleted file mode 100644 index 9bb4628a3be64..0000000000000 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs +++ /dev/null @@ -1,35 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.InteropServices; - -namespace System.Runtime.CompilerServices -{ - /// - /// Managed structure used by CrossLoaderAllocatorHeap to isolate per LoaderAllocator - /// data. - /// - [StructLayout(LayoutKind.Sequential)] - internal sealed class LAHashDependentHashTracker - { - private GCHandle _dependentHandle; - private IntPtr _loaderAllocator; - - ~LAHashDependentHashTracker() - { - if (_dependentHandle.IsAllocated) - _dependentHandle.Free(); - } - } - - /// - /// Managed structure used by CrossLoaderAllocatorHeap to hold a set of references - /// to LAHashDependentHashTracker's - /// - [StructLayout(LayoutKind.Sequential)] - internal sealed class LAHashKeyToTrackers - { - private object? _trackerOrTrackerSet; - private object? _laLocalKeyValueStore; - } -} diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/GCHeapHash.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/GCHeapHash.cs deleted file mode 100644 index 82f99197a6a7e..0000000000000 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/GCHeapHash.cs +++ /dev/null @@ -1,19 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.InteropServices; - -namespace System.Runtime.CompilerServices -{ - /// - /// Managed structure used by GCHeapHash in CLR to provide a hashtable manipulated - /// by C++ runtime code which manages its memory in the GC heap. - /// - [StructLayout(LayoutKind.Sequential)] - internal sealed class GCHeapHash - { - private Array? _data; - private int _count; - private int _deletedCount; - } -} diff --git a/src/coreclr/inc/CrstTypes.def b/src/coreclr/inc/CrstTypes.def index 279118f3012ae..d4f8118fc8e42 100644 --- a/src/coreclr/inc/CrstTypes.def +++ b/src/coreclr/inc/CrstTypes.def @@ -544,7 +544,7 @@ Crst InlineTrackingMap End Crst JitInlineTrackingMap - AcquiredBefore CodeVersioning ThreadStore LoaderAllocator + AcquiredBefore CodeVersioning ThreadStore MethodDescBackpatchInfoTracker End Crst EventPipe @@ -568,8 +568,8 @@ Crst COMCallWrapper End Crst MethodDescBackpatchInfoTracker - AcquiredBefore FuncPtrStubs ThreadStore SystemDomain - AcquiredAfter ReJITGlobalRequest + AcquiredBefore FuncPtrStubs + AcquiredAfter ReJITGlobalRequest ThreadStore SystemDomain End Crst NativeImageEagerFixups diff --git a/src/coreclr/inc/crsttypes.h b/src/coreclr/inc/crsttypes.h index 815b263646617..93ea7f8847d59 100644 --- a/src/coreclr/inc/crsttypes.h +++ b/src/coreclr/inc/crsttypes.h @@ -200,7 +200,7 @@ int g_rgCrstLevelMap[] = 7, // CrstISymUnmanagedReader 11, // CrstJit 0, // CrstJitGenericHandleCache - 16, // CrstJitInlineTrackingMap + 13, // CrstJitInlineTrackingMap 4, // CrstJitPatchpoint -1, // CrstJitPerf 6, // CrstJumpStubCache @@ -210,7 +210,7 @@ int g_rgCrstLevelMap[] = 16, // CrstLoaderAllocatorReferences 3, // CrstLoaderHeap 3, // CrstManagedObjectWrapperMap - 14, // CrstMethodDescBackpatchInfoTracker + 10, // CrstMethodDescBackpatchInfoTracker 5, // CrstModule 15, // CrstModuleFixup 4, // CrstModuleLookupTable @@ -231,7 +231,7 @@ int g_rgCrstLevelMap[] = 0, // CrstRCWCleanupList 10, // CrstReadyToRunEntryPointToMethodDescMap 8, // CrstReflection - 17, // CrstReJITGlobalRequest + 14, // CrstReJITGlobalRequest 4, // CrstRetThunkCache 3, // CrstSavedExceptionInfo 0, // CrstSaveModuleProfileData diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index f2ef79e5c1738..4075d5d9bd28f 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -71,9 +71,15 @@ // default traits if it can be assigned from 0. // static const bool IsDeleted(const ELEMENT &e) Compare element with Deleted sentinal value. May be inherited from the // default traits if it can be assigned from -1. +// static bool ShouldDelete(const ELEMENT &e) Called in addition to IsDeleted() when s_supports_autoremove is true, see more +// information there. // // static void OnDestructPerEntryCleanupAction(ELEMENT& e) Called on every element when in hashtable destructor. // s_DestructPerEntryCleanupAction must be set to true if implemented. +// static void OnRemovePerEntryCleanupAction(ELEMENT& e) Called when an element is removed from the hashtable. +// s_RemovePerEntryCleanupAction must be set to true if implemented. When elements +// are removed in the destructor, OnDestructPerEntryCleanupAction() is called +// instead if s_DestructPerEntryCleanupAction is true. // // s_growth_factor_numerator // s_growth_factor_denominator Factor to grow allocation (numerator/denominator). @@ -92,7 +98,17 @@ // in that there may be more copies of the template instantiated through // the system as different variants are used. // +// s_supports_autoremove When autoremove is supported, ShouldDelete() is called in addition to +// IsDeleted() in any situation that involves walking the table's elements, to +// determine if an element should be deleted. It enables the hash table to +// self-clean elements whose underlying lifetime may be controlled externally. Note +// that since some lookup/iteration operations are const (can operate on a +// "const SHash"), when autoremove is supported, any such const operation may still +// modify the hash table. If this is set to true, s_supports_remove must also be +// true. +// // s_DestructPerEntryCleanupAction Set to true if OnDestructPerEntryCleanupAction has non-empty implementation. +// s_RemovePerEntryCleanupAction Set to true if OnRemovePerEntryCleanupAction has non-empty implementation. // // DefaultHashTraits provides defaults for seldomly customized values in traits classes. @@ -115,15 +131,20 @@ class DefaultSHashTraits static const COUNT_T s_minimum_allocation = 7; static const bool s_supports_remove = true; + static const bool s_supports_autoremove = false; static ELEMENT Null() { return (ELEMENT)(TADDR)0; } static ELEMENT Deleted() { return (ELEMENT)(TADDR)-1; } static bool IsNull(const ELEMENT &e) { return e == (ELEMENT)(TADDR)0; } static bool IsDeleted(const ELEMENT &e) { return e == (ELEMENT)(TADDR)-1; } + static bool ShouldDelete(const ELEMENT &e) { return false; } static inline void OnDestructPerEntryCleanupAction(const ELEMENT& e) { /* Do nothing */ } static const bool s_DestructPerEntryCleanupAction = false; + static void OnRemovePerEntryCleanupAction(const ELEMENT &e) {} + static const bool s_RemovePerEntryCleanupAction = false; + static const bool s_NoThrow = true; // No defaults - must specify: @@ -177,6 +198,10 @@ class EMPTY_BASES_DECL SHash : public TRAITS const element_t* LookupPtr(key_t key) const; + // Pointer-based flavor to replace an existing element (allows efficient access to tables of structures) + + void ReplacePtr(const element_t *elementPtr, const element_t &newElement); + // Add an element to the hash table. This will never replace an element; multiple // elements may be stored with the same key. @@ -298,7 +323,7 @@ class EMPTY_BASES_DECL SHash : public TRAITS // it is perfectly fine for the element to be a duplicate - if so it // is added an additional time. Returns TRUE if a new empty spot was used; // FALSE if an existing deleted slot. - static BOOL Add(element_t *table, count_t tableSize, const element_t &element); + BOOL Add(element_t *table, count_t tableSize, const element_t &element) const; // Utility function to add a new element to the hash table, if no element with the same key // is already there. Otherwise, it will replace the existing element. This has the effect of @@ -308,7 +333,7 @@ class EMPTY_BASES_DECL SHash : public TRAITS // Utility function to find the first element with the given key in // the hash table. - static const element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key); + const element_t* Lookup(PTR_element_t table, count_t tableSize, key_t key) const; // Utility function to remove the first element with the given key // in the hash table. @@ -334,13 +359,17 @@ class EMPTY_BASES_DECL SHash : public TRAITS // Some compilers won't compile the separate implementation in shash.inl protected: + SHash *m_hash; PTR_element_t m_table; count_t m_tableSize; count_t m_index; - + // Iteration may modify the hash table if s_supports_autoremove is true. Since it typically does not modify the hash + // table, it should be possible to iterate over a "const SHash". So this takes a "const SHash *" and casts away the + // const to support autoremove. Index(const SHash *hash, BOOL begin) - : m_table(hash->m_table), + : m_hash(const_cast(hash)), + m_table(hash->m_table), m_tableSize(hash->m_tableSize), m_index(begin ? 0 : m_tableSize) { @@ -358,9 +387,24 @@ class EMPTY_BASES_DECL SHash : public TRAITS { LIMITED_METHOD_CONTRACT; - if (m_index < m_tableSize) - if (TRAITS::IsNull(m_table[m_index]) || TRAITS::IsDeleted(m_table[m_index])) - Next(); + if (m_index >= m_tableSize) + { + return; + } + + if (!TRAITS::IsNull(m_table[m_index]) && !TRAITS::IsDeleted(m_table[m_index])) + { + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(m_table[m_index])) + { + m_hash->RemoveElement(m_table, m_tableSize, &m_table[m_index]); + } + else + { + return; + } + } + + Next(); } void Next() @@ -376,7 +420,16 @@ class EMPTY_BASES_DECL SHash : public TRAITS if (m_index >= m_tableSize) break; if (!TRAITS::IsNull(m_table[m_index]) && !TRAITS::IsDeleted(m_table[m_index])) - break; + { + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(m_table[m_index])) + { + m_hash->RemoveElement(m_table, m_tableSize, &m_table[m_index]); + } + else + { + break; + } + } } } @@ -442,11 +495,26 @@ class EMPTY_BASES_DECL SHash : public TRAITS m_increment = (hash % (this->m_tableSize-1)) + 1; // Find first valid element + if (TRAITS::IsNull(this->m_table[this->m_index])) + { this->m_index = this->m_tableSize; - else if (TRAITS::IsDeleted(this->m_table[this->m_index]) - || !TRAITS::Equals(m_key, TRAITS::GetKey(this->m_table[this->m_index]))) - Next(); + return; + } + + if (!TRAITS::IsDeleted(this->m_table[this->m_index])) + { + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(m_table[m_index])) + { + m_hash->RemoveElement(m_table, m_tableSize, &m_table[m_index]); + } + else if (TRAITS::Equals(m_key, TRAITS::GetKey(this->m_table[this->m_index]))) + { + return; + } + } + + Next(); } } @@ -466,8 +534,18 @@ class EMPTY_BASES_DECL SHash : public TRAITS break; } - if (!TRAITS::IsDeleted(this->m_table[this->m_index]) - && TRAITS::Equals(m_key, TRAITS::GetKey(this->m_table[this->m_index]))) + if (TRAITS::IsDeleted(this->m_table[this->m_index])) + { + continue; + } + + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(m_table[m_index])) + { + m_hash->RemoveElement(m_table, m_tableSize, &m_table[m_index]); + continue; + } + + if (TRAITS::Equals(m_key, TRAITS::GetKey(this->m_table[this->m_index]))) { break; } diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index d5c2bd41d781e..aa09d14d4a108 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -24,6 +24,8 @@ SHash::SHash() static_assert_no_msg(SHash::s_growth_factor_numerator > SHash::s_growth_factor_denominator); static_assert_no_msg(SHash::s_density_factor_numerator < SHash::s_density_factor_denominator); #endif + + static_assert_no_msg(TRAITS::s_supports_remove || !TRAITS::s_supports_autoremove); } template @@ -38,6 +40,13 @@ SHash::~SHash() TRAITS::OnDestructPerEntryCleanupAction(*i); } } + else if (TRAITS::s_RemovePerEntryCleanupAction) + { + for (Iterator i = Begin(); i != End(); i++) + { + TRAITS::OnRemovePerEntryCleanupAction(*i); + } + } delete [] m_table; } @@ -90,6 +99,34 @@ const typename SHash::element_t * SHash::LookupPtr(key_t key) co RETURN Lookup(m_table, m_tableSize, key); } +template +void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement) +{ + CONTRACT_VOID + { + NOTHROW; + GC_NOTRIGGER; + INSTANCE_CHECK; + PRECONDITION(m_table <= elementPtr); + PRECONDITION(elementPtr < m_table + m_tableSize); + PRECONDITION(!TRAITS::IsNull(*elementPtr)); + PRECONDITION(!TRAITS::IsDeleted(*elementPtr)); + PRECONDITION(elementPtr == LookupPtr(TRAITS::GetKey(*elementPtr))); + PRECONDITION(!TRAITS::IsNull(newElement)); + PRECONDITION(!TRAITS::IsDeleted(newElement)); + PRECONDITION(TRAITS::Equals(TRAITS::GetKey(newElement), TRAITS::GetKey(*elementPtr))); + } + CONTRACT_END; + + if (TRAITS::s_RemovePerEntryCleanupAction) + { + TRAITS::OnRemovePerEntryCleanupAction(*elementPtr); + } + + *const_cast(elementPtr) = newElement; + RETURN; +} + template void SHash::Add(const element_t & element) { @@ -277,11 +314,11 @@ void SHash::RemoveAll() } CONTRACT_END; - if (TRAITS::s_DestructPerEntryCleanupAction) + if (TRAITS::s_RemovePerEntryCleanupAction) { for (Iterator i = Begin(); i != End(); i++) { - TRAITS::OnDestructPerEntryCleanupAction(*i); + TRAITS::OnRemovePerEntryCleanupAction(*i); } } @@ -495,10 +532,18 @@ void SHash::ForEach(Functor &functor) for (count_t i = 0; i < m_tableSize; i++) { element_t element = m_table[i]; - if (!TRAITS::IsNull(element) && !TRAITS::IsDeleted(element)) + if (TRAITS::IsNull(element) || TRAITS::IsDeleted(element)) + { + continue; + } + + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(element)) { - functor(element); + RemoveElement(m_table, m_tableSize, &m_table[i]); + continue; } + + functor(element); } } @@ -587,8 +632,9 @@ SHash::ReplaceTable(element_t * newTable, count_t newTableSize) for (Iterator i = Begin(), end = End(); i != end; i++) { const element_t & cur = (*i); - if (!TRAITS::IsNull(cur) && !TRAITS::IsDeleted(cur)) - Add(newTable, newTableSize, cur); + _ASSERTE(!TRAITS::IsNull(cur)); + _ASSERTE(!TRAITS::IsDeleted(cur)); + Add(newTable, newTableSize, cur); } m_table = PTR_element_t(newTable); @@ -619,7 +665,7 @@ SHash::DeleteOldTable(element_t * oldTable) } template -const typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) +const typename SHash::element_t * SHash::Lookup(PTR_element_t table, count_t tableSize, key_t key) const { CONTRACT(const element_t *) { @@ -644,10 +690,16 @@ const typename SHash::element_t * SHash::Lookup(PTR_element_t ta if (TRAITS::IsNull(current)) RETURN NULL; - if (!TRAITS::IsDeleted(current) - && TRAITS::Equals(key, TRAITS::GetKey(current))) + if (!TRAITS::IsDeleted(current)) { - RETURN ¤t; + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) + { + const_cast *>(this)->RemoveElement(table, tableSize, ¤t); + } + else if (TRAITS::Equals(key, TRAITS::GetKey(current))) + { + RETURN ¤t; + } } if (increment == 0) @@ -660,7 +712,7 @@ const typename SHash::element_t * SHash::Lookup(PTR_element_t ta } template -BOOL SHash::Add(element_t * table, count_t tableSize, const element_t & element) +BOOL SHash::Add(element_t * table, count_t tableSize, const element_t & element) const { CONTRACT(BOOL) { @@ -692,6 +744,13 @@ BOOL SHash::Add(element_t * table, count_t tableSize, const element_t & RETURN FALSE; } + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) + { + const_cast *>(this)->RemoveElement(table, tableSize, ¤t); + table[index] = element; + RETURN FALSE; + } + if (increment == 0) increment = (hash % (tableSize-1)) + 1; @@ -733,6 +792,11 @@ void SHash::AddOrReplace(element_t *table, count_t tableSize, const elem } else if (TRAITS::Equals(key, TRAITS::GetKey(current))) { + if (TRAITS::s_RemovePerEntryCleanupAction) + { + TRAITS::OnRemovePerEntryCleanupAction(current); + } + table[index] = element; RETURN; } @@ -769,12 +833,23 @@ void SHash::Remove(element_t *table, count_t tableSize, key_t key) if (TRAITS::IsNull(current)) RETURN; - if (!TRAITS::IsDeleted(current) - && TRAITS::Equals(key, TRAITS::GetKey(current))) + if (!TRAITS::IsDeleted(current)) { - table[index] = TRAITS::Deleted(); - m_tableCount--; - RETURN; + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) + { + RemoveElement(table, tableSize, ¤t); + } + else if (TRAITS::Equals(key, TRAITS::GetKey(current))) + { + if (TRAITS::s_RemovePerEntryCleanupAction) + { + TRAITS::OnRemovePerEntryCleanupAction(current); + } + + table[index] = TRAITS::Deleted(); + m_tableCount--; + RETURN; + } } if (increment == 0) @@ -793,12 +868,17 @@ void SHash::RemoveElement(element_t *table, count_t tableSize, element_t { NOTHROW; GC_NOTRIGGER; - static_assert(TRAITS::s_supports_remove, "This SHash does not support remove operations."); + PRECONDITION(TRAITS::s_supports_remove); PRECONDITION(table <= element && element < table + tableSize); PRECONDITION(!TRAITS::IsNull(*element) && !TRAITS::IsDeleted(*element)); } CONTRACT_END; + if (TRAITS::s_RemovePerEntryCleanupAction) + { + TRAITS::OnRemovePerEntryCleanupAction(*element); + } + *element = TRAITS::Deleted(); m_tableCount--; RETURN; diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index 55076560a46a4..1e1563ee04291 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -1394,8 +1394,6 @@ void SystemDomain::LoadBaseSystemClasses() g_pDelegateClass = CoreLibBinder::GetClass(CLASS__DELEGATE); g_pMulticastDelegateClass = CoreLibBinder::GetClass(CLASS__MULTICAST_DELEGATE); - CrossLoaderAllocatorHashSetup::EnsureTypesLoaded(); - // further loading of nonprimitive types may need casting support. // initialize cast cache here. CastCache::Initialize(); diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index fbe728ff7e1c0..37d7dc5cc60c8 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -1177,7 +1177,8 @@ class BaseDomain // To be used when the thread may enter cooperative GC mode while holding the lock. The thread enters a // forbid-suspend-for-debugger region along with acquiring the lock, such that it would not suspend for the debugger while - // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock. + // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock. This holder cannot + // be used when there may be GC allocation inside its scope (see the base class for more information). class DomainCacheCrstHolderForGCCoop : private CrstAndForbidSuspendForDebuggerHolder { public: diff --git a/src/coreclr/vm/callcounting.cpp b/src/coreclr/vm/callcounting.cpp index 2ff450334494b..344f68b17b5ca 100644 --- a/src/coreclr/vm/callcounting.cpp +++ b/src/coreclr/vm/callcounting.cpp @@ -553,20 +553,8 @@ bool CallCountingManager::SetCodeEntryPoint( { THROWS; GC_NOTRIGGER; - - // Backpatching entry point slots requires cooperative GC mode, see MethodDescBackpatchInfoTracker::Backpatch_Locked(). - // The code version manager's table lock is an unsafe lock that may be taken in any GC mode. The lock is taken in - // cooperative GC mode on other paths, so the caller must use the same ordering to prevent deadlock (switch to - // cooperative GC mode before taking the lock). PRECONDITION(!activeCodeVersion.IsNull()); - if (activeCodeVersion.GetMethodDesc()->MayHaveEntryPointSlotsToBackpatch()) - { - MODE_COOPERATIVE; - } - else - { - MODE_ANY; - } + MODE_ANY; } CONTRACTL_END; @@ -874,13 +862,7 @@ void CallCountingManager::CompleteCallCounting() TieredCompilationManager *tieredCompilationManager = appDomain->GetTieredCompilationManager(); CodeVersionManager *codeVersionManager = appDomain->GetCodeVersionManager(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder; - - // Backpatching entry point slots requires cooperative GC mode, see - // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that - // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering - // must be used here to prevent deadlock. - GCX_COOP(); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; CodeVersionManager::LockHolder codeVersioningLockHolder; for (auto itEnd = s_callCountingManagers->End(), it = s_callCountingManagers->Begin(); it != itEnd; ++it) @@ -1002,8 +984,6 @@ void CallCountingManager::StopAndDeleteAllCallCountingStubs() TieredCompilationManager *tieredCompilationManager = GetAppDomain()->GetTieredCompilationManager(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder; - ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_OTHER); struct AutoRestartEE { @@ -1014,11 +994,7 @@ void CallCountingManager::StopAndDeleteAllCallCountingStubs() } } autoRestartEE; - // Backpatching entry point slots requires cooperative GC mode, see - // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that - // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering - // must be used here to prevent deadlock. - GCX_COOP(); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; CodeVersionManager::LockHolder codeVersioningLockHolder; // After the following, no method's entry point would be pointing to a call counting stub diff --git a/src/coreclr/vm/ceemain.cpp b/src/coreclr/vm/ceemain.cpp index 8404413beb345..3800fc4df0a1c 100644 --- a/src/coreclr/vm/ceemain.cpp +++ b/src/coreclr/vm/ceemain.cpp @@ -666,6 +666,7 @@ void EEStartupHelper() Thread::StaticInitialize(); ThreadpoolMgr::StaticInitialize(); + JITInlineTrackingMap::StaticInitialize(); MethodDescBackpatchInfoTracker::StaticInitialize(); CodeVersionManager::StaticInitialize(); TieredCompilationManager::StaticInitialize(); diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index d59e13d45d45d..e0cc64a0f42c6 100644 --- a/src/coreclr/vm/codeversion.cpp +++ b/src/coreclr/vm/codeversion.cpp @@ -1496,6 +1496,8 @@ HRESULT CodeVersionManager::SetActiveILCodeVersions(ILCodeVersion* pActiveVersio } #endif + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; + // Assume that a new IL code version will be added for a method def, and that an instantiation may not yet have native code // for the default native code version CodeVersionManager::SetInitialNativeCodeVersionMayNotBeTheDefaultNativeCodeVersion(); @@ -1544,11 +1546,6 @@ HRESULT CodeVersionManager::SetActiveILCodeVersions(ILCodeVersion* pActiveVersio // step 3 - update each pre-existing method instantiation { - // Backpatching entry point slots requires cooperative GC mode, see - // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that - // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering - // must be used here to prevent deadlock. - GCX_COOP(); LockHolder codeVersioningLockHolder; for (DWORD i = 0; i < cActiveVersions; i++) @@ -1758,10 +1755,6 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary( NativeCodeVersion newActiveVersion; do { - bool mayHaveEntryPointSlotsToBackpatch = doPublish && pMethodDesc->MayHaveEntryPointSlotsToBackpatch(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder( - mayHaveEntryPointSlotsToBackpatch); - // Try a faster check to see if we can avoid checking the currently active code version // - For the default code version, if a profiler is attached it may be notified of JIT events and may request rejit // synchronously on the same thread. In that case, for back-compat as described below, the returned code must be for @@ -1774,6 +1767,9 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary( { if (doPublish) { + bool mayHaveEntryPointSlotsToBackpatch = pMethodDesc->MayHaveEntryPointSlotsToBackpatch(); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder( + mayHaveEntryPointSlotsToBackpatch); pMethodDesc->TrySetInitialCodeEntryPointForVersionableMethod(pCode, mayHaveEntryPointSlotsToBackpatch); } else @@ -1785,11 +1781,8 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary( break; } - // Backpatching entry point slots requires cooperative GC mode, see - // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that - // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering - // must be used here to prevent deadlock. - GCX_MAYBE_COOP(mayHaveEntryPointSlotsToBackpatch); + bool mayHaveEntryPointSlotsToBackpatch = doPublish && pMethodDesc->MayHaveEntryPointSlotsToBackpatch(); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder(mayHaveEntryPointSlotsToBackpatch); LockHolder codeVersioningLockHolder; hr = GetActiveILCodeVersion(pMethodDesc).GetOrCreateActiveNativeCodeVersion(pMethodDesc, &newActiveVersion); @@ -1880,21 +1873,9 @@ HRESULT CodeVersionManager::PublishNativeCodeVersion(MethodDesc* pMethod, Native { CONTRACTL { - GC_NOTRIGGER; - - // Backpatching entry point slots requires cooperative GC mode, see MethodDescBackpatchInfoTracker::Backpatch_Locked(). - // The code version manager's table lock is an unsafe lock that may be taken in any GC mode. The lock is taken in - // cooperative GC mode on other paths, so the caller must use the same ordering to prevent deadlock (switch to - // cooperative GC mode before taking the lock). - if (pMethod->MayHaveEntryPointSlotsToBackpatch()) - { - MODE_COOPERATIVE; - } - else - { - MODE_ANY; - } NOTHROW; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index ee95e5fd8a6d4..33f9d7e30d4d8 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -1236,13 +1236,6 @@ DEFINE_CLASS(NULLABLE_COMPARER, CollectionsGeneric, NullableComparer`1) DEFINE_CLASS(INATTRIBUTE, Interop, InAttribute) -DEFINE_CLASS_U(CompilerServices, GCHeapHash, GCHeapHashObject) -DEFINE_FIELD_U(_data, GCHeapHashObject, _data) -DEFINE_FIELD_U(_count, GCHeapHashObject, _count) -DEFINE_FIELD_U(_deletedCount, GCHeapHashObject, _deletedCount) - -DEFINE_CLASS(GCHEAPHASH, CompilerServices, GCHeapHash) - DEFINE_CLASS(CASTHELPERS, CompilerServices, CastHelpers) DEFINE_FIELD(CASTHELPERS, TABLE, s_table) DEFINE_METHOD(CASTHELPERS, ISINSTANCEOFANY, IsInstanceOfAny, SM_PtrVoid_Obj_RetObj) @@ -1256,21 +1249,6 @@ DEFINE_METHOD(CASTHELPERS, UNBOX, Unbox, SM_Ptr DEFINE_METHOD(CASTHELPERS, STELEMREF, StelemRef, SM_Array_Int_Obj_RetVoid) DEFINE_METHOD(CASTHELPERS, LDELEMAREF, LdelemaRef, SM_Array_Int_PtrVoid_RetRefObj) -DEFINE_CLASS_U(CompilerServices, LAHashDependentHashTracker, LAHashDependentHashTrackerObject) -DEFINE_FIELD_U(_dependentHandle, LAHashDependentHashTrackerObject,_dependentHandle) -DEFINE_FIELD_U(_loaderAllocator, LAHashDependentHashTrackerObject,_loaderAllocator) - -DEFINE_CLASS(LAHASHDEPENDENTHASHTRACKER, CompilerServices, LAHashDependentHashTracker) -#if FOR_ILLINK -DEFINE_METHOD(LAHASHDEPENDENTHASHTRACKER, CTOR, .ctor, IM_RetVoid) -#endif - -DEFINE_CLASS_U(CompilerServices, LAHashKeyToTrackers, LAHashKeyToTrackersObject) -DEFINE_FIELD_U(_trackerOrTrackerSet, LAHashKeyToTrackersObject, _trackerOrTrackerSet) -DEFINE_FIELD_U(_laLocalKeyValueStore, LAHashKeyToTrackersObject, _laLocalKeyValueStore) - -DEFINE_CLASS(LAHASHKEYTOTRACKERS, CompilerServices, LAHashKeyToTrackers) - DEFINE_CLASS_U(System, GCMemoryInfoData, GCMemoryInfoData) DEFINE_FIELD_U(_highMemoryLoadThresholdBytes, GCMemoryInfoData, highMemLoadThresholdBytes) DEFINE_FIELD_U(_totalAvailableMemoryBytes, GCMemoryInfoData, totalAvailableMemoryBytes) diff --git a/src/coreclr/vm/crossloaderallocatorhash.h b/src/coreclr/vm/crossloaderallocatorhash.h index 2e01343dc1263..9f16cc07f21c6 100644 --- a/src/coreclr/vm/crossloaderallocatorhash.h +++ b/src/coreclr/vm/crossloaderallocatorhash.h @@ -4,7 +4,9 @@ #ifndef CROSSLOADERALLOCATORHASH_H #define CROSSLOADERALLOCATORHASH_H -#include "gcheaphashtable.h" +#define DISABLE_COPY(T) \ + T(const T &) = delete; \ + T &operator =(const T &) = delete class LoaderAllocator; @@ -14,53 +16,66 @@ class NoRemoveDefaultCrossLoaderAllocatorHashTraits public: typedef TKey_ TKey; typedef TValue_ TValue; + typedef COUNT_T TCount; - static bool IsNull(const TValue &value) { return value == NULL; } - static TValue NullValue() { return NULL; } + static const bool s_supports_remove = false; -#ifndef DACCESS_COMPILE - static void SetUsedEntries(TValue* pStartOfValuesData, DWORD entriesInArrayTotal, DWORD usedEntries); - static bool AddToValuesInHeapMemory(OBJECTREF *pKeyValueStore, const TKey& key, const TValue& value); -#endif //!DACCESS_COMPILE - static DWORD ComputeUsedEntries(OBJECTREF *pKeyValueStore, DWORD *pEntriesInArrayTotal); - template - static bool VisitKeyValueStore(OBJECTREF *pLoaderAllocatorRef, OBJECTREF *pKeyValueStore, Visitor &visitor); - static TKey ReadKeyFromKeyValueStore(OBJECTREF *pKeyValueStore); + // CrossLoaderAllocatorHash requires that a particular null value exist, which represents an empty value slot + static bool IsNullValue(const TValue &value) { return value == NULL; } + static TValue NullValue() { return NULL; } + + static BOOL KeyEquals(const TKey &k1, const TKey &k2) { return k1 == k2; } + static BOOL ValueEquals(const TValue &v1, const TValue &v2) { return v1 == v2; } + static TCount Hash(const TKey &k) { return (TCount)(size_t)k; } + + static LoaderAllocator *GetLoaderAllocator(const TKey &k) { return k->GetLoaderAllocator(); } }; template class DefaultCrossLoaderAllocatorHashTraits : public NoRemoveDefaultCrossLoaderAllocatorHashTraits { public: - typedef TKey_ TKey; - typedef TValue_ TValue; - -#ifndef DACCESS_COMPILE - static void DeleteValueInHeapMemory(OBJECTREF keyValueStore, const TValue& value); -#endif //!DACCESS_COMPILE + static const bool s_supports_remove = true; }; -struct GCHeapHashDependentHashTrackerHashTraits : public DefaultGCHeapHashTraits +// Base class for a native object that depends on a LoaderAllocator's lifetime +class LADependentNativeObject { - typedef LoaderAllocator* PtrTypeKey; +protected: + LADependentNativeObject() = default; - static INT32 Hash(PtrTypeKey *pValue); - static INT32 Hash(PTRARRAYREF arr, INT32 index); - static bool DoesEntryMatchKey(PTRARRAYREF arr, INT32 index, PtrTypeKey *pKey); - static bool IsDeleted(PTRARRAYREF arr, INT32 index, GCHEAPHASHOBJECTREF gcHeap); -}; +public: + virtual ~LADependentNativeObject() = default; -typedef GCHeapHash GCHeapHashDependentHashTrackerHash; + DISABLE_COPY(LADependentNativeObject); +}; -template -struct KeyToValuesGCHeapHashTraits : public DefaultGCHeapHashTraits +// A handle to an LADependentNativeObject that is registered with the LoaderAllocator. The handle would be cleared when the +// LoaderAllocator is collected. Appropriate locking must be used with this class and in the LoaderAllocator's code that clears +// handles. +class LADependentHandleToNativeObject { - template - static INT32 Hash(TKey *pValue); - static INT32 Hash(PTRARRAYREF arr, INT32 index); +private: + LADependentNativeObject *m_dependentObject; + +public: + LADependentHandleToNativeObject(LADependentNativeObject *dependentObject) : m_dependentObject(dependentObject) {} + ~LADependentHandleToNativeObject() { delete m_dependentObject; } - template - static bool DoesEntryMatchKey(PTRARRAYREF arr, INT32 index, TKey *pKey); + // See notes about synchronization in Clear() + LADependentNativeObject *GetDependentObject() const { return m_dependentObject; } + + void Clear() + { + _ASSERTE(m_dependentObject != nullptr); + + // The callers of GetDependentObject() and Clear() must ensure that they cannot run concurrently, and that a + // GetDependentObject() following a Clear() sees a null dependent object in a thread-safe manner + delete m_dependentObject; + m_dependentObject = nullptr; + } + + DISABLE_COPY(LADependentHandleToNativeObject); }; // Hashtable of key to a list of values where the key may live in a different loader allocator @@ -86,7 +101,16 @@ struct KeyToValuesGCHeapHashTraits : public DefaultGCHeapHashTraits // // VisitValuesOfKey will visit all values that have the same key. // +// IMPORTANT NOTE ABOUT SYNCHRONIZATION +// +// Any lock used to synchronize access to an instance of this data structure must also be +// acquired in AssemblyLoaderAllocator::CleanupDependentHandlesToNativeObjects() to ensure that +// while visiting values, the LoaderAllocator of the value remains alive inside the lock. +// Visited values must not be used outside the lock, as the LoaderAllocator of the value may +// may not be alive outside the lock. +// // IMPLEMENTATION DESIGN +// // This data structure is a series of hashtables and lists. // // In general, this data structure builds a set of values associated with a key per @@ -127,10 +151,246 @@ struct KeyToValuesGCHeapHashTraits : public DefaultGCHeapHashTraits template class CrossLoaderAllocatorHash { + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // Types used by CrossLoaderAllocatorHash + private: typedef typename TRAITS::TKey TKey; typedef typename TRAITS::TValue TValue; - typedef GCHeapHash> KeyToValuesGCHeapHash; + typedef typename TRAITS::TCount TCount; + + class KeyValueStoreOrLAHashKeyToTrackers + { + protected: + KeyValueStoreOrLAHashKeyToTrackers() = default; + + public: + virtual ~KeyValueStoreOrLAHashKeyToTrackers() = default; + + virtual bool IsLAHashKeyToTrackers() const { return false; } + }; + + class LAHashDependentHashTrackerOrTrackerSet + { + private: + const bool _isTrackerSet; + + protected: + LAHashDependentHashTrackerOrTrackerSet(bool isTrackerSet) : _isTrackerSet(isTrackerSet) {} + + public: + bool IsTrackerSet() const { return _isTrackerSet; } + }; + + class KeyValueStore : public KeyValueStoreOrLAHashKeyToTrackers + { + private: + const TCount _capacity; + const TKey _key; + TValue _values[0]; + + private: + KeyValueStore(TCount capacity, const TKey &key) : _capacity(capacity), _key(key) {} + + public: + static KeyValueStore *Create(TCount capacity, const TKey &key); + + TCount GetCapacity() const { return _capacity; } + TKey GetKey() const { return _key; } + TValue *GetValues() { return _values; } + }; + + class LAHashKeyToTrackers : public KeyValueStoreOrLAHashKeyToTrackers + { + public: + LAHashDependentHashTrackerOrTrackerSet *_trackerOrTrackerSet; + + // _laLocalKeyValueStore holds an object that represents a Key value (which must always be valid for the lifetime of the + // CrossLoaderAllocatorHeapHash, and the values which must also be valid for that entire lifetime. When a value might + // have a shorter lifetime it is accessed through the _trackerOrTrackerSet variable, which allows access to hashtables which + // are associated with that remote loaderallocator through a dependent handle, so that lifetime can be managed. + KeyValueStore *_laLocalKeyValueStore; + + LAHashKeyToTrackers(KeyValueStore *laLocalKeyValueStore) + : _trackerOrTrackerSet(NULL), _laLocalKeyValueStore(laLocalKeyValueStore) + {} + + virtual ~LAHashKeyToTrackers() override; + + virtual bool IsLAHashKeyToTrackers() const override { return true; } + }; + + class EMPTY_BASES_DECL KeyToValuesHashTraits : public DefaultSHashTraits + { + private: + typedef DefaultSHashTraits Base; + + public: + typedef TCount count_t; + typedef TKey key_t; + + static const bool s_supports_remove = TRAITS::s_supports_remove; + static const bool s_RemovePerEntryCleanupAction = true; + + static KeyValueStoreOrLAHashKeyToTrackers *Deleted() + { + if (s_supports_remove) + { + return Base::Deleted(); + } + + UNREACHABLE(); + } + + static bool IsDeleted(KeyValueStoreOrLAHashKeyToTrackers *e) { return s_supports_remove && Base::IsDeleted(e); } + static void OnRemovePerEntryCleanupAction(KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry) { delete hashKeyEntry; } + static TKey GetKey(KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry); + static BOOL Equals(const TKey &k1, const TKey &k2) { return TRAITS::KeyEquals(k1, k2); } + static TCount Hash(const TKey &k) { return TRAITS::Hash(k); } + + #ifndef DACCESS_COMPILE + static void SetUsedEntries(KeyValueStore *keyValueStore, TCount entriesInArrayTotal, TCount usedEntries); + static bool AddToValuesInHeapMemory( + KeyValueStore **pKeyValueStore, + NewHolder &keyValueStoreHolder, + const TKey& key, + const TValue& value); + #endif // !DACCESS_COMPILE + static TCount ComputeUsedEntries(KeyValueStore *keyValueStore, TCount *pEntriesInArrayTotal); + template + static bool VisitKeyValueStore(LoaderAllocator *loaderAllocator, KeyValueStore *keyValueStore, Visitor &visitor); + #ifndef DACCESS_COMPILE + static void DeleteValueInHeapMemory(KeyValueStore *keyValueStore, const TValue& value); + #endif // !DACCESS_COMPILE + }; + + typedef SHash KeyToValuesHash; + + class LADependentKeyToValuesHash : public LADependentNativeObject + { + private: + KeyToValuesHash _keyToValuesHash; + + public: + virtual ~LADependentKeyToValuesHash() override = default; + + KeyToValuesHash *GetKeyToValuesHash() { return &_keyToValuesHash; } + }; + + class LAHashDependentHashTracker : public LAHashDependentHashTrackerOrTrackerSet + { + private: + LoaderAllocator *const _loaderAllocator; + LADependentHandleToNativeObject *const _dependentHandle; + UINT64 _refCount; + + public: + LAHashDependentHashTracker(LoaderAllocator *loaderAllocator, LADependentKeyToValuesHash *dependentKeyValueStoreHash) + : LAHashDependentHashTrackerOrTrackerSet(false /* isTrackerSet */), + _loaderAllocator(loaderAllocator), + _dependentHandle(CreateDependentHandle(loaderAllocator, dependentKeyValueStoreHash)), + _refCount(1) + {} + + private: + static LADependentHandleToNativeObject *CreateDependentHandle( + LoaderAllocator *loaderAllocator, + LADependentKeyToValuesHash *dependentKeyValueStoreHash); + ~LAHashDependentHashTracker(); // only accessible via DecRefCount() + + public: + bool IsLoaderAllocatorLive() const { return _dependentHandle->GetDependentObject() != NULL; } + + bool IsTrackerFor(LoaderAllocator *loaderAllocator) const + { + return loaderAllocator == _loaderAllocator && IsLoaderAllocatorLive(); + } + + KeyToValuesHash *GetDependentKeyToValuesHash() const; + + // Be careful with this. This isn't safe to use unless something is keeping the LoaderAllocator live, or there is no + // intention to dereference this pointer. + LoaderAllocator *GetLoaderAllocatorUnsafe() const { return _loaderAllocator; } + + #ifndef DACCESS_COMPILE + void IncRefCount() + { + _ASSERTE(_refCount != 0); + ++_refCount; + } + + void DecRefCount() + { + _ASSERTE(_refCount != 0); + if (--_refCount == 0) + { + delete this; + } + } + + static void StaticDecRefCount(LAHashDependentHashTracker *dependentTracker) + { + if (dependentTracker != NULL) + { + dependentTracker->DecRefCount(); + } + } + + using NewTrackerHolder = SpecializedWrapper; + #endif // !DACCESS_COMPILE + }; + + class EMPTY_BASES_DECL LAHashDependentHashTrackerHashTraits : public DefaultSHashTraits + { + public: + typedef TCount count_t; + typedef LoaderAllocator *key_t; + + static const bool s_supports_autoremove = true; + static const bool s_RemovePerEntryCleanupAction = true; + + static bool ShouldDelete(LAHashDependentHashTracker *dependentTracker) + { + #ifndef DACCESS_COMPILE + // This is a tricky bit of logic used which detects freed loader allocators lazily + // and deletes them from the hash table while looking up or otherwise walking the hashtable + // for any purpose. OnRemovePerEntryCleanupAction() is invoked for removed elements to + // handle cleanup of the actual data. + return !dependentTracker->IsLoaderAllocatorLive(); + #else + return false; + #endif + } + + static void OnRemovePerEntryCleanupAction(LAHashDependentHashTracker *dependentTracker) + { + #ifndef DACCESS_COMPILE + // Dependent trackers may be stored in multiple hash tables and are ref-counted when added/removed from a hash + // table. The ref count decrement may also delete the tracker. + dependentTracker->DecRefCount(); + #endif + } + + static LoaderAllocator *GetKey(LAHashDependentHashTracker *tracker) { return tracker->GetLoaderAllocatorUnsafe(); } + static BOOL Equals(LoaderAllocator *la1, LoaderAllocator *la2) { return la1 == la2; } + static TCount Hash(LoaderAllocator *la) { return (TCount)(size_t)la; } + }; + + typedef SHash LAHashDependentHashTrackerHash; + + class LAHashDependentHashTrackerSetWrapper : public LAHashDependentHashTrackerOrTrackerSet + { + private: + LAHashDependentHashTrackerHash _trackerSet; + + public: + LAHashDependentHashTrackerSetWrapper() : LAHashDependentHashTrackerOrTrackerSet(true /* isTrackerSet */) {} + + LAHashDependentHashTrackerHash *GetTrackerSet() { return &_trackerSet; } + }; + + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // CrossLoaderAllocatorHash members public: @@ -164,32 +424,28 @@ class CrossLoaderAllocatorHash private: #ifndef DACCESS_COMPILE - void EnsureManagedObjectsInitted(); - LAHASHDEPENDENTHASHTRACKERREF GetDependentTrackerForLoaderAllocator(LoaderAllocator* pLoaderAllocator); - GCHEAPHASHOBJECTREF GetKeyToValueCrossLAHashForHashkeyToTrackers(LAHASHKEYTOTRACKERSREF hashKeyToTrackersUnsafe, LoaderAllocator* pValueLoaderAllocator); + LAHashDependentHashTracker *GetDependentTrackerForLoaderAllocator(LoaderAllocator *pLoaderAllocator); + KeyToValuesHash *GetKeyToValueCrossLAHashForHashkeyToTrackers( + LAHashKeyToTrackers *hashKeyToTrackers, + LoaderAllocator *pValueLoaderAllocator); #endif // !DACCESS_COMPILE template - static bool VisitKeyValueStore(OBJECTREF *pLoaderAllocatorRef, OBJECTREF *pKeyValueStore, Visitor &visitor); + static bool VisitKeyValueStore(LoaderAllocator *loaderAllocator, KeyValueStore *keyValueStore, Visitor &visitor); template - static bool VisitTracker(TKey key, LAHASHDEPENDENTHASHTRACKERREF trackerUnsafe, Visitor &visitor); + static bool VisitTracker(TKey key, LAHashDependentHashTracker *tracker, Visitor &visitor); template - static bool VisitTrackerAllEntries(LAHASHDEPENDENTHASHTRACKERREF trackerUnsafe, Visitor &visitor); + static bool VisitTrackerAllEntries(LAHashDependentHashTracker *tracker, Visitor &visitor); template - static bool VisitKeyToTrackerAllEntries(OBJECTREF hashKeyEntryUnsafe, Visitor &visitor); - static void DeleteEntryTracker(TKey key, LAHASHDEPENDENTHASHTRACKERREF trackerUnsafe); + static bool VisitKeyToTrackerAllLALocalEntries(KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry, Visitor &visitor); + static void DeleteEntryTracker(TKey key, LAHashDependentHashTracker *tracker); private: LoaderAllocator *m_pLoaderAllocator = 0; - OBJECTHANDLE m_loaderAllocatorToDependentTrackerHash = 0; - OBJECTHANDLE m_keyToDependentTrackersHash = 0; - OBJECTHANDLE m_globalDependentTrackerRootHandle = 0; + LAHashDependentHashTrackerHash m_loaderAllocatorToDependentTrackerHash; + KeyToValuesHash m_keyToDependentTrackersHash; }; -class CrossLoaderAllocatorHashSetup -{ -public: - inline static void EnsureTypesLoaded(); -}; +#undef DISABLE_COPY #endif // CROSSLOADERALLOCATORHASH_H diff --git a/src/coreclr/vm/crossloaderallocatorhash.inl b/src/coreclr/vm/crossloaderallocatorhash.inl index 2b3d08403f45d..d9527c39bf852 100644 --- a/src/coreclr/vm/crossloaderallocatorhash.inl +++ b/src/coreclr/vm/crossloaderallocatorhash.inl @@ -5,32 +5,36 @@ #define CROSSLOADERALLOCATORHASH_INL #ifdef CROSSLOADERALLOCATORHASH_H -#include "gcheaphashtable.inl" - -template -/*static*/ DWORD NoRemoveDefaultCrossLoaderAllocatorHashTraits::ComputeUsedEntries(OBJECTREF *pKeyValueStore, DWORD *pEntriesInArrayTotal) +template +/*static*/ typename CrossLoaderAllocatorHash::TCount +CrossLoaderAllocatorHash::KeyToValuesHashTraits::ComputeUsedEntries( + KeyValueStore *keyValueStore, + TCount *pEntriesInArrayTotal) { CONTRACTL { NOTHROW; GC_NOTRIGGER; - MODE_COOPERATIVE; + MODE_ANY; } CONTRACTL_END; - DWORD entriesInArrayTotal = (((I1ARRAYREF)*pKeyValueStore)->GetNumComponents() - sizeof(TKey))/sizeof(TValue); - DWORD usedEntries; - TValue* pStartOfValuesData = (TValue*)(((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements() + sizeof(TKey)); + // An empty value slot may be used to store a count + static_assert_no_msg(sizeof(TValue) >= sizeof(TCount)); + + TCount entriesInArrayTotal = keyValueStore->GetCapacity(); + TCount usedEntries; + TValue *pStartOfValuesData = keyValueStore->GetValues(); if (entriesInArrayTotal == 0) { usedEntries = 0; } - else if ((entriesInArrayTotal >= 2) && (pStartOfValuesData[entriesInArrayTotal - 2] == (TValue)0)) + else if ((entriesInArrayTotal >= 2) && TRAITS::IsNullValue(pStartOfValuesData[entriesInArrayTotal - 2])) { - usedEntries = (DWORD)(SIZE_T)pStartOfValuesData[entriesInArrayTotal - 1]; + usedEntries = *(TCount *)&pStartOfValuesData[entriesInArrayTotal - 1]; } - else if (pStartOfValuesData[entriesInArrayTotal - 1] == (TValue)0) + else if (TRAITS::IsNullValue(pStartOfValuesData[entriesInArrayTotal - 1])) { usedEntries = entriesInArrayTotal - 1; } @@ -44,59 +48,132 @@ template } #ifndef DACCESS_COMPILE -template -/*static*/ void NoRemoveDefaultCrossLoaderAllocatorHashTraits::SetUsedEntries(TValue* pStartOfValuesData, DWORD entriesInArrayTotal, DWORD usedEntries) +template +/*static*/ void CrossLoaderAllocatorHash::KeyToValuesHashTraits::SetUsedEntries( + KeyValueStore *keyValueStore, + TCount entriesInArrayTotal, + TCount usedEntries) { + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + // An empty value slot may be used to store a count + static_assert_no_msg(sizeof(TValue) >= sizeof(TCount)); + + _ASSERTE(entriesInArrayTotal == keyValueStore->GetCapacity()); + if (usedEntries < entriesInArrayTotal) { + TValue *pStartOfValuesData = keyValueStore->GetValues(); if (usedEntries == (entriesInArrayTotal - 1)) { - pStartOfValuesData[entriesInArrayTotal - 1] = (TValue)0; + pStartOfValuesData[entriesInArrayTotal - 1] = TRAITS::NullValue(); } else { - pStartOfValuesData[entriesInArrayTotal - 1] = (TValue)((INT_PTR)usedEntries); - pStartOfValuesData[entriesInArrayTotal - 2] = (TValue)0; + *(TCount *)&pStartOfValuesData[entriesInArrayTotal - 1] = usedEntries; + pStartOfValuesData[entriesInArrayTotal - 2] = TRAITS::NullValue(); } } } -template -/*static*/ bool NoRemoveDefaultCrossLoaderAllocatorHashTraits::AddToValuesInHeapMemory(OBJECTREF *pKeyValueStore, const TKey& key, const TValue& value) +template +/*static*/ typename CrossLoaderAllocatorHash::KeyValueStore *CrossLoaderAllocatorHash::KeyValueStore::Create( + TCount capacity, + const TKey &key) { CONTRACTL { THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + NewArrayHolder bufferHolder = new BYTE[sizeof(KeyValueStore) + capacity * sizeof(TValue)]; + KeyValueStore *keyValueStore = new(bufferHolder) KeyValueStore(capacity, key); + bufferHolder.SuppressRelease(); + for (TCount i = 0; i < capacity; i++) + { + keyValueStore->GetValues()[i] = TRAITS::NullValue(); + } + + return keyValueStore; +} + +template +CrossLoaderAllocatorHash::LAHashKeyToTrackers::~LAHashKeyToTrackers() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; - static_assert(sizeof(TKey)==sizeof(TValue), "Assume keys and values are the same size"); + delete _laLocalKeyValueStore; + + LAHashDependentHashTrackerOrTrackerSet *trackerOrTrackerSet = _trackerOrTrackerSet; + if (trackerOrTrackerSet == NULL) + { + return; + } + + if (trackerOrTrackerSet->IsTrackerSet()) + { + delete static_cast(trackerOrTrackerSet); + } + else + { + static_cast(trackerOrTrackerSet)->DecRefCount(); + } +} + +template +/*static*/ bool CrossLoaderAllocatorHash::KeyToValuesHashTraits::AddToValuesInHeapMemory( + KeyValueStore **pKeyValueStore, + NewHolder &keyValueStoreHolder, + const TKey& key, + const TValue& value) +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; bool updatedKeyValueStore = false; + KeyValueStore *keyValueStore = *pKeyValueStore; - if (*pKeyValueStore == NULL) + if (keyValueStore == NULL) { - *pKeyValueStore = AllocatePrimitiveArray(ELEMENT_TYPE_I1, IsNull(value) ? sizeof(TKey) : sizeof(TKey) + sizeof(TValue)); + keyValueStoreHolder = *pKeyValueStore = keyValueStore = + KeyValueStore::Create(TRAITS::IsNullValue(value) ? 0 : 1, key); updatedKeyValueStore = true; - TKey* pKeyLoc = (TKey*)((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements(); - *pKeyLoc = key; - if (!IsNull(value)) + if (!TRAITS::IsNullValue(value)) { - TValue* pValueLoc = (TValue*)(((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements() + sizeof(TKey)); - *pValueLoc = value; + keyValueStore->GetValues()[0] = value; } } - else if (!IsNull(value)) + else if (!TRAITS::IsNullValue(value)) { - DWORD entriesInArrayTotal; - DWORD usedEntries = ComputeUsedEntries(pKeyValueStore, &entriesInArrayTotal); + _ASSERTE(TRAITS::KeyEquals(key, keyValueStore->GetKey())); + + TCount entriesInArrayTotal; + TCount usedEntries = ComputeUsedEntries(keyValueStore, &entriesInArrayTotal); if (usedEntries == entriesInArrayTotal) { // There isn't free space. Build a new, bigger array with the existing data - DWORD newSize; + TCount newSize; if (usedEntries < 8) newSize = usedEntries + 1; // Grow very slowly initially. The cost of allocation/copy is cheap, and this holds very tight on memory usage else @@ -106,56 +183,40 @@ template COMPlusThrow(kOverflowException); // Allocate the new array. - I1ARRAYREF newKeyValueStore = (I1ARRAYREF)AllocatePrimitiveArray(ELEMENT_TYPE_I1, newSize*sizeof(TValue) + sizeof(TKey)); + KeyValueStore *newKeyValueStore = KeyValueStore::Create(newSize, key); + memcpyNoGCRefs(newKeyValueStore->GetValues(), keyValueStore->GetValues(), entriesInArrayTotal * sizeof(TValue)); - // Since, AllocatePrimitiveArray may have triggered a GC, recapture all data pointers from GC objects - void* pStartOfNewArray = newKeyValueStore->GetDirectPointerToNonObjectElements(); - void* pStartOfOldArray = ((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements(); - - memcpyNoGCRefs(pStartOfNewArray, pStartOfOldArray, ((I1ARRAYREF)*pKeyValueStore)->GetNumComponents()); - - *pKeyValueStore = (OBJECTREF)newKeyValueStore; + keyValueStoreHolder = *pKeyValueStore = keyValueStore = newKeyValueStore; updatedKeyValueStore = true; entriesInArrayTotal = newSize; } // There is free space. Append on the end - TValue* pStartOfValuesData = (TValue*)(((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements() + sizeof(TKey)); - SetUsedEntries(pStartOfValuesData, entriesInArrayTotal, usedEntries + 1); - pStartOfValuesData[usedEntries] = value; + SetUsedEntries(keyValueStore, entriesInArrayTotal, usedEntries + 1); + keyValueStore->GetValues()[usedEntries] = value; } return updatedKeyValueStore; } #endif //!DACCESS_COMPILE -template -/*static*/ TKey_ NoRemoveDefaultCrossLoaderAllocatorHashTraits::ReadKeyFromKeyValueStore(OBJECTREF *pKeyValueStore) -{ - WRAPPER_NO_CONTRACT; - - TKey* pKeyLoc = (TKey*)((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements(); - return *pKeyLoc; -} - -template +template template -/*static*/ bool NoRemoveDefaultCrossLoaderAllocatorHashTraits::VisitKeyValueStore(OBJECTREF *pLoaderAllocatorRef, OBJECTREF *pKeyValueStore, Visitor &visitor) +/*static*/ bool CrossLoaderAllocatorHash::KeyToValuesHashTraits::VisitKeyValueStore( + LoaderAllocator *loaderAllocator, + KeyValueStore *keyValueStore, + Visitor &visitor) { WRAPPER_NO_CONTRACT; - DWORD entriesInArrayTotal; - DWORD usedEntries = ComputeUsedEntries(pKeyValueStore, &entriesInArrayTotal); + TKey key = keyValueStore->GetKey(); + TCount entriesInArrayTotal; + TCount usedEntries = ComputeUsedEntries(keyValueStore, &entriesInArrayTotal); - for (DWORD index = 0; index < usedEntries; ++index) + for (TCount index = 0; index < usedEntries; ++index) { - // Capture pKeyLoc and pStartOfValuesData inside of loop, as we aren't protecting these pointers into the GC heap, so they - // are not permitted to live across the call to visitor (in case visitor triggers a GC) - TKey* pKeyLoc = (TKey*)((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements(); - TValue* pStartOfValuesData = (TValue*)(((I1ARRAYREF)*pKeyValueStore)->GetDirectPointerToNonObjectElements() + sizeof(TKey)); - - if (!visitor(*pLoaderAllocatorRef, *pKeyLoc, pStartOfValuesData[index])) + if (!visitor(loaderAllocator, key, keyValueStore->GetValues()[index])) { return false; } @@ -165,14 +226,16 @@ template } #ifndef DACCESS_COMPILE -template -/*static*/ void DefaultCrossLoaderAllocatorHashTraits::DeleteValueInHeapMemory(OBJECTREF keyValueStore, const TValue& value) +template +/*static*/ void CrossLoaderAllocatorHash::KeyToValuesHashTraits::DeleteValueInHeapMemory( + KeyValueStore *keyValueStore, + const TValue& value) { CONTRACTL { NOTHROW; GC_NOTRIGGER; - MODE_COOPERATIVE; + MODE_ANY; } CONTRACTL_END; @@ -180,159 +243,105 @@ template // values list is sorted, and then doing a binary search for the value instead // of the linear search - DWORD entriesInArrayTotal; - DWORD usedEntries = NoRemoveDefaultCrossLoaderAllocatorHashTraits::ComputeUsedEntries(&keyValueStore, &entriesInArrayTotal); - TValue* pStartOfValuesData = (TValue*)(((I1ARRAYREF)keyValueStore)->GetDirectPointerToNonObjectElements() + sizeof(TKey)); + TCount entriesInArrayTotal; + TCount usedEntries = ComputeUsedEntries(keyValueStore, &entriesInArrayTotal); + TValue *pStartOfValuesData = keyValueStore->GetValues(); - for (DWORD iEntry = 0; iEntry < usedEntries; iEntry++) + for (TCount iEntry = 0; iEntry < usedEntries; iEntry++) { - if (pStartOfValuesData[iEntry] == value) + if (TRAITS::ValueEquals(pStartOfValuesData[iEntry], value)) { memmove(pStartOfValuesData + iEntry, pStartOfValuesData + iEntry + 1, (usedEntries - iEntry - 1) * sizeof(TValue)); - SetUsedEntries(pStartOfValuesData, entriesInArrayTotal, usedEntries - 1); + SetUsedEntries(keyValueStore, entriesInArrayTotal, usedEntries - 1); return; } } } -#endif //!DACCESS_COMPILE - -/*static*/ inline INT32 GCHeapHashDependentHashTrackerHashTraits::Hash(PtrTypeKey *pValue) -{ - LIMITED_METHOD_CONTRACT; - return (INT32)(SIZE_T)*pValue; -} - -/*static*/ inline INT32 GCHeapHashDependentHashTrackerHashTraits::Hash(PTRARRAYREF arr, INT32 index) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - LAHASHDEPENDENTHASHTRACKERREF value = (LAHASHDEPENDENTHASHTRACKERREF)arr->GetAt(index); - LoaderAllocator *pLoaderAllocator = value->GetLoaderAllocatorUnsafe(); - return Hash(&pLoaderAllocator); -} -/*static*/ inline bool GCHeapHashDependentHashTrackerHashTraits::DoesEntryMatchKey(PTRARRAYREF arr, INT32 index, PtrTypeKey *pKey) +template +/*static*/ LADependentHandleToNativeObject *CrossLoaderAllocatorHash::LAHashDependentHashTracker::CreateDependentHandle( + LoaderAllocator *loaderAllocator, + LADependentKeyToValuesHash *dependentKeyValueStoreHash) { CONTRACTL { - NOTHROW; + THROWS; GC_NOTRIGGER; - MODE_COOPERATIVE; + MODE_ANY; } CONTRACTL_END; - LAHASHDEPENDENTHASHTRACKERREF value = (LAHASHDEPENDENTHASHTRACKERREF)arr->GetAt(index); - - return value->IsTrackerFor(*pKey); + NewHolder dependentHandleHolder = + new LADependentHandleToNativeObject(dependentKeyValueStoreHash); + loaderAllocator->RegisterDependentHandleToNativeObjectForCleanup(dependentHandleHolder); + return dependentHandleHolder.Extract(); } -/*static*/ inline bool GCHeapHashDependentHashTrackerHashTraits::IsDeleted(PTRARRAYREF arr, INT32 index, GCHEAPHASHOBJECTREF gcHeap) +template +CrossLoaderAllocatorHash::LAHashDependentHashTracker::~LAHashDependentHashTracker() { CONTRACTL { NOTHROW; GC_NOTRIGGER; - MODE_COOPERATIVE; + MODE_ANY; } CONTRACTL_END; - OBJECTREF valueInHeap = arr->GetAt(index); - - if (valueInHeap == NULL) - return false; - - if (gcHeap == valueInHeap) - return true; - - // This is a tricky bit of logic used which detects freed loader allocators lazily - // and deletes them from the GCHeapHash while looking up or otherwise walking the hashtable - // for any purpose. - LAHASHDEPENDENTHASHTRACKERREF value = (LAHASHDEPENDENTHASHTRACKERREF)valueInHeap; - if (!value->IsLoaderAllocatorLive()) + if (IsLoaderAllocatorLive()) { -#ifndef DACCESS_COMPILE - arr->SetAt(index, gcHeap); - gcHeap->DecrementCount(true); -#endif // DACCESS_COMPILE - - return true; + _loaderAllocator->UnregisterDependentHandleToNativeObjectFromCleanup(_dependentHandle); } - return false; -} - -template -template -/*static*/ INT32 KeyToValuesGCHeapHashTraits::Hash(TKey *pValue) -{ - LIMITED_METHOD_CONTRACT; - return (INT32)(SIZE_T)*pValue; + delete _dependentHandle; } +#endif //!DACCESS_COMPILE -template -/*static*/ inline INT32 KeyToValuesGCHeapHashTraits::Hash(PTRARRAYREF arr, INT32 index) +template +typename CrossLoaderAllocatorHash::KeyToValuesHash * +CrossLoaderAllocatorHash::LAHashDependentHashTracker::GetDependentKeyToValuesHash() const { CONTRACTL { NOTHROW; GC_NOTRIGGER; - MODE_COOPERATIVE; + MODE_ANY; } CONTRACTL_END; - OBJECTREF hashKeyEntry = arr->GetAt(index); - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - OBJECTREF keyValueStore; - - if (hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) + LADependentNativeObject *dependentObject = _dependentHandle->GetDependentObject(); + if (dependentObject == NULL) { - hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)hashKeyEntry; - keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; - } - else - { - keyValueStore = hashKeyEntry; + return NULL; } - typename TRAITS::TKey key = TRAITS::ReadKeyFromKeyValueStore(&keyValueStore); - return Hash(&key); + return static_cast(dependentObject)->GetKeyToValuesHash(); } -template -template -/*static*/ bool KeyToValuesGCHeapHashTraits::DoesEntryMatchKey(PTRARRAYREF arr, INT32 index, TKey *pKey) +template +/*static*/ typename CrossLoaderAllocatorHash::TKey CrossLoaderAllocatorHash::KeyToValuesHashTraits::GetKey( + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry) { CONTRACTL { NOTHROW; GC_NOTRIGGER; - MODE_COOPERATIVE; + MODE_ANY; } CONTRACTL_END; - OBJECTREF hashKeyEntry = arr->GetAt(index); - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - OBJECTREF keyValueStore; - - if (hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) + KeyValueStore *keyValueStore; + if (hashKeyEntry->IsLAHashKeyToTrackers()) { - hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)hashKeyEntry; + LAHashKeyToTrackers *hashKeyToTrackers = static_cast(hashKeyEntry); keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; } else { - keyValueStore = hashKeyEntry; + keyValueStore = static_cast(hashKeyEntry); } - TKey key = TRAITS::ReadKeyFromKeyValueStore(&keyValueStore); - - return key == *pKey; + return keyValueStore->GetKey(); } #ifndef DACCESS_COMPILE @@ -342,128 +351,123 @@ void CrossLoaderAllocatorHash::Add(TKey key, TValue value, LoaderAllocat CONTRACTL { THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; + // This data structure actually doesn't have this invariant, but it is expected that uses of this + // data structure will require that the key's loader allocator is the same as that of this data structure. + _ASSERTE(TRAITS::GetLoaderAllocator(key) == m_pLoaderAllocator); - struct { - KeyToValuesGCHeapHash keyToTrackersHash; - KeyToValuesGCHeapHash keyToValuePerLAHash; - OBJECTREF keyValueStore; - OBJECTREF hashKeyEntry; - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - } gc; - ZeroMemory(&gc, sizeof(gc)); - GCPROTECT_BEGIN(gc) - { - EnsureManagedObjectsInitted(); - - bool addToKeyValuesHash = false; - // This data structure actually doesn't have this invariant, but it is expected that uses of this - // data structure will require that the key's loader allocator is the same as that of this data structure. - _ASSERTE(key->GetLoaderAllocator() == m_pLoaderAllocator); + // Add may involve multiple changes to the data structure. The data structure is set up such that each of those changes + // keeps the data structure in a valid state, so OOMs in the middle of an Add operation may end up making some changes but + // the data structure remains in a valid state. Holders are used to clean up memory that was allocated and not yet inserted + // into the data structure. Once inserted into the data structure, it takes over the lifetime of the memory. - gc.keyToTrackersHash = KeyToValuesGCHeapHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_keyToDependentTrackersHash)); - INT32 index = gc.keyToTrackersHash.GetValueIndex(&key); + NewHolder keyValueStoreHolder; + NewHolder hashKeyToTrackersHolder; + KeyValueStore *keyValueStore = NULL; + LAHashKeyToTrackers *hashKeyToTrackers = NULL; - if (index == -1) - { - addToKeyValuesHash = true; - TRAITS::AddToValuesInHeapMemory(&gc.keyValueStore, key, pLoaderAllocatorOfValue == m_pLoaderAllocator ? value : TRAITS::NullValue()); + KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; + KeyValueStoreOrLAHashKeyToTrackers *const *hashKeyEntryPtr = keyToTrackersHash.LookupPtr(key); + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry; - if (pLoaderAllocatorOfValue != m_pLoaderAllocator) - { - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)AllocateObject(CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)); - SetObjectReference(&gc.hashKeyToTrackers->_laLocalKeyValueStore, gc.keyValueStore); - gc.hashKeyEntry = gc.hashKeyToTrackers; - } - else - { - gc.hashKeyEntry = gc.keyValueStore; - } + if (hashKeyEntryPtr == NULL) + { + KeyToValuesHashTraits::AddToValuesInHeapMemory( + &keyValueStore, + keyValueStoreHolder, + key, + pLoaderAllocatorOfValue == m_pLoaderAllocator ? value : TRAITS::NullValue()); - gc.keyToTrackersHash.Add(&key, [&gc](PTRARRAYREF arr, INT32 index) - { - arr->SetAt(index, (OBJECTREF)gc.hashKeyEntry); - }); + if (pLoaderAllocatorOfValue != m_pLoaderAllocator) + { + hashKeyToTrackersHolder = hashKeyToTrackers = new LAHashKeyToTrackers(keyValueStore); + keyValueStoreHolder.SuppressRelease(); + hashKeyEntry = hashKeyToTrackers; } else { - gc.keyToTrackersHash.GetElement(index, gc.hashKeyEntry); - - if (gc.hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) - { - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)gc.hashKeyEntry; - gc.keyValueStore = gc.hashKeyToTrackers->_laLocalKeyValueStore; - } - else - { - gc.keyValueStore = gc.hashKeyEntry; - } - - bool updatedKeyValueStore = false; + hashKeyEntry = keyValueStore; + } - if (pLoaderAllocatorOfValue == m_pLoaderAllocator) - { - updatedKeyValueStore = TRAITS::AddToValuesInHeapMemory(&gc.keyValueStore, key, value); - } + keyToTrackersHash.Add(hashKeyEntry); + keyValueStoreHolder.SuppressRelease(); + hashKeyToTrackersHolder.SuppressRelease(); + } + else + { + hashKeyEntry = *hashKeyEntryPtr; + if (hashKeyEntry->IsLAHashKeyToTrackers()) + { + hashKeyToTrackers = static_cast(hashKeyEntry); + keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; + } + else + { + keyValueStore = static_cast(hashKeyEntry); + } + if (pLoaderAllocatorOfValue == m_pLoaderAllocator) + { + bool updatedKeyValueStore = + KeyToValuesHashTraits::AddToValuesInHeapMemory(&keyValueStore, keyValueStoreHolder, key, value); if (updatedKeyValueStore) { - if (gc.hashKeyToTrackers != NULL) + if (hashKeyToTrackers != NULL) { - SetObjectReference(&gc.hashKeyToTrackers->_laLocalKeyValueStore, gc.keyValueStore); + hashKeyToTrackers->_laLocalKeyValueStore = keyValueStore; } else { - gc.hashKeyEntry = gc.keyValueStore; - gc.keyToTrackersHash.SetElement(index, gc.hashKeyEntry); + hashKeyEntry = keyValueStore; + keyToTrackersHash.ReplacePtr(hashKeyEntryPtr, hashKeyEntry); } + + keyValueStoreHolder.SuppressRelease(); } } + } - // If the LoaderAllocator matches, we've finished adding by now, otherwise, we need to get the remove hash and work with that - if (pLoaderAllocatorOfValue != m_pLoaderAllocator) + // If the LoaderAllocator matches, we've finished adding by now, otherwise, we need to get the remove hash and work with that + if (pLoaderAllocatorOfValue != m_pLoaderAllocator) + { + if (hashKeyToTrackers == NULL) { - if (gc.hashKeyToTrackers == NULL) - { - // Nothing has yet caused the trackers proxy object to be setup. Create it now, and update the keyToTrackersHash - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)AllocateObject(CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)); - SetObjectReference(&gc.hashKeyToTrackers->_laLocalKeyValueStore, gc.keyValueStore); - gc.hashKeyEntry = gc.hashKeyToTrackers; - gc.keyToTrackersHash.SetElement(index, gc.hashKeyEntry); - } + // Nothing has yet caused the trackers proxy object to be setup. Create it now, and update the keyToTrackersHash. + // Don't need to use the holder here since there's no allocation before it's put into the hash table. + hashKeyToTrackers = new LAHashKeyToTrackers(keyValueStore); + hashKeyEntry = hashKeyToTrackers; + keyToTrackersHash.ReplacePtr(hashKeyEntryPtr, hashKeyEntry); + } - // Must add it to the cross LA structure - GCHEAPHASHOBJECTREF gcheapKeyToValue = GetKeyToValueCrossLAHashForHashkeyToTrackers(gc.hashKeyToTrackers, pLoaderAllocatorOfValue); + // Must add it to the cross LA structure + KeyToValuesHash *keyToValuePerLAHash = + GetKeyToValueCrossLAHashForHashkeyToTrackers(hashKeyToTrackers, pLoaderAllocatorOfValue); - gc.keyToValuePerLAHash = KeyToValuesGCHeapHash(gcheapKeyToValue); + KeyValueStoreOrLAHashKeyToTrackers *const *hashKeyEntryInPerLAHashPtr = keyToValuePerLAHash->LookupPtr(key); + if (hashKeyEntryInPerLAHashPtr != NULL) + { + _ASSERTE(!(*hashKeyEntryInPerLAHashPtr)->IsLAHashKeyToTrackers()); + keyValueStore = static_cast(*hashKeyEntryInPerLAHashPtr); - INT32 indexInKeyValueHash = gc.keyToValuePerLAHash.GetValueIndex(&key); - if (indexInKeyValueHash != -1) + if (KeyToValuesHashTraits::AddToValuesInHeapMemory(&keyValueStore, keyValueStoreHolder, key, value)) { - gc.keyToValuePerLAHash.GetElement(indexInKeyValueHash, gc.keyValueStore); - - if (TRAITS::AddToValuesInHeapMemory(&gc.keyValueStore, key, value)) - { - gc.keyToValuePerLAHash.SetElement(indexInKeyValueHash, gc.keyValueStore); - } + keyToValuePerLAHash->ReplacePtr(hashKeyEntryInPerLAHashPtr, keyValueStore); + keyValueStoreHolder.SuppressRelease(); } - else - { - gc.keyValueStore = NULL; - TRAITS::AddToValuesInHeapMemory(&gc.keyValueStore, key, value); + } + else + { + keyValueStore = NULL; + KeyToValuesHashTraits::AddToValuesInHeapMemory(&keyValueStore, keyValueStoreHolder, key, value); - gc.keyToValuePerLAHash.Add(&key, [&gc](PTRARRAYREF arr, INT32 index) - { - arr->SetAt(index, gc.keyValueStore); - }); - } + keyToValuePerLAHash->Add(keyValueStore); + keyValueStoreHolder.SuppressRelease(); } } - GCPROTECT_END(); } #endif // !DACCESS_COMPILE @@ -473,72 +477,54 @@ void CrossLoaderAllocatorHash::Remove(TKey key, TValue value, LoaderAllo { CONTRACTL { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; // This data structure actually doesn't have this invariant, but it is expected that uses of this // data structure will require that the key's loader allocator is the same as that of this data structure. - _ASSERTE(key->GetLoaderAllocator() == m_pLoaderAllocator); + _ASSERTE(TRAITS::GetLoaderAllocator(key) == m_pLoaderAllocator); - if (m_keyToDependentTrackersHash == NULL) + KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = keyToTrackersHash.Lookup(key); + if (hashKeyEntry == NULL) { - // If the heap objects haven't been initted, then there is nothing to delete return; } - struct { - KeyToValuesGCHeapHash keyToTrackersHash; - KeyToValuesGCHeapHash keyToValuePerLAHash; - OBJECTREF hashKeyEntry; - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - OBJECTREF keyValueStore; - } gc; + LAHashKeyToTrackers *hashKeyToTrackers = NULL; + KeyValueStore *keyValueStore; - ZeroMemory(&gc, sizeof(gc)); - GCPROTECT_BEGIN(gc) + if (hashKeyEntry->IsLAHashKeyToTrackers()) { - gc.keyToTrackersHash = KeyToValuesGCHeapHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_keyToDependentTrackersHash)); - INT32 index = gc.keyToTrackersHash.GetValueIndex(&key); + hashKeyToTrackers = static_cast(hashKeyEntry); + keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; + } + else + { + keyValueStore = static_cast(hashKeyEntry); + } - if (index != -1) + // Check to see if value can be removed from this data structure directly. + if (m_pLoaderAllocator == pLoaderAllocatorOfValue) + { + KeyToValuesHashTraits::DeleteValueInHeapMemory(keyValueStore, value); + } + else if (hashKeyToTrackers != NULL) + { + // Must remove it from the cross LA structure + KeyToValuesHash *keyToValuePerLAHash = + GetKeyToValueCrossLAHashForHashkeyToTrackers(hashKeyToTrackers, pLoaderAllocatorOfValue); + hashKeyEntry = keyToValuePerLAHash->Lookup(key); + if (hashKeyEntry != NULL) { - gc.keyToTrackersHash.GetElement(index, gc.hashKeyEntry); - - if (gc.hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) - { - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)gc.hashKeyEntry; - gc.keyValueStore = gc.hashKeyToTrackers->_laLocalKeyValueStore; - } - else - { - gc.keyValueStore = gc.hashKeyEntry; - } - - // Check to see if value can be added to this data structure directly. - if (m_pLoaderAllocator == pLoaderAllocatorOfValue) - { - TRAITS::DeleteValueInHeapMemory(gc.keyValueStore, value); - } - else if (gc.hashKeyToTrackers != NULL) - { - // Must remove it from the cross LA structure - GCHEAPHASHOBJECTREF gcheapKeyToValue = GetKeyToValueCrossLAHashForHashkeyToTrackers(gc.hashKeyToTrackers, pLoaderAllocatorOfValue); - - gc.keyToValuePerLAHash = KeyToValuesGCHeapHash(gcheapKeyToValue); - - INT32 indexInKeyValueHash = gc.keyToValuePerLAHash.GetValueIndex(&key); - if (indexInKeyValueHash != -1) - { - gc.keyToValuePerLAHash.GetElement(indexInKeyValueHash, gc.keyValueStore); - TRAITS::DeleteValueInHeapMemory(gc.keyValueStore, value); - } - } + _ASSERTE(!hashKeyEntry->IsLAHashKeyToTrackers()); + keyValueStore = static_cast(hashKeyEntry); + KeyToValuesHashTraits::DeleteValueInHeapMemory(keyValueStore, value); } } - GCPROTECT_END(); } #endif // !DACCESS_COMPILE @@ -548,178 +534,101 @@ bool CrossLoaderAllocatorHash::VisitValuesOfKey(TKey key, Visitor &visit { WRAPPER_NO_CONTRACT; - class VisitIndividualEntryKeyValueHash - { - public: - TKey m_key; - Visitor *m_pVisitor; - GCHeapHashDependentHashTrackerHash *m_pDependentTrackerHash; - - VisitIndividualEntryKeyValueHash(TKey key, Visitor *pVisitor, GCHeapHashDependentHashTrackerHash *pDependentTrackerHash) : - m_key(key), - m_pVisitor(pVisitor), - m_pDependentTrackerHash(pDependentTrackerHash) - {} - - bool operator()(INT32 index) - { - WRAPPER_NO_CONTRACT; - - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - m_pDependentTrackerHash->GetElement(index, dependentTracker); - return VisitTracker(m_key, dependentTracker, *m_pVisitor); - } - }; - // This data structure actually doesn't have this invariant, but it is expected that uses of this // data structure will require that the key's loader allocator is the same as that of this data structure. - _ASSERTE(key->GetLoaderAllocator() == m_pLoaderAllocator); + _ASSERTE(TRAITS::GetLoaderAllocator(key) == m_pLoaderAllocator); - // Check to see that something has been added - if (m_keyToDependentTrackersHash == NULL) + KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = keyToTrackersHash.Lookup(key); + if (hashKeyEntry == NULL) + { return true; + } + + // We have an entry in the hashtable for the key/dependenthandle. + LAHashKeyToTrackers *hashKeyToTrackers = NULL; + KeyValueStore *keyValueStore; - bool result = true; - struct + if (hashKeyEntry->IsLAHashKeyToTrackers()) { - KeyToValuesGCHeapHash keyToTrackersHash; - GCHeapHashDependentHashTrackerHash dependentTrackerHash; - LAHASHDEPENDENTHASHTRACKERREF dependentTrackerMaybe; - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - OBJECTREF hashKeyEntry; - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - OBJECTREF keyValueStore; - OBJECTREF nullref; - } gc; - ZeroMemory(&gc, sizeof(gc)); - GCPROTECT_BEGIN(gc) + hashKeyToTrackers = static_cast(hashKeyEntry); + keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; + } + else { - gc.keyToTrackersHash = KeyToValuesGCHeapHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_keyToDependentTrackersHash)); - INT32 index = gc.keyToTrackersHash.GetValueIndex(&key); - if (index != -1) - { - // We have an entry in the hashtable for the key/dependenthandle. - gc.keyToTrackersHash.GetElement(index, gc.hashKeyEntry); + keyValueStore = static_cast(hashKeyEntry); + } - if (gc.hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) - { - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)gc.hashKeyEntry; - gc.keyValueStore = gc.hashKeyToTrackers->_laLocalKeyValueStore; - } - else - { - gc.keyValueStore = gc.hashKeyEntry; - } + // Now hashKeyToTrackers is filled in and keyValueStore - // Now gc.hashKeyToTrackers is filled in and keyValueStore + // visit local entries + if (!VisitKeyValueStore(NULL, keyValueStore, visitor)) + { + return false; + } - // visit local entries - result = VisitKeyValueStore(&gc.nullref, &gc.keyValueStore, visitor); + if (hashKeyToTrackers == NULL) + { + return true; + } - if (gc.hashKeyToTrackers != NULL) - { - // Is there a single dependenttracker here, or a set. + // Is there a single dependenttracker here, or a set. - if (gc.hashKeyToTrackers->_trackerOrTrackerSet->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHDEPENDENTHASHTRACKER)) - { - gc.dependentTracker = (LAHASHDEPENDENTHASHTRACKERREF)gc.hashKeyToTrackers->_trackerOrTrackerSet; - result = VisitTracker(key, gc.dependentTracker, visitor); - } - else - { - gc.dependentTrackerHash = GCHeapHashDependentHashTrackerHash(gc.hashKeyToTrackers->_trackerOrTrackerSet); - VisitIndividualEntryKeyValueHash visitIndivididualKeys(key, &visitor, &gc.dependentTrackerHash); - result = gc.dependentTrackerHash.VisitAllEntryIndices(visitIndivididualKeys); - } + if (!hashKeyToTrackers->_trackerOrTrackerSet->IsTrackerSet()) + { + LAHashDependentHashTracker *dependentTracker = + static_cast(hashKeyToTrackers->_trackerOrTrackerSet); + return VisitTracker(key, dependentTracker, visitor); + } + else + { + LAHashDependentHashTrackerHash *dependentTrackerHash = + static_cast(hashKeyToTrackers->_trackerOrTrackerSet)->GetTrackerSet(); + for (LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash->Begin(), + itEnd = dependentTrackerHash->End(); + it != itEnd; + ++it) + { + LAHashDependentHashTracker *dependentTracker = *it; + if (!VisitTracker(key, dependentTracker, visitor)) + { + return false; } } - } - GCPROTECT_END(); - return result; + return true; + } } template template bool CrossLoaderAllocatorHash::VisitAllKeyValuePairs(Visitor &visitor) { - CONTRACTL - { - MODE_COOPERATIVE; - GC_NOTRIGGER; - } - CONTRACTL_END; - - class VisitAllEntryKeyToDependentTrackerHash - { - public: - Visitor *m_pVisitor; - KeyToValuesGCHeapHash *m_pKeyToTrackerHash; - - VisitAllEntryKeyToDependentTrackerHash(Visitor *pVisitor, KeyToValuesGCHeapHash *pKeyToTrackerHash) : - m_pVisitor(pVisitor), - m_pKeyToTrackerHash(pKeyToTrackerHash) - {} - - bool operator()(INT32 index) - { - WRAPPER_NO_CONTRACT; - - OBJECTREF hashKeyEntry; - m_pKeyToTrackerHash->GetElement(index, hashKeyEntry); - return VisitKeyToTrackerAllEntries(hashKeyEntry, *m_pVisitor); - } - }; + WRAPPER_NO_CONTRACT; - class VisitAllEntryDependentTrackerHash + KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; + for (KeyToValuesHash::Iterator it = keyToTrackersHash.Begin(), itEnd = keyToTrackersHash.End(); it != itEnd; ++it) { - public: - Visitor *m_pVisitor; - GCHeapHashDependentHashTrackerHash *m_pDependentTrackerHash; - - VisitAllEntryDependentTrackerHash(Visitor *pVisitor, GCHeapHashDependentHashTrackerHash *pDependentTrackerHash) : - m_pVisitor(pVisitor), - m_pDependentTrackerHash(pDependentTrackerHash) - {} - - bool operator()(INT32 index) + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *it; + if (!VisitKeyToTrackerAllLALocalEntries(hashKeyEntry, visitor)) { - WRAPPER_NO_CONTRACT; - - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - m_pDependentTrackerHash->GetElement(index, dependentTracker); - return VisitTrackerAllEntries(dependentTracker, *m_pVisitor); + return false; } - }; + } - struct + LAHashDependentHashTrackerHash &dependentTrackerHash = m_loaderAllocatorToDependentTrackerHash; + for (LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash.Begin(), itEnd = dependentTrackerHash.End(); + it != itEnd; + ++it) { - KeyToValuesGCHeapHash keyToTrackersHash; - GCHeapHashDependentHashTrackerHash dependentTrackerHash; - } gc; - ZeroMemory(&gc, sizeof(gc)); - bool result = true; - GCPROTECT_BEGIN(gc) - { - if (m_keyToDependentTrackersHash != NULL) - { - // Visit all local entries - gc.keyToTrackersHash = KeyToValuesGCHeapHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_keyToDependentTrackersHash)); - VisitAllEntryKeyToDependentTrackerHash visitAllEntryKeys(&visitor, &gc.keyToTrackersHash); - result = gc.keyToTrackersHash.VisitAllEntryIndices(visitAllEntryKeys); - } - - if (m_loaderAllocatorToDependentTrackerHash != NULL) + LAHashDependentHashTracker *dependentTracker = *it; + if (!VisitTrackerAllEntries(dependentTracker, visitor)) { - // Visit the non-local data - gc.dependentTrackerHash = GCHeapHashDependentHashTrackerHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_loaderAllocatorToDependentTrackerHash)); - VisitAllEntryDependentTrackerHash visitDependentTrackers(&visitor, &gc.dependentTrackerHash); - result = gc.dependentTrackerHash.VisitAllEntryIndices(visitDependentTrackers); + return false; } } - GCPROTECT_END(); - return result; + return true; } #ifndef DACCESS_COMPILE @@ -728,96 +637,67 @@ void CrossLoaderAllocatorHash::RemoveAll(TKey key) { CONTRACTL { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; - class DeleteIndividualEntryKeyValueHash - { - public: - TKey m_key; - GCHeapHashDependentHashTrackerHash *m_pDependentTrackerHash; - - DeleteIndividualEntryKeyValueHash(TKey key, GCHeapHashDependentHashTrackerHash *pDependentTrackerHash) : - m_key(key), - m_pDependentTrackerHash(pDependentTrackerHash) - {} - - bool operator()(INT32 index) - { - WRAPPER_NO_CONTRACT; - - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - m_pDependentTrackerHash->GetElement(index, dependentTracker); - DeleteEntryTracker(m_key, dependentTracker); - return true; - } - }; - // This data structure actually doesn't have this invariant, but it is expected that uses of this // data structure will require that the key's loader allocator is the same as that of this data structure. - _ASSERTE(key->GetLoaderAllocator() == m_pLoaderAllocator); + _ASSERTE(TRAITS::GetLoaderAllocator(key) == m_pLoaderAllocator); - if (m_keyToDependentTrackersHash == NULL) + KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; + KeyValueStoreOrLAHashKeyToTrackers *const *hashKeyEntryPtr = keyToTrackersHash.LookupPtr(key); + if (hashKeyEntryPtr == NULL) { - return; // Nothing was ever added, so removing all is easy + return; } - struct + // We have an entry in the hashtable for the key/dependenthandle. + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *hashKeyEntryPtr; + LAHashKeyToTrackers *hashKeyToTrackers = NULL; + KeyValueStore *keyValueStore; + + if (hashKeyEntry->IsLAHashKeyToTrackers()) { - KeyToValuesGCHeapHash keyToTrackersHash; - GCHeapHashDependentHashTrackerHash dependentTrackerHash; - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - OBJECTREF hashKeyEntry; - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - OBJECTREF keyValueStore; - } gc; - ZeroMemory(&gc, sizeof(gc)); - GCPROTECT_BEGIN(gc) + hashKeyToTrackers = static_cast(hashKeyEntry); + keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; + } + else { - gc.keyToTrackersHash = KeyToValuesGCHeapHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_keyToDependentTrackersHash)); - INT32 index = gc.keyToTrackersHash.GetValueIndex(&key); - if (index != -1) - { - // We have an entry in the hashtable for the key/dependenthandle. - gc.keyToTrackersHash.GetElement(index, gc.hashKeyEntry); + keyValueStore = static_cast(hashKeyEntry); + } - if (gc.hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) - { - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)gc.hashKeyEntry; - gc.keyValueStore = gc.hashKeyToTrackers->_laLocalKeyValueStore; - } - else - { - gc.keyValueStore = gc.hashKeyEntry; - } + // Now hashKeyToTrackers is filled in - // Now gc.hashKeyToTrackers is filled in + if (hashKeyToTrackers != NULL) + { + // Is there a single dependenttracker here, or a set. - if (gc.hashKeyToTrackers != NULL) + if (!hashKeyToTrackers->_trackerOrTrackerSet->IsTrackerSet()) + { + LAHashDependentHashTracker *dependentTracker = + static_cast(hashKeyToTrackers->_trackerOrTrackerSet); + DeleteEntryTracker(key, dependentTracker); + } + else + { + LAHashDependentHashTrackerHash *dependentTrackerHash = + static_cast(hashKeyToTrackers->_trackerOrTrackerSet)->GetTrackerSet(); + for (LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash->Begin(), + itEnd = dependentTrackerHash->End(); + it != itEnd; + ++it) { - // Is there a single dependenttracker here, or a set. - - if (gc.hashKeyToTrackers->_trackerOrTrackerSet->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHDEPENDENTHASHTRACKER)) - { - gc.dependentTracker = (LAHASHDEPENDENTHASHTRACKERREF)gc.hashKeyToTrackers->_trackerOrTrackerSet; - DeleteEntryTracker(key, gc.dependentTracker); - } - else - { - gc.dependentTrackerHash = GCHeapHashDependentHashTrackerHash(gc.hashKeyToTrackers->_trackerOrTrackerSet); - DeleteIndividualEntryKeyValueHash deleteIndividualKeyValues(key, &gc.dependentTrackerHash); - gc.dependentTrackerHash.VisitAllEntryIndices(deleteIndividualKeyValues); - } + LAHashDependentHashTracker *dependentTracker = *it; + DeleteEntryTracker(key, dependentTracker); } - - // Remove entry from key to tracker hash - gc.keyToTrackersHash.DeleteEntry(&key); } } - GCPROTECT_END(); + + // Remove entry from key to tracker hash + keyToTrackersHash.RemovePtr(const_cast(hashKeyEntryPtr)); } #endif // !DACCESS_COMPILE @@ -830,399 +710,201 @@ void CrossLoaderAllocatorHash::Init(LoaderAllocator *pAssociatedLoaderAl template template -/*static*/ bool CrossLoaderAllocatorHash::VisitKeyValueStore(OBJECTREF *pLoaderAllocatorRef, OBJECTREF *pKeyValueStore, Visitor &visitor) +/*static*/ bool CrossLoaderAllocatorHash::VisitKeyValueStore(LoaderAllocator *loaderAllocator, KeyValueStore *keyValueStore, Visitor &visitor) { WRAPPER_NO_CONTRACT; - return TRAITS::VisitKeyValueStore(pLoaderAllocatorRef, pKeyValueStore, visitor); + return KeyToValuesHashTraits::VisitKeyValueStore(loaderAllocator, keyValueStore, visitor); } template template -/*static*/ bool CrossLoaderAllocatorHash::VisitTracker(TKey key, LAHASHDEPENDENTHASHTRACKERREF trackerUnsafe, Visitor &visitor) +/*static*/ bool CrossLoaderAllocatorHash::VisitTracker(TKey key, LAHashDependentHashTracker *tracker, Visitor &visitor) { - CONTRACTL - { - MODE_COOPERATIVE; - GC_NOTRIGGER; - } - CONTRACTL_END; - - struct - { - LAHASHDEPENDENTHASHTRACKERREF tracker; - OBJECTREF loaderAllocatorRef; - GCHEAPHASHOBJECTREF keyToValuesHashObject; - KeyToValuesGCHeapHash keyToValuesHash; - OBJECTREF keyValueStore; - }gc; - - ZeroMemory(&gc, sizeof(gc)); - gc.tracker = trackerUnsafe; - - bool result = true; + WRAPPER_NO_CONTRACT; - GCPROTECT_BEGIN(gc); + KeyToValuesHash *keyToValuesHash = tracker->GetDependentKeyToValuesHash(); + if (keyToValuesHash != NULL) { - gc.tracker->GetDependentAndLoaderAllocator(&gc.loaderAllocatorRef, &gc.keyToValuesHashObject); - if (gc.keyToValuesHashObject != NULL) + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = keyToValuesHash->Lookup(key); + if (hashKeyEntry != NULL) { - gc.keyToValuesHash = KeyToValuesGCHeapHash(gc.keyToValuesHashObject); - INT32 indexInKeyValueHash = gc.keyToValuesHash.GetValueIndex(&key); - if (indexInKeyValueHash != -1) - { - gc.keyToValuesHash.GetElement(indexInKeyValueHash, gc.keyValueStore); - - result = VisitKeyValueStore(&gc.loaderAllocatorRef, &gc.keyValueStore, visitor); - } + _ASSERTE(!hashKeyEntry->IsLAHashKeyToTrackers()); + return VisitKeyValueStore(tracker->GetLoaderAllocatorUnsafe(), static_cast(hashKeyEntry), visitor); } } - GCPROTECT_END(); - return result; + return true; } template template -/*static*/ bool CrossLoaderAllocatorHash::VisitTrackerAllEntries(LAHASHDEPENDENTHASHTRACKERREF trackerUnsafe, Visitor &visitor) +/*static*/ bool CrossLoaderAllocatorHash::VisitTrackerAllEntries(LAHashDependentHashTracker *tracker, Visitor &visitor) { - CONTRACTL - { - MODE_COOPERATIVE; - GC_NOTRIGGER; - } - CONTRACTL_END; - - struct - { - LAHASHDEPENDENTHASHTRACKERREF tracker; - OBJECTREF loaderAllocatorRef; - GCHEAPHASHOBJECTREF keyToValuesHashObject; - KeyToValuesGCHeapHash keyToValuesHash; - OBJECTREF keyValueStore; - }gc; - - class VisitAllEntryKeyValueHash - { - public: - Visitor *m_pVisitor; - KeyToValuesGCHeapHash *m_pKeysToValueHash; - OBJECTREF *m_pKeyValueStore; - OBJECTREF *m_pLoaderAllocatorRef; - - VisitAllEntryKeyValueHash(Visitor *pVisitor, KeyToValuesGCHeapHash *pKeysToValueHash, OBJECTREF *pKeyValueStore, OBJECTREF *pLoaderAllocatorRef) : - m_pVisitor(pVisitor), - m_pKeysToValueHash(pKeysToValueHash), - m_pKeyValueStore(pKeyValueStore), - m_pLoaderAllocatorRef(pLoaderAllocatorRef) - {} - - bool operator()(INT32 index) - { - WRAPPER_NO_CONTRACT; - - m_pKeysToValueHash->GetElement(index, *m_pKeyValueStore); - return VisitKeyValueStore(m_pLoaderAllocatorRef, m_pKeyValueStore, m_pVisitor); - } - }; - - ZeroMemory(&gc, sizeof(gc)); - gc.tracker = trackerUnsafe; - - bool result = true; + WRAPPER_NO_CONTRACT; - GCPROTECT_BEGIN(gc); + KeyToValuesHash *keyToValuesHash = tracker->GetDependentKeyToValuesHash(); + if (keyToValuesHash != NULL) { - gc.tracker->GetDependentAndLoaderAllocator(&gc.loaderAllocatorRef, &gc.keyToValuesHashObject); - if (gc.keyToValuesHashObject != NULL) + LoaderAllocator *loaderAllocator = tracker->GetLoaderAllocatorUnsafe(); + for (KeyToValuesHash::Iterator it = keyToValuesHash->Begin(), itEnd = keyToValuesHash->End(); it != itEnd; ++it) { - gc.keyToValuesHash = KeyToValuesGCHeapHash(gc.keyToValuesHashObject); - result = gc.keyToValuesHash.VisitAllEntryIndices(VisitAllEntryKeyValueHash(&visitor, &gc.keyToValuesHash, &gc.keyValueStore, &gc.loaderAllocatorRef)); + KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *it; + _ASSERTE(!hashKeyEntry->IsLAHashKeyToTrackers()); + if (!VisitKeyValueStore(loaderAllocator, static_cast(hashKeyEntry), visitor)) + { + return false; + } } } - GCPROTECT_END(); - return result; + return true; } template template -/*static*/ bool CrossLoaderAllocatorHash::VisitKeyToTrackerAllEntries(OBJECTREF hashKeyEntryUnsafe, Visitor &visitor) +/*static*/ bool CrossLoaderAllocatorHash::VisitKeyToTrackerAllLALocalEntries(KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry, Visitor &visitor) { - CONTRACTL - { - MODE_COOPERATIVE; - GC_NOTRIGGER; - } - CONTRACTL_END; + WRAPPER_NO_CONTRACT; - struct + KeyValueStore *keyValueStore; + if (hashKeyEntry->IsLAHashKeyToTrackers()) { - OBJECTREF hashKeyEntry; - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - OBJECTREF keyValueStore; - OBJECTREF loaderAllocatorRef; - } gc; - - ZeroMemory(&gc, sizeof(gc)); - gc.hashKeyEntry = hashKeyEntryUnsafe; - - bool result = true; - - GCPROTECT_BEGIN(gc); + LAHashKeyToTrackers *hashKeyToTrackers = static_cast(hashKeyEntry); + keyValueStore = hashKeyToTrackers->_laLocalKeyValueStore; + } + else { - if (gc.hashKeyEntry->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHKEYTOTRACKERS)) - { - gc.hashKeyToTrackers = (LAHASHKEYTOTRACKERSREF)gc.hashKeyEntry; - gc.keyValueStore = gc.hashKeyToTrackers->_laLocalKeyValueStore; - } - else - { - gc.keyValueStore = gc.hashKeyEntry; - } - - result = VisitKeyValueStore(&gc.loaderAllocatorRef, &gc.keyValueStore, visitor); + keyValueStore = static_cast(hashKeyEntry); } - GCPROTECT_END(); - return result; + return VisitKeyValueStore(NULL, keyValueStore, visitor); } template -/*static*/ void CrossLoaderAllocatorHash::DeleteEntryTracker(TKey key, LAHASHDEPENDENTHASHTRACKERREF trackerUnsafe) +/*static*/ void CrossLoaderAllocatorHash::DeleteEntryTracker(TKey key, LAHashDependentHashTracker *tracker) { CONTRACTL { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; - struct - { - LAHASHDEPENDENTHASHTRACKERREF tracker; - OBJECTREF loaderAllocatorRef; - GCHEAPHASHOBJECTREF keyToValuesHashObject; - KeyToValuesGCHeapHash keyToValuesHash; - }gc; - - ZeroMemory(&gc, sizeof(gc)); - gc.tracker = trackerUnsafe; - - GCPROTECT_BEGIN(gc); + KeyToValuesHash *keyToValuesHash = tracker->GetDependentKeyToValuesHash(); + if (keyToValuesHash != NULL) { - gc.tracker->GetDependentAndLoaderAllocator(&gc.loaderAllocatorRef, &gc.keyToValuesHashObject); - if (gc.keyToValuesHashObject != NULL) - { - gc.keyToValuesHash = KeyToValuesGCHeapHash(gc.keyToValuesHashObject); - gc.keyToValuesHash.DeleteEntry(&key); - } + keyToValuesHash->Remove(key); } - GCPROTECT_END(); } #ifndef DACCESS_COMPILE -/*static */inline void CrossLoaderAllocatorHashSetup::EnsureTypesLoaded() -{ - STANDARD_VM_CONTRACT; - - // Force these types to be loaded, so that the hashtable logic can use CoreLibBinder::GetExistingClass - // throughout and avoid lock ordering issues - CoreLibBinder::GetClass(CLASS__LAHASHKEYTOTRACKERS); - CoreLibBinder::GetClass(CLASS__LAHASHDEPENDENTHASHTRACKER); - CoreLibBinder::GetClass(CLASS__GCHEAPHASH); - TypeHandle elemType = TypeHandle(CoreLibBinder::GetElementType(ELEMENT_TYPE_I1)); - TypeHandle typHnd = ClassLoader::LoadArrayTypeThrowing(elemType, ELEMENT_TYPE_SZARRAY, 0); - elemType = TypeHandle(CoreLibBinder::GetElementType(ELEMENT_TYPE_OBJECT)); - typHnd = ClassLoader::LoadArrayTypeThrowing(elemType, ELEMENT_TYPE_SZARRAY, 0); -} - template -void CrossLoaderAllocatorHash::EnsureManagedObjectsInitted() +typename CrossLoaderAllocatorHash::LAHashDependentHashTracker * +CrossLoaderAllocatorHash::GetDependentTrackerForLoaderAllocator(LoaderAllocator *pLoaderAllocator) { CONTRACTL { THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; - if (m_loaderAllocatorToDependentTrackerHash == NULL) + LAHashDependentHashTrackerHash &dependentTrackerHash = m_loaderAllocatorToDependentTrackerHash; + LAHashDependentHashTracker *dependentTracker = dependentTrackerHash.Lookup(pLoaderAllocator); + if (dependentTracker != NULL) { - OBJECTREF laToDependentHandleHashObject = AllocateObject(CoreLibBinder::GetExistingClass(CLASS__GCHEAPHASH)); - m_loaderAllocatorToDependentTrackerHash = m_pLoaderAllocator->GetDomain()->CreateHandle(laToDependentHandleHashObject); - m_pLoaderAllocator->RegisterHandleForCleanup(m_loaderAllocatorToDependentTrackerHash); + // We have an entry in the hashtable for the key/dependenthandle. + return dependentTracker; } - if (m_keyToDependentTrackersHash == NULL) - { - OBJECTREF m_keyToDependentTrackersHashObject = AllocateObject(CoreLibBinder::GetExistingClass(CLASS__GCHEAPHASH)); - m_keyToDependentTrackersHash = m_pLoaderAllocator->GetDomain()->CreateHandle(m_keyToDependentTrackersHashObject); - m_pLoaderAllocator->RegisterHandleForCleanup(m_keyToDependentTrackersHash); - } + NewHolder laDependentKeyToValuesHashHolder = new LADependentKeyToValuesHash(); + LAHashDependentHashTracker::NewTrackerHolder dependentTrackerHolder = + new LAHashDependentHashTracker(pLoaderAllocator, laDependentKeyToValuesHashHolder); + laDependentKeyToValuesHashHolder.SuppressRelease(); + + dependentTrackerHash.Add(dependentTrackerHolder); + return dependentTrackerHolder.Extract(); } #endif // !DACCESS_COMPILE #ifndef DACCESS_COMPILE template -LAHASHDEPENDENTHASHTRACKERREF CrossLoaderAllocatorHash::GetDependentTrackerForLoaderAllocator(LoaderAllocator* pLoaderAllocator) +typename CrossLoaderAllocatorHash::KeyToValuesHash * +CrossLoaderAllocatorHash::GetKeyToValueCrossLAHashForHashkeyToTrackers( + LAHashKeyToTrackers *hashKeyToTrackers, + LoaderAllocator *pValueLoaderAllocator) { CONTRACTL { THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + GC_NOTRIGGER; + MODE_ANY; } CONTRACTL_END; - struct - { - GCHeapHashDependentHashTrackerHash dependentTrackerHash; - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - GCHEAPHASHOBJECTREF GCHeapHashForKeyToValueStore; - } gc; - ZeroMemory(&gc, sizeof(gc)); - GCPROTECT_BEGIN(gc) - { - gc.dependentTrackerHash = GCHeapHashDependentHashTrackerHash((GCHEAPHASHOBJECTREF)ObjectFromHandle(m_loaderAllocatorToDependentTrackerHash)); - INT32 index = gc.dependentTrackerHash.GetValueIndex(&pLoaderAllocator); - if (index != -1) - { - // We have an entry in the hashtable for the key/dependenthandle. - gc.dependentTrackerHash.GetElement(index, gc.dependentTracker); - } - else - { - gc.dependentTracker = (LAHASHDEPENDENTHASHTRACKERREF)AllocateObject(CoreLibBinder::GetExistingClass(CLASS__LAHASHDEPENDENTHASHTRACKER)); - gc.GCHeapHashForKeyToValueStore = (GCHEAPHASHOBJECTREF)AllocateObject(CoreLibBinder::GetExistingClass(CLASS__GCHEAPHASH)); + LAHashDependentHashTracker *dependentTracker; - OBJECTREF exposedObject = pLoaderAllocator->GetExposedObject(); - if (exposedObject == NULL) - { - if (m_globalDependentTrackerRootHandle == NULL) - { - // Global LoaderAllocator does not have an exposed object, so create a fake one - exposedObject = AllocateObject(CoreLibBinder::GetExistingClass(CLASS__OBJECT)); - m_globalDependentTrackerRootHandle = GetAppDomain()->CreateHandle(exposedObject); - m_pLoaderAllocator->RegisterHandleForCleanup(m_globalDependentTrackerRootHandle); - } - else - { - exposedObject = ObjectFromHandle(m_globalDependentTrackerRootHandle); - } - } - - OBJECTHANDLE dependentHandle = GetAppDomain()->CreateDependentHandle(exposedObject, gc.GCHeapHashForKeyToValueStore); - gc.dependentTracker->Init(dependentHandle, pLoaderAllocator); - gc.dependentTrackerHash.Add(&pLoaderAllocator, [&gc](PTRARRAYREF arr, INT32 index) - { - arr->SetAt(index, (OBJECTREF)gc.dependentTracker); - }); - } - } - GCPROTECT_END(); - - return gc.dependentTracker; -} -#endif // !DACCESS_COMPILE - -#ifndef DACCESS_COMPILE -template -GCHEAPHASHOBJECTREF CrossLoaderAllocatorHash::GetKeyToValueCrossLAHashForHashkeyToTrackers(LAHASHKEYTOTRACKERSREF hashKeyToTrackersUnsafe, LoaderAllocator* pValueLoaderAllocator) -{ - CONTRACTL + // Is there a single dependenttracker here, or a set, or no dependenttracker at all + if (hashKeyToTrackers->_trackerOrTrackerSet == NULL) { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; + dependentTracker = GetDependentTrackerForLoaderAllocator(pValueLoaderAllocator); + hashKeyToTrackers->_trackerOrTrackerSet = dependentTracker; + dependentTracker->IncRefCount(); } - CONTRACTL_END; - - struct - { - GCHeapHashDependentHashTrackerHash dependentTrackerHash; - LAHASHDEPENDENTHASHTRACKERREF dependentTrackerMaybe; - LAHASHDEPENDENTHASHTRACKERREF dependentTracker; - LAHASHKEYTOTRACKERSREF hashKeyToTrackers; - GCHEAPHASHOBJECTREF returnValue; - } gc; - ZeroMemory(&gc, sizeof(gc)); - // Now gc.hashKeyToTrackers is filled in. - gc.hashKeyToTrackers = hashKeyToTrackersUnsafe; - GCPROTECT_BEGIN(gc) + else if (!hashKeyToTrackers->_trackerOrTrackerSet->IsTrackerSet()) { - EnsureManagedObjectsInitted(); - - // Is there a single dependenttracker here, or a set, or no dependenttracker at all - if (gc.hashKeyToTrackers->_trackerOrTrackerSet == NULL) + LAHashDependentHashTracker *dependentTrackerMaybe = + static_cast(hashKeyToTrackers->_trackerOrTrackerSet); + if (dependentTrackerMaybe->IsTrackerFor(pValueLoaderAllocator)) { - gc.dependentTracker = GetDependentTrackerForLoaderAllocator(pValueLoaderAllocator); - SetObjectReference(&gc.hashKeyToTrackers->_trackerOrTrackerSet, gc.dependentTracker); + // We've found the right dependent tracker. + dependentTracker = dependentTrackerMaybe; } - else if (gc.hashKeyToTrackers->_trackerOrTrackerSet->GetMethodTable() == CoreLibBinder::GetExistingClass(CLASS__LAHASHDEPENDENTHASHTRACKER)) + else { - gc.dependentTrackerMaybe = (LAHASHDEPENDENTHASHTRACKERREF)gc.hashKeyToTrackers->_trackerOrTrackerSet; - if (gc.dependentTrackerMaybe->IsTrackerFor(pValueLoaderAllocator)) + dependentTracker = GetDependentTrackerForLoaderAllocator(pValueLoaderAllocator); + if (!dependentTrackerMaybe->IsLoaderAllocatorLive()) { - // We've found the right dependent tracker. - gc.dependentTracker = gc.dependentTrackerMaybe; + hashKeyToTrackers->_trackerOrTrackerSet = dependentTracker; + dependentTrackerMaybe->DecRefCount(); + dependentTracker->IncRefCount(); } else { - gc.dependentTracker = GetDependentTrackerForLoaderAllocator(pValueLoaderAllocator); - if (!gc.dependentTrackerMaybe->IsLoaderAllocatorLive()) - { - SetObjectReference(&gc.hashKeyToTrackers->_trackerOrTrackerSet, gc.dependentTracker); - } - else - { - // Allocate the dependent tracker hash - // Fill with the existing dependentTrackerMaybe, and gc.DependentTracker - gc.dependentTrackerHash = GCHeapHashDependentHashTrackerHash(AllocateObject(CoreLibBinder::GetExistingClass(CLASS__GCHEAPHASH))); - LoaderAllocator *pLoaderAllocatorKey = gc.dependentTracker->GetLoaderAllocatorUnsafe(); - gc.dependentTrackerHash.Add(&pLoaderAllocatorKey, [&gc](PTRARRAYREF arr, INT32 index) - { - arr->SetAt(index, (OBJECTREF)gc.dependentTracker); - }); - pLoaderAllocatorKey = gc.dependentTrackerMaybe->GetLoaderAllocatorUnsafe(); - gc.dependentTrackerHash.Add(&pLoaderAllocatorKey, [&gc](PTRARRAYREF arr, INT32 index) - { - arr->SetAt(index, (OBJECTREF)gc.dependentTrackerMaybe); - }); - SetObjectReference(&gc.hashKeyToTrackers->_trackerOrTrackerSet, gc.dependentTrackerHash.GetGCHeapRef()); - } + // Allocate the dependent tracker hash + // Fill with the existing dependentTrackerMaybe, and DependentTracker + NewHolder dependentTrackerHashWrapperHolder = + new LAHashDependentHashTrackerSetWrapper(); + LAHashDependentHashTrackerHash *dependentTrackerHash = dependentTrackerHashWrapperHolder->GetTrackerSet(); + dependentTrackerHash->Add(dependentTracker); + dependentTracker->IncRefCount(); + dependentTrackerHash->Add(dependentTrackerMaybe); + hashKeyToTrackers->_trackerOrTrackerSet = dependentTrackerHashWrapperHolder.Extract(); } } - else + } + else + { + LAHashDependentHashTrackerHash *dependentTrackerHash = + static_cast(hashKeyToTrackers->_trackerOrTrackerSet)->GetTrackerSet(); + + dependentTracker = dependentTrackerHash->Lookup(pValueLoaderAllocator); + if (dependentTracker == NULL) { - gc.dependentTrackerHash = GCHeapHashDependentHashTrackerHash(gc.hashKeyToTrackers->_trackerOrTrackerSet); + // Dependent tracker not yet attached to this key - INT32 indexOfTracker = gc.dependentTrackerHash.GetValueIndex(&pValueLoaderAllocator); - if (indexOfTracker == -1) - { - // Dependent tracker not yet attached to this key - - // Get dependent tracker - gc.dependentTracker = GetDependentTrackerForLoaderAllocator(pValueLoaderAllocator); - gc.dependentTrackerHash.Add(&pValueLoaderAllocator, [&gc](PTRARRAYREF arr, INT32 index) - { - arr->SetAt(index, (OBJECTREF)gc.dependentTracker); - }); - } - else - { - gc.dependentTrackerHash.GetElement(indexOfTracker, gc.dependentTracker); - } + // Get dependent tracker + dependentTracker = GetDependentTrackerForLoaderAllocator(pValueLoaderAllocator); + dependentTrackerHash->Add(dependentTracker); + dependentTracker->IncRefCount(); } - - // At this stage gc.dependentTracker is setup to have a good value - gc.dependentTracker->GetDependentAndLoaderAllocator(NULL, &gc.returnValue); } - GCPROTECT_END(); - return gc.returnValue; + // At this stage dependentTracker is setup to have a good value + return dependentTracker->GetDependentKeyToValuesHash(); } #endif // !DACCESS_COMPILE diff --git a/src/coreclr/vm/crst.h b/src/coreclr/vm/crst.h index a8f2065703d6e..b67b6c2708621 100644 --- a/src/coreclr/vm/crst.h +++ b/src/coreclr/vm/crst.h @@ -395,6 +395,16 @@ friend class Crst; // This just exists to convert legacy OS Critical Section patterns over to holders. typedef DacHolder UnsafeCrstInverseHolder; + // This holder acquires the lock in preemptive GC mode and puts the thread in a forbid-suspend-for-debugger region. It is + // used when cooperative GC mode would be entered while the lock is held, which can normally suspend for the debugger and + // cause FuncEvals to deadlock upon trying to acquire the lock. Using this holder prevents the thread from suspending for + // the debugger upon GC mode switches inside the forbid region. + // + // However, the forbid region does not prevent the thread from waiting for a pending GC upon allocating from the GC. For + // instance, some other thread may have started a GC and suspended for the debugger, then this thread allocates from the GC + // and needs to perform a GC, so it waits for the other GC to complete. At that point debugger suspension cannot complete + // because this thread is stuck inside the forbid region. So, this holder should not be used in cases that may allocate from + // the GC. class CrstAndForbidSuspendForDebuggerHolder { private: diff --git a/src/coreclr/vm/fptrstubs.cpp b/src/coreclr/vm/fptrstubs.cpp index cd3dd33d4d863..05c4fedf22d74 100644 --- a/src/coreclr/vm/fptrstubs.cpp +++ b/src/coreclr/vm/fptrstubs.cpp @@ -156,7 +156,7 @@ PCODE FuncPtrStubs::GetFuncPtrStub(MethodDesc * pMD, PrecodeType type) _ASSERTE(pMD->IsVersionableWithVtableSlotBackpatch()); PCODE temporaryEntryPoint = pMD->GetTemporaryEntryPoint(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCPreemp slotBackpatchLockHolder; + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; // Set the funcptr stub's entry point to the current entry point inside the lock and after the funcptr stub is exposed, // to synchronize with backpatching in MethodDesc::BackpatchEntryPointSlots() diff --git a/src/coreclr/vm/gcheaphashtable.h b/src/coreclr/vm/gcheaphashtable.h deleted file mode 100644 index d47ebaf232299..0000000000000 --- a/src/coreclr/vm/gcheaphashtable.h +++ /dev/null @@ -1,141 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -#ifndef GCHEAPHASHTABLE_H -#define GCHEAPHASHTABLE_H - -class GCHeapHashObject; - -template -struct DefaultGCHeapHashTraits -{ - typedef PTRARRAYREF THashArrayType; - static const INT32 s_growth_factor_numerator = 3; - static const INT32 s_growth_factor_denominator = 2; - - static const INT32 s_density_factor_numerator = 3; - static const INT32 s_density_factor_denominator = 4; - - static const INT32 s_densitywithdeletes_factor_numerator = 7; - static const INT32 s_densitywithdeletes_factor_denominator = 8; - - static const INT32 s_minimum_allocation = 7; - - static bool IsNull(PTRARRAYREF arr, INT32 index); - static bool IsDeleted(PTRARRAYREF arr, INT32 index, GCHEAPHASHOBJECTREF gcHeap); -#ifndef DACCESS_COMPILE - static THashArrayType AllocateArray(INT32 size); -#endif - - // Not a part of the traits api, but used to allow derived traits to save on code - static OBJECTREF GetValueAtIndex(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index); - -#ifndef DACCESS_COMPILE - static void CopyValue(THashArrayType srcArray, INT32 indexSrc, THashArrayType destinationArray, INT32 indexDest); - static void DeleteEntry(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index); -#endif // !DACCESS_COMPILE - - template - static void GetElement(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index, TElement& foundElement); - -#ifndef DACCESS_COMPILE - template - static void SetElement(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index, TElement& foundElement); -#endif // !DACCESS_COMPILE -}; - -template -struct GCHeapHashTraitsPointerToPointerList : public DefaultGCHeapHashTraits -{ - static INT32 Hash(PtrTypeKey *pValue); - static INT32 Hash(PTRARRAYREF arr, INT32 index); - static bool DoesEntryMatchKey(PTRARRAYREF arr, INT32 index, PtrTypeKey *pKey); -}; - - -// GCHeapHash is based on the logic of SHash, and utilizes the same basic structure (which allows the key/value -// to be one and the same, or other interesting memory tweaks.) To avoid GC pointer issues, responsibility for allocating -// the underlying arrays and manipulating the entries is entirely extracted to the traits class, and responsibility -// for creation of elements is deferred into the caller of the add function. (See example uses in CrossLoaderAllocatorHash) -// As the GCHeapHash is actually a managed object, but the code for manipulating the hash is written here in native code, -// allocating an instance of this class does not actually allocate a hashtable. Instead, the hashtable is allocated by -// allocating an instance of the GCHeapHash type, and then passing the allocated object into this type's constructor to -// assign the value. This class is designed to be used protected within a GC_PROTECT region. See examples in CrossLoaderAllocatorHash. -template -class GCHeapHash -{ - GCHEAPHASHOBJECTREF m_gcHeapHash; - - typedef typename TRAITS::THashArrayType THashArrayType; - typedef INT32 count_t; - - private: - // Insert into hashtable without growing. GCHEAPHASHOBJECTREF must be GC protected as must be TKey if needed - template - void Insert(TKey *pKey, const TValueSetter &valueSetter); - void CheckGrowth(); - void Grow(); - THashArrayType Grow_OnlyAllocateNewTable(); - - bool IsPrime(count_t number); - count_t NextPrime(count_t number); - - void ReplaceTable(THashArrayType newTable); - - template - count_t CallHash(TKey* pValue) - { - WRAPPER_NO_CONTRACT; - - count_t hash = TRAITS::Hash(pValue); - hash = hash < 0 ? -hash : hash; - if (hash < 0) - return 1; - else - return hash; - } - - count_t CallHash(THashArrayType arr, count_t index) - { - WRAPPER_NO_CONTRACT; - - count_t hash = TRAITS::Hash(arr, index); - hash = hash < 0 ? -hash : hash; - if (hash < 0) - return 1; - else - return hash; - } - - public: - - template - bool VisitAllEntryIndices(TVisitor &visitor); - - template - void Add(TKey *pKey, const TValueSetter &valueSetter); - - // Get the index in the hashtable of the value which matches key, or -1 if there are no matches - template - INT32 GetValueIndex(TKey *pKey); - - template - void GetElement(INT32 index, TElement& foundElement); - - // Use this to update an value within the hashtable directly. - // It is ONLY safe to do if the index already points at an element - // which already exists and has the same key as the newElementValue - template - void SetElement(INT32 index, TElement& newElementValue); - - template - void DeleteEntry(TKey *pKey); - - GCHEAPHASHOBJECTREF GetGCHeapRef() { LIMITED_METHOD_CONTRACT; return m_gcHeapHash; } - - GCHeapHash(GCHEAPHASHOBJECTREF gcHeap) : m_gcHeapHash(gcHeap) {} - GCHeapHash(OBJECTREF gcHeap) : m_gcHeapHash((GCHEAPHASHOBJECTREF)gcHeap) {} - GCHeapHash() : m_gcHeapHash((GCHEAPHASHOBJECTREF)TADDR(NULL)) {} -}; - -#endif // GCHEAPHASHTABLE_H diff --git a/src/coreclr/vm/gcheaphashtable.inl b/src/coreclr/vm/gcheaphashtable.inl deleted file mode 100644 index 6149dcc52dbc5..0000000000000 --- a/src/coreclr/vm/gcheaphashtable.inl +++ /dev/null @@ -1,529 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -#ifdef GCHEAPHASHTABLE_H -#ifndef GCHEAPHASHTABLE_INL -#define GCHEAPHASHTABLE_INL - -template -/*static */bool DefaultGCHeapHashTraits::IsNull(PTRARRAYREF arr, INT32 index) -{ - LIMITED_METHOD_CONTRACT; - - return arr->GetAt(index) == 0; -} - -template -/*static */bool DefaultGCHeapHashTraits::IsDeleted(PTRARRAYREF arr, INT32 index, GCHEAPHASHOBJECTREF gcHeap) -{ - LIMITED_METHOD_CONTRACT; - - if (removeSupported) - { - return gcHeap == arr->GetAt(index); - } - else - { - return false; - } -} - -#ifndef DACCESS_COMPILE -template -/*static*/ typename DefaultGCHeapHashTraits::THashArrayType DefaultGCHeapHashTraits::AllocateArray(INT32 size) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - return (THashArrayType)AllocateObjectArray(size, g_pObjectClass); -} -#endif // !DACCESS_COMPILE - - // Not a part of the traits api, but used to allow derived traits to save on code -template -/*static*/ OBJECTREF DefaultGCHeapHashTraits::GetValueAtIndex(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index) -{ - LIMITED_METHOD_CONTRACT; - - PTRARRAYREF arr((PTRARRAYREF)(*pgcHeap)->GetData()); - - OBJECTREF value = arr->GetAt(index); - - return value; -} - -#ifndef DACCESS_COMPILE -template -/*static*/ void DefaultGCHeapHashTraits::CopyValue(THashArrayType srcArray, INT32 indexSrc, THashArrayType destinationArray, INT32 indexDest) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - if (srcArray == NULL) - COMPlusThrow(kNullReferenceException); - - if ((INT32)srcArray->GetNumComponents() < indexSrc) - COMPlusThrow(kIndexOutOfRangeException); - - OBJECTREF value = srcArray->GetAt(indexSrc); - - if ((INT32)destinationArray->GetNumComponents() < indexDest) - COMPlusThrow(kIndexOutOfRangeException); - - destinationArray->SetAt(indexDest, value); -} -#endif // !DACCESS_COMPILE - -#ifndef DACCESS_COMPILE -template -/*static*/ void DefaultGCHeapHashTraits::DeleteEntry(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - static_assert(removeSupported, "This hash doesn't support remove"); - - PTRARRAYREF arr((PTRARRAYREF)(*pgcHeap)->GetData()); - - if (arr == NULL) - COMPlusThrow(kNullReferenceException); - - if ((INT32)arr->GetNumComponents() < index) - COMPlusThrow(kIndexOutOfRangeException); - - // The deleted sentinel is a self-pointer - arr->SetAt(index, *pgcHeap); -} -#endif // !DACCESS_COMPILE - -template -template -/*static*/ void DefaultGCHeapHashTraits::GetElement(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index, TElement& foundElement) -{ - LIMITED_METHOD_CONTRACT; - - foundElement = (TElement)GetValueAtIndex(pgcHeap, index); -} - -#ifndef DACCESS_COMPILE -template -template -/*static*/ void DefaultGCHeapHashTraits::SetElement(GCHEAPHASHOBJECTREF *pgcHeap, INT32 index, TElement& foundElement) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - PTRARRAYREF arr((PTRARRAYREF)(*pgcHeap)->GetData()); - - if (arr == NULL) - COMPlusThrow(kNullReferenceException); - - if ((INT32)arr->GetNumComponents() < index) - COMPlusThrow(kIndexOutOfRangeException); - - arr->SetAt(index, foundElement); -} -#endif // !DACCESS_COMPILE - -template -/*static */INT32 GCHeapHashTraitsPointerToPointerList::Hash(PtrTypeKey *pValue) -{ - LIMITED_METHOD_CONTRACT; - return (INT32)*pValue; -} - -template -/*static */INT32 GCHeapHashTraitsPointerToPointerList::Hash(PTRARRAYREF arr, INT32 index) -{ - LIMITED_METHOD_CONTRACT; - - UPTRARRAYREF value = (UPTRARRAYREF)arr->GetAt(index); - - return (INT32)*value->GetDirectConstPointerToNonObjectElements(); -} - -template -/*static */bool GCHeapHashTraitsPointerToPointerList::DoesEntryMatchKey(PTRARRAYREF arr, INT32 index, PtrTypeKey *pKey) -{ - LIMITED_METHOD_CONTRACT; - - UPTRARRAYREF value = (UPTRARRAYREF)arr->GetAt(index); - UPTR uptrValue = *value->GetDirectConstPointerToNonObjectElements(); - - return ((UPTR)*pKey) == uptrValue; -} - -template -template -void GCHeapHash::Insert(TKey *pKey, const TValueSetter &valueSetter) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - count_t hash = CallHash(pKey); - count_t tableSize = m_gcHeapHash->GetCapacity(); - count_t index = hash % tableSize; - count_t increment = 0; // delay computation - - while (TRUE) - { - THashArrayType arr((THashArrayType)(m_gcHeapHash)->GetData()); - - bool isNull = TRAITS::IsNull(arr, index); - bool isDeleted = false; - if (!isNull && TRAITS::IsDeleted(arr, index, m_gcHeapHash)) - isDeleted = true; - - if (isNull || isDeleted) - { - if (arr == NULL) - COMPlusThrow(kNullReferenceException); - - if ((INT32)arr->GetNumComponents() < index) - COMPlusThrow(kIndexOutOfRangeException); - - valueSetter(arr, index); - m_gcHeapHash->IncrementCount(isDeleted); - return; - } - - if (increment == 0) - increment = (hash % (tableSize-1)) + 1; - - index += increment; - if (index >= tableSize) - index -= tableSize; - } -} - -template -void GCHeapHash::CheckGrowth() -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - count_t tableMax = (count_t) (m_gcHeapHash->GetCapacity() * TRAITS::s_density_factor_numerator / TRAITS::s_density_factor_denominator); - if (m_gcHeapHash->GetCount() == tableMax) - Grow(); - else - { - tableMax = (count_t) (m_gcHeapHash->GetCapacity() * TRAITS::s_densitywithdeletes_factor_numerator / TRAITS::s_densitywithdeletes_factor_denominator); - if ((m_gcHeapHash->GetCount() + m_gcHeapHash->GetDeletedCount()) >= tableMax) - { - THashArrayType newTable = TRAITS::AllocateArray(m_gcHeapHash->GetCapacity()); - ReplaceTable(newTable); - } - } -} - -template -void GCHeapHash::Grow() -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - THashArrayType newTable = Grow_OnlyAllocateNewTable(); - ReplaceTable(newTable); -} - -template -typename GCHeapHash::THashArrayType GCHeapHash::Grow_OnlyAllocateNewTable() -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - INSTANCE_CHECK; - } - CONTRACTL_END; - - count_t newSize = (count_t) (m_gcHeapHash->GetCount() - * TRAITS::s_growth_factor_numerator / TRAITS::s_growth_factor_denominator - * TRAITS::s_density_factor_denominator / TRAITS::s_density_factor_numerator); - if (newSize < TRAITS::s_minimum_allocation) - newSize = TRAITS::s_minimum_allocation; - - // handle potential overflow - if (newSize < m_gcHeapHash->GetCount()) - ThrowOutOfMemory(); - - return TRAITS::AllocateArray(NextPrime(newSize)); -} - -template -bool GCHeapHash::IsPrime(count_t number) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - // This is a very low-tech check for primality, which doesn't scale very well. - // There are more efficient tests if this proves to be burdensome for larger - // tables. - - if ((number & 1) == 0) - return false; - - count_t factor = 3; - while (factor * factor <= number) - { - if ((number % factor) == 0) - return false; - factor += 2; - } - - return true; -} - -template -typename GCHeapHash::count_t GCHeapHash::NextPrime(count_t number) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - static_assert(sizeof(INT32) == sizeof(g_shash_primes[0]), "the cast below of g_shash_primes[] to INT32 isn't safe due to loss of precision."); - - for (int i = 0; i < (int) (sizeof(g_shash_primes) / sizeof(g_shash_primes[0])); i++) { - if ((INT32)g_shash_primes[i] >= number) - return (INT32)g_shash_primes[i]; - } - - if ((number&1) == 0) - number++; - - while (number != 1) { - if (IsPrime(number)) - return number; - number +=2; - } - - // overflow - ThrowOutOfMemory(); -} - -template -void GCHeapHash::ReplaceTable(THashArrayType newTable) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - GCPROTECT_BEGIN(newTable); - { - count_t newTableSize = (count_t)newTable->GetNumComponents(); - count_t oldTableSize = m_gcHeapHash->GetCapacity(); - - // Move all entries over to the new table - count_t capacity = m_gcHeapHash->GetCapacity(); - - for (count_t index = 0; index < capacity; ++index) - { - THashArrayType arr((THashArrayType)(m_gcHeapHash)->GetData()); - if (!TRAITS::IsNull(arr, index) && !TRAITS::IsDeleted(arr, index, m_gcHeapHash)) - { - count_t hash = CallHash(arr, index); - count_t tableSize = (count_t)newTable->GetNumComponents(); - count_t newIndex = hash % tableSize; - count_t increment = 0; // delay computation - - // Value to copy is in index - while (TRUE) - { - if (TRAITS::IsNull(newTable, newIndex)) - { - arr = (THashArrayType)(m_gcHeapHash)->GetData(); - TRAITS::CopyValue(arr, index, newTable, newIndex); - break; - } - - if (increment == 0) - increment = (hash % (tableSize-1)) + 1; - - newIndex += increment; - if (newIndex >= tableSize) - newIndex -= tableSize; - } - } - } - - m_gcHeapHash->SetTable((BASEARRAYREF)newTable); - - // We've just copied the table to a new table. There are no deleted items as - // we skipped them all, so reset the deleted count to zero - m_gcHeapHash->SetDeletedCountToZero(); - } - GCPROTECT_END(); -} - -template -template -bool GCHeapHash::VisitAllEntryIndices(TVisitor &visitor) -{ - WRAPPER_NO_CONTRACT; - - count_t capacity = m_gcHeapHash->GetCapacity(); - - for (count_t index = 0; index < capacity; ++index) - { - THashArrayType arr((THashArrayType)(m_gcHeapHash)->GetData()); - if (!TRAITS::IsNull(arr, index) && !TRAITS::IsDeleted(arr, index, m_gcHeapHash)) - { - if (!visitor(index)) - return false; - } - } - - return true; -} - -template -template -void GCHeapHash::Add(TKey *pKey, const TValueSetter &valueSetter) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - CheckGrowth(); - Insert(pKey, valueSetter); -} - - // Get the index in the hashtable of the value which matches key, or -1 if there are no matches -template -template -INT32 GCHeapHash::GetValueIndex(TKey *pKey) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - count_t hash = CallHash(pKey); - count_t tableSize = m_gcHeapHash->GetCapacity(); - - // If the table is empty, then there aren't any entries. Just return. - if (m_gcHeapHash->GetCount() == 0) - return -1; - - count_t index = hash % tableSize; - count_t increment = 0; // delay computation - - THashArrayType arr((THashArrayType)(m_gcHeapHash)->GetData()); - - while (m_gcHeapHash->GetCount() != 0) /* the TRAITS::IsDeleted function is allowed to reduce the count */ - { - if (TRAITS::IsNull(arr, index)) - { - return -1; - } - - if (!TRAITS::IsDeleted(arr, index, m_gcHeapHash) && TRAITS::DoesEntryMatchKey(arr, index, pKey)) - { - return index; - } - - if (increment == 0) - increment = (hash % (tableSize-1)) + 1; - - index += increment; - if (index >= tableSize) - index -= tableSize; - } - - return -1; -} - -template -template -void GCHeapHash::GetElement(INT32 index, TElement& foundElement) -{ - WRAPPER_NO_CONTRACT; - - TRAITS::GetElement(&m_gcHeapHash, index, foundElement); -} - - // Use this to update an value within the hashtable directly. - // It is ONLY safe to do if the index already points at an element - // which already exists and has the same key as the newElementValue -template -template -void GCHeapHash::SetElement(INT32 index, TElement& newElementValue) -{ - TRAITS::SetElement(&m_gcHeapHash, index, newElementValue); -} - -template -template -void GCHeapHash::DeleteEntry(TKey *pKey) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - MODE_COOPERATIVE; - } - CONTRACTL_END; - - INT32 index = GetValueIndex(pKey); - if (index != -1) - { - TRAITS::DeleteEntry(&m_gcHeapHash, index); - m_gcHeapHash->DecrementCount(true); - } -} - -#endif // GCHEAPHASHTABLE_INL -#endif // GCHEAPHASHTABLE_H diff --git a/src/coreclr/vm/inlinetracking.cpp b/src/coreclr/vm/inlinetracking.cpp index 86d73ea0a7657..4bd60ab598788 100644 --- a/src/coreclr/vm/inlinetracking.cpp +++ b/src/coreclr/vm/inlinetracking.cpp @@ -432,9 +432,9 @@ Module* PersistentInlineTrackingMapR2R2::GetModuleByIndex(DWORD index) #if !defined(DACCESS_COMPILE) -JITInlineTrackingMap::JITInlineTrackingMap(LoaderAllocator *pAssociatedLoaderAllocator) : - m_mapCrst(CrstJitInlineTrackingMap), - m_map() +CrstStatic JITInlineTrackingMap::s_mapCrst; + +JITInlineTrackingMap::JITInlineTrackingMap(LoaderAllocator *pAssociatedLoaderAllocator) : m_map() { LIMITED_METHOD_CONTRACT; @@ -445,12 +445,12 @@ BOOL JITInlineTrackingMap::InliningExistsDontTakeLock(MethodDesc *inliner, Metho { LIMITED_METHOD_CONTRACT; - _ASSERTE(m_mapCrst.OwnedByCurrentThread()); + _ASSERTE(s_mapCrst.OwnedByCurrentThread()); _ASSERTE(inliner != NULL); _ASSERTE(inlinee != NULL); BOOL found = FALSE; - auto lambda = [&](OBJECTREF obj, MethodDesc *lambdaInlinee, MethodDesc *lambdaInliner) + auto lambda = [&](LoaderAllocator *loaderAllocatorOfInliner, MethodDesc *lambdaInlinee, MethodDesc *lambdaInliner) { _ASSERTE(inlinee == lambdaInlinee); @@ -474,7 +474,7 @@ void JITInlineTrackingMap::AddInlining(MethodDesc *inliner, MethodDesc *inlinee) inlinee = inlinee->LoadTypicalMethodDefinition(); - CrstHolder holder(&m_mapCrst); + CrstHolder holder(&s_mapCrst); AddInliningDontTakeLock(inliner, inlinee); } @@ -482,12 +482,10 @@ void JITInlineTrackingMap::AddInliningDontTakeLock(MethodDesc *inliner, MethodDe { LIMITED_METHOD_CONTRACT; - _ASSERTE(m_mapCrst.OwnedByCurrentThread()); + _ASSERTE(s_mapCrst.OwnedByCurrentThread()); _ASSERTE(inliner != NULL); _ASSERTE(inlinee != NULL); - GCX_COOP(); - if (!InliningExistsDontTakeLock(inliner, inlinee)) { LoaderAllocator *loaderAllocatorOfInliner = inliner->GetLoaderAllocator(); diff --git a/src/coreclr/vm/inlinetracking.h b/src/coreclr/vm/inlinetracking.h index 9c01669db840a..d0d2621dbc767 100644 --- a/src/coreclr/vm/inlinetracking.h +++ b/src/coreclr/vm/inlinetracking.h @@ -370,10 +370,9 @@ class JITInlineTrackingMap } CONTRACTL_END; - GCX_COOP(); - CrstHolder holder(&m_mapCrst); + CrstHolder holder(&s_mapCrst); - auto lambda = [&](OBJECTREF obj, MethodDesc *lambdaInlinee, MethodDesc *lambdaInliner) + auto lambda = [&](LoaderAllocator *loaderAllocatorOfInliner, MethodDesc *lambdaInlinee, MethodDesc *lambdaInliner) { _ASSERTE(lambdaInlinee == inlinee); @@ -383,10 +382,18 @@ class JITInlineTrackingMap m_map.VisitValuesOfKey(inlinee, lambda); } + static void StaticInitialize() + { + WRAPPER_NO_CONTRACT; + s_mapCrst.Init(CrstJitInlineTrackingMap); + } + + static CrstBase *GetMapCrst() { return &s_mapCrst; } + private: BOOL InliningExistsDontTakeLock(MethodDesc *inliner, MethodDesc *inlinee); - Crst m_mapCrst; + static CrstStatic s_mapCrst; InliningInfoTrackerHash m_map; }; diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index 039bb3825837a..0bf3f6da3ee11 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -577,6 +577,10 @@ void LoaderAllocator::GCLoaderAllocators(LoaderAllocator* pOriginalLoaderAllocat pDomainLoaderAllocatorDestroyIterator->ReleaseManagedAssemblyLoadContext(); + // The native objects in dependent handles may refer to the virtual call stub manager's heaps, so clear the dependent + // handles first + pDomainLoaderAllocatorDestroyIterator->CleanupDependentHandlesToNativeObjects(); + // The following code was previously happening on delete ~DomainAssembly->Terminate // We are moving this part here in order to make sure that we can unload a LoaderAllocator // that didn't have a DomainAssembly @@ -1681,6 +1685,11 @@ void AssemblyLoaderAllocator::SetCollectible() void AssemblyLoaderAllocator::Init(AppDomain* pAppDomain) { m_Id.Init(); + + // This is CRST_UNSAFE_ANYMODE to enable registering/unregistering dependent handles to native objects without changing the + // GC mode, in case the caller requires that + m_dependentHandleToNativeObjectSetCrst.Init(CrstLeafLock, CRST_UNSAFE_ANYMODE); + LoaderAllocator::Init((BaseDomain *)pAppDomain); if (IsCollectible()) { @@ -1876,6 +1885,70 @@ void AssemblyLoaderAllocator::CleanupHandles() } } +void AssemblyLoaderAllocator::RegisterDependentHandleToNativeObjectForCleanup(LADependentHandleToNativeObject *dependentHandle) +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + _ASSERTE(dependentHandle != nullptr); + + CrstHolder setLockHolder(&m_dependentHandleToNativeObjectSetCrst); + + _ASSERTE(m_dependentHandleToNativeObjectSet.Lookup(dependentHandle) == NULL); + m_dependentHandleToNativeObjectSet.Add(dependentHandle); +} + +void AssemblyLoaderAllocator::UnregisterDependentHandleToNativeObjectFromCleanup(LADependentHandleToNativeObject *dependentHandle) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + _ASSERTE(dependentHandle != nullptr); + + CrstHolder setLockHolder(&m_dependentHandleToNativeObjectSetCrst); + + _ASSERTE(m_dependentHandleToNativeObjectSet.Lookup(dependentHandle) != NULL); + m_dependentHandleToNativeObjectSet.Remove(dependentHandle); +} + +void AssemblyLoaderAllocator::CleanupDependentHandlesToNativeObjects() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_PREEMPTIVE; + } + CONTRACTL_END; + + // Locks under which dependent handles may be used must all be taken here to ensure that a thread using a dependent handle + // would either it cleared, or that the dependent object remains valid under those locks. In particular, any locks used to + // synchronize uses of CrossLoaderAllocatorHash instances must also be taken here. + CrstHolder jitInlineTrackingMapLockHolder(JITInlineTrackingMap::GetMapCrst()); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; + + CrstHolder setLockHolder(&m_dependentHandleToNativeObjectSetCrst); + + for (DependentHandleToNativeObjectSet::Iterator it = m_dependentHandleToNativeObjectSet.Begin(), + itEnd = m_dependentHandleToNativeObjectSet.End(); + it != itEnd; + ++it) + { + LADependentHandleToNativeObject *dependentHandle = *it; + dependentHandle->Clear(); + } +} + void LoaderAllocator::RegisterFailedTypeInitForCleanup(ListLockEntry *pListLockEntry) { CONTRACTL diff --git a/src/coreclr/vm/loaderallocator.hpp b/src/coreclr/vm/loaderallocator.hpp index 846ec6346d418..36213d641e2e4 100644 --- a/src/coreclr/vm/loaderallocator.hpp +++ b/src/coreclr/vm/loaderallocator.hpp @@ -673,6 +673,12 @@ class LoaderAllocator PTR_OnStackReplacementManager GetOnStackReplacementManager(); #endif // FEATURE_ON_STACK_REPLACEMENT +#ifndef DACCESS_COMPILE +public: + virtual void RegisterDependentHandleToNativeObjectForCleanup(LADependentHandleToNativeObject *dependentHandle) {}; + virtual void UnregisterDependentHandleToNativeObjectFromCleanup(LADependentHandleToNativeObject *dependentHandle) {}; + virtual void CleanupDependentHandlesToNativeObjects() {}; +#endif }; // class LoaderAllocator typedef VPTR(LoaderAllocator) PTR_LoaderAllocator; @@ -766,6 +772,20 @@ class AssemblyLoaderAllocator : public LoaderAllocator #if !defined(DACCESS_COMPILE) CustomAssemblyBinder* m_binderToRelease; #endif + +private: + class DependentHandleToNativeObjectHashTraits : public PtrSetSHashTraits {}; + typedef SHash DependentHandleToNativeObjectSet; + + CrstExplicitInit m_dependentHandleToNativeObjectSetCrst; + DependentHandleToNativeObjectSet m_dependentHandleToNativeObjectSet; + +#ifndef DACCESS_COMPILE +public: + virtual void RegisterDependentHandleToNativeObjectForCleanup(LADependentHandleToNativeObject *dependentHandle) override; + virtual void UnregisterDependentHandleToNativeObjectFromCleanup(LADependentHandleToNativeObject *dependentHandle) override; + virtual void CleanupDependentHandlesToNativeObjects() override; +#endif }; typedef VPTR(AssemblyLoaderAllocator) PTR_AssemblyLoaderAllocator; diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index 6449386918767..08e4a3a278da6 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -3110,7 +3110,7 @@ void MethodDesc::RecordAndBackpatchEntryPointSlot( GCX_PREEMP(); LoaderAllocator *mdLoaderAllocator = GetLoaderAllocator(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder; + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; RecordAndBackpatchEntryPointSlot_Locked( mdLoaderAllocator, diff --git a/src/coreclr/vm/methoddescbackpatchinfo.cpp b/src/coreclr/vm/methoddescbackpatchinfo.cpp index 9a63100726776..2a8d438048ab7 100644 --- a/src/coreclr/vm/methoddescbackpatchinfo.cpp +++ b/src/coreclr/vm/methoddescbackpatchinfo.cpp @@ -77,9 +77,7 @@ void MethodDescBackpatchInfoTracker::Backpatch_Locked(MethodDesc *pMethodDesc, P _ASSERTE(IsLockOwnedByCurrentThread()); _ASSERTE(pMethodDesc != nullptr); - GCX_COOP(); - - auto lambda = [&entryPoint](OBJECTREF obj, MethodDesc *pMethodDesc, UINT_PTR slotData) + auto lambda = [&entryPoint](LoaderAllocator *pLoaderAllocatorOfSlot, MethodDesc *pMethodDesc, UINT_PTR slotData) { TADDR slot; @@ -101,8 +99,6 @@ void MethodDescBackpatchInfoTracker::AddSlotAndPatch_Locked(MethodDesc *pMethodD _ASSERTE(pMethodDesc != nullptr); _ASSERTE(pMethodDesc->MayHaveEntryPointSlotsToBackpatch()); - GCX_COOP(); - UINT_PTR slotData; slotData = EntryPointSlots::ConvertSlotAndTypePairToUINT_PTR(slot, slotType); diff --git a/src/coreclr/vm/methoddescbackpatchinfo.h b/src/coreclr/vm/methoddescbackpatchinfo.h index b94882bbf7271..d9c706cac6530 100644 --- a/src/coreclr/vm/methoddescbackpatchinfo.h +++ b/src/coreclr/vm/methoddescbackpatchinfo.h @@ -95,42 +95,23 @@ class MethodDescBackpatchInfoTracker static bool IsLockOwnedByCurrentThread(); #endif -public: - // To be used when the thread will remain in preemptive GC mode while holding the lock - class ConditionalLockHolderForGCPreemp : private CrstHolderWithState - { - public: - ConditionalLockHolderForGCPreemp(bool acquireLock = true) - : CrstHolderWithState( -#ifndef DACCESS_COMPILE - acquireLock ? &s_lock : nullptr -#else - nullptr -#endif - ) - { - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_PREEMPTIVE; - } - CONTRACTL_END; - } - - DISABLE_COPY(ConditionalLockHolderForGCPreemp); - }; - #ifndef DACCESS_COMPILE public: - // To be used when the thread may enter cooperative GC mode while holding the lock. The thread enters a - // forbid-suspend-for-debugger region along with acquiring the lock, such that it would not suspend for the debugger while - // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock. - class ConditionalLockHolderForGCCoop : private CrstAndForbidSuspendForDebuggerHolder + class ConditionalLockHolder : private CrstHolderWithState { + #ifdef ENABLE_CONTRACTS_IMPL + private: + GCNoTrigger m_gcNoTriggerHolder; + #endif + public: - ConditionalLockHolderForGCCoop(bool acquireLock = true) - : CrstAndForbidSuspendForDebuggerHolder(acquireLock ? &s_lock : nullptr) + // Acquire the lock and put the thread in a GC_NOTRIGGER scope to ensure that this lock cannot be held while the thread + // suspends for the debugger, otherwise FuncEvals that run may deadlock on acquiring this lock + ConditionalLockHolder(bool acquireLock = true) + : CrstHolderWithState(acquireLock ? &s_lock : nullptr) + #ifdef ENABLE_CONTRACTS_IMPL + , m_gcNoTriggerHolder(acquireLock, __FUNCTION__, __FILE__, __LINE__) + #endif { CONTRACTL { @@ -141,7 +122,7 @@ class MethodDescBackpatchInfoTracker CONTRACTL_END; } - DISABLE_COPY(ConditionalLockHolderForGCCoop); + DISABLE_COPY(ConditionalLockHolder); }; #endif diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 8071392c9b4cc..34d5863d73f71 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1952,20 +1952,3 @@ void ExceptionObject::GetStackTrace(StackTraceArray & stackTrace, PTRARRAYREF * #endif // !defined(DACCESS_COMPILE) } - -bool LAHashDependentHashTrackerObject::IsLoaderAllocatorLive() -{ - return (ObjectFromHandle(_dependentHandle) != NULL); -} - -void LAHashDependentHashTrackerObject::GetDependentAndLoaderAllocator(OBJECTREF *pLoaderAllocatorRef, GCHEAPHASHOBJECTREF *pGCHeapHash) -{ - OBJECTREF primary = ObjectFromHandle(_dependentHandle); - if (pLoaderAllocatorRef != NULL) - *pLoaderAllocatorRef = primary; - - IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager(); - // Secondary is tracked only if primary is non-null - if (pGCHeapHash != NULL) - *pGCHeapHash = (GCHEAPHASHOBJECTREF)(OBJECTREF)((primary != NULL) ? mgr->GetDependentHandleSecondary(_dependentHandle) : NULL); -} diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index bf3f9c0bc17bc..e64ece892585b 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2700,133 +2700,4 @@ typedef REF EXCEPTIONREF; typedef PTR_ExceptionObject EXCEPTIONREF; #endif // USE_CHECKED_OBJECTREFS -class GCHeapHashObject : public Object -{ -#ifdef DACCESS_COMPILE - friend class ClrDataAccess; -#endif - friend class GCHeap; - friend class JIT_TrialAlloc; - friend class CheckAsmOffsets; - friend class COMString; - friend class CoreLibBinder; - - private: - BASEARRAYREF _data; - INT32 _count; - INT32 _deletedCount; - - public: - INT32 GetCount() { LIMITED_METHOD_CONTRACT; return _count; } - void IncrementCount(bool replacingDeletedItem) - { - LIMITED_METHOD_CONTRACT; - ++_count; - if (replacingDeletedItem) - --_deletedCount; - } - - void DecrementCount(bool deletingItem) - { - LIMITED_METHOD_CONTRACT; - --_count; - if (deletingItem) - ++_deletedCount; - } - INT32 GetDeletedCount() { LIMITED_METHOD_CONTRACT; return _deletedCount; } - void SetDeletedCountToZero() { LIMITED_METHOD_CONTRACT; _deletedCount = 0; } - INT32 GetCapacity() { LIMITED_METHOD_CONTRACT; if (_data == NULL) return 0; else return (_data->GetNumComponents()); } - BASEARRAYREF GetData() { LIMITED_METHOD_CONTRACT; return _data; } - - void SetTable(BASEARRAYREF data) - { - STATIC_CONTRACT_NOTHROW; - STATIC_CONTRACT_GC_NOTRIGGER; - STATIC_CONTRACT_MODE_COOPERATIVE; - - SetObjectReference((OBJECTREF*)&_data, (OBJECTREF)data); - } - - protected: - GCHeapHashObject() {LIMITED_METHOD_CONTRACT; } - ~GCHeapHashObject() {LIMITED_METHOD_CONTRACT; } -}; - -typedef DPTR(GCHeapHashObject) PTR_GCHeapHashObject; - -#ifdef USE_CHECKED_OBJECTREFS -typedef REF GCHEAPHASHOBJECTREF; -#else // USE_CHECKED_OBJECTREFS -typedef PTR_GCHeapHashObject GCHEAPHASHOBJECTREF; -#endif // USE_CHECKED_OBJECTREFS - -class LAHashDependentHashTrackerObject : public Object -{ -#ifdef DACCESS_COMPILE - friend class ClrDataAccess; -#endif - friend class CheckAsmOffsets; - friend class CoreLibBinder; - - private: - OBJECTHANDLE _dependentHandle; - LoaderAllocator* _loaderAllocator; - - public: - bool IsLoaderAllocatorLive(); - bool IsTrackerFor(LoaderAllocator *pLoaderAllocator) - { - if (pLoaderAllocator != _loaderAllocator) - return false; - - return IsLoaderAllocatorLive(); - } - - void GetDependentAndLoaderAllocator(OBJECTREF *pLoaderAllocatorRef, GCHEAPHASHOBJECTREF *pGCHeapHash); - - // Be careful with this. This isn't safe to use unless something is keeping the LoaderAllocator live, or there is no intention to dereference this pointer - LoaderAllocator* GetLoaderAllocatorUnsafe() - { - return _loaderAllocator; - } - - void Init(OBJECTHANDLE dependentHandle, LoaderAllocator* loaderAllocator) - { - LIMITED_METHOD_CONTRACT; - _dependentHandle = dependentHandle; - _loaderAllocator = loaderAllocator; - } -}; - -class LAHashKeyToTrackersObject : public Object -{ -#ifdef DACCESS_COMPILE - friend class ClrDataAccess; -#endif - friend class CheckAsmOffsets; - friend class CoreLibBinder; - - public: - // _trackerOrTrackerSet is either a reference to a LAHashDependentHashTracker, or to a GCHeapHash of LAHashDependentHashTracker objects. - OBJECTREF _trackerOrTrackerSet; - // _laLocalKeyValueStore holds an object that represents a Key value (which must always be valid for the lifetime of the - // CrossLoaderAllocatorHeapHash, and the values which must also be valid for that entire lifetime. When a value might - // have a shorter lifetime it is accessed through the _trackerOrTrackerSet variable, which allows access to hashtables which - // are associated with that remote loaderallocator through a dependent handle, so that lifetime can be managed. - OBJECTREF _laLocalKeyValueStore; -}; - -typedef DPTR(LAHashDependentHashTrackerObject) PTR_LAHashDependentHashTrackerObject; -typedef DPTR(LAHashKeyToTrackersObject) PTR_LAHashKeyToTrackersObject; - - -#ifdef USE_CHECKED_OBJECTREFS -typedef REF LAHASHDEPENDENTHASHTRACKERREF; -typedef REF LAHASHKEYTOTRACKERSREF; -#else // USE_CHECKED_OBJECTREFS -typedef PTR_LAHashDependentHashTrackerObject LAHASHDEPENDENTHASHTRACKERREF; -typedef PTR_LAHashKeyToTrackersObject LAHASHKEYTOTRACKERSREF; -#endif // USE_CHECKED_OBJECTREFS - - #endif // _OBJECT_H_ diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 9d75ccc075c2d..df1178befd6e2 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -90,8 +90,7 @@ PCODE MethodDesc::DoBackpatch(MethodTable * pMT, MethodTable *pDispatchingMT, BO // Only take the lock if the method is versionable with vtable slot backpatch, for recording slots and synchronizing with // backpatching slots - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder( - isVersionableWithVtableSlotBackpatch); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder(isVersionableWithVtableSlotBackpatch); // Get the method entry point inside the lock above to synchronize with backpatching in // MethodDesc::BackpatchEntryPointSlots() diff --git a/src/coreclr/vm/rejit.cpp b/src/coreclr/vm/rejit.cpp index 80f05489ad3b2..8807b0934e285 100644 --- a/src/coreclr/vm/rejit.cpp +++ b/src/coreclr/vm/rejit.cpp @@ -643,8 +643,6 @@ HRESULT ReJitManager::UpdateActiveILVersions( SHash::Iterator endIter = mgrToCodeActivationBatch.End(); { - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder; - for (SHash::Iterator iter = beginIter; iter != endIter; iter++) { CodeActivationBatch * pCodeActivationBatch = *iter; diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index d37e6d32632c3..7b76ebf0c48ad 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2113,9 +2113,18 @@ void Thread::RareDisablePreemptiveGC() // Note IsGCInProgress is also true for say Pause (anywhere SuspendEE happens) and GCThread is the // thread that did the Pause. While in Pause if another thread attempts Rev/Pinvoke it should get inside the following and // block until resume - if ((GCHeapUtilities::IsGCInProgress() && (this != ThreadSuspend::GetSuspensionThread())) || - ((m_State & TS_DebugSuspendPending) && !IsInForbidSuspendForDebuggerRegion()) || - (m_State & TS_StackCrawlNeeded)) + if (( + (GCHeapUtilities::IsGCInProgress() && (this != ThreadSuspend::GetSuspensionThread())) || + (m_State & TS_DebugSuspendPending) || + (m_State & TS_StackCrawlNeeded) + ) && + + // When in a forbid-suspend-for-debugger region and the thread waits for an in-progress GC, it would become stuck in the + // forbid region until the GC is complete. In the meantime, the debugger may need to have the runtime suspend for the + // debugger first, perhaps in a GC-unsafe point. This thread would not be able to suspend for the debugger and would + // cause a deadlock during debugger suspension. When in the forbid region, allow disabling preemptive GC and bypass the + // above heuristics. + !IsInForbidSuspendForDebuggerRegion()) { STRESS_LOG1(LF_SYNC, LL_INFO1000, "RareDisablePreemptiveGC: entering. Thread state = %x\n", m_State.Load()); @@ -2181,9 +2190,10 @@ void Thread::RareDisablePreemptiveGC() // However, it is possible for the current thread to become the GC // thread while in this loop. This happens if you use the COM+ // debugger to suspend this thread and then release it. + _ASSERTE(!IsInForbidSuspendForDebuggerRegion()); if (! ((GCHeapUtilities::IsGCInProgress() && (this != ThreadSuspend::GetSuspensionThread())) || - ((m_State & TS_DebugSuspendPending) && !IsInForbidSuspendForDebuggerRegion()) || - (m_State & TS_StackCrawlNeeded)) ) + (m_State & TS_DebugSuspendPending) || + (m_State & TS_StackCrawlNeeded)) ) { break; } diff --git a/src/coreclr/vm/tieredcompilation.cpp b/src/coreclr/vm/tieredcompilation.cpp index 9adb2ca09785e..9611c33356a93 100644 --- a/src/coreclr/vm/tieredcompilation.cpp +++ b/src/coreclr/vm/tieredcompilation.cpp @@ -579,13 +579,7 @@ bool TieredCompilationManager::TryDeactivateTieringDelay() COUNT_T methodCount = methodsPendingCounting->GetCount(); CodeVersionManager *codeVersionManager = GetAppDomain()->GetCodeVersionManager(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder; - - // Backpatching entry point slots requires cooperative GC mode, see - // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that - // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering - // must be used here to prevent deadlock. - GCX_COOP(); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; CodeVersionManager::LockHolder codeVersioningLockHolder; for (COUNT_T i = 0; i < methodCount; ++i) @@ -946,14 +940,7 @@ void TieredCompilationManager::ActivateCodeVersion(NativeCodeVersion nativeCodeV HRESULT hr = S_OK; { bool mayHaveEntryPointSlotsToBackpatch = pMethod->MayHaveEntryPointSlotsToBackpatch(); - MethodDescBackpatchInfoTracker::ConditionalLockHolderForGCCoop slotBackpatchLockHolder( - mayHaveEntryPointSlotsToBackpatch); - - // Backpatching entry point slots requires cooperative GC mode, see - // MethodDescBackpatchInfoTracker::Backpatch_Locked(). The code version manager's table lock is an unsafe lock that - // may be taken in any GC mode. The lock is taken in cooperative GC mode on some other paths, so the same ordering - // must be used here to prevent deadlock. - GCX_MAYBE_COOP(mayHaveEntryPointSlotsToBackpatch); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder(mayHaveEntryPointSlotsToBackpatch); CodeVersionManager::LockHolder codeVersioningLockHolder; // As long as we are exclusively using any non-JumpStamp publishing for tiered compilation From 7a769b25cd2b63e1657a64b3ee8336576a483dc4 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 25 Mar 2022 14:31:47 -0700 Subject: [PATCH 2/7] Fix build on Unixes --- src/coreclr/inc/shash.h | 8 ++++---- src/coreclr/vm/crossloaderallocatorhash.inl | 15 +++++++++------ src/coreclr/vm/loaderallocator.hpp | 6 +++--- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 4075d5d9bd28f..ce137ec5cc3a1 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -504,9 +504,9 @@ class EMPTY_BASES_DECL SHash : public TRAITS if (!TRAITS::IsDeleted(this->m_table[this->m_index])) { - if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(m_table[m_index])) + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(this->m_table[this->m_index])) { - m_hash->RemoveElement(m_table, m_tableSize, &m_table[m_index]); + this->m_hash->RemoveElement(this->m_table, this->m_tableSize, &this->m_table[this->m_index]); } else if (TRAITS::Equals(m_key, TRAITS::GetKey(this->m_table[this->m_index]))) { @@ -539,9 +539,9 @@ class EMPTY_BASES_DECL SHash : public TRAITS continue; } - if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(m_table[m_index])) + if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(this->m_table[this->m_index])) { - m_hash->RemoveElement(m_table, m_tableSize, &m_table[m_index]); + this->m_hash->RemoveElement(this->m_table, this->m_tableSize, &this->m_table[this->m_index]); continue; } diff --git a/src/coreclr/vm/crossloaderallocatorhash.inl b/src/coreclr/vm/crossloaderallocatorhash.inl index d9527c39bf852..85c7d4b07a53c 100644 --- a/src/coreclr/vm/crossloaderallocatorhash.inl +++ b/src/coreclr/vm/crossloaderallocatorhash.inl @@ -584,7 +584,7 @@ bool CrossLoaderAllocatorHash::VisitValuesOfKey(TKey key, Visitor &visit { LAHashDependentHashTrackerHash *dependentTrackerHash = static_cast(hashKeyToTrackers->_trackerOrTrackerSet)->GetTrackerSet(); - for (LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash->Begin(), + for (typename LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash->Begin(), itEnd = dependentTrackerHash->End(); it != itEnd; ++it) @@ -607,7 +607,7 @@ bool CrossLoaderAllocatorHash::VisitAllKeyValuePairs(Visitor &visitor) WRAPPER_NO_CONTRACT; KeyToValuesHash &keyToTrackersHash = m_keyToDependentTrackersHash; - for (KeyToValuesHash::Iterator it = keyToTrackersHash.Begin(), itEnd = keyToTrackersHash.End(); it != itEnd; ++it) + for (typename KeyToValuesHash::Iterator it = keyToTrackersHash.Begin(), itEnd = keyToTrackersHash.End(); it != itEnd; ++it) { KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *it; if (!VisitKeyToTrackerAllLALocalEntries(hashKeyEntry, visitor)) @@ -617,7 +617,8 @@ bool CrossLoaderAllocatorHash::VisitAllKeyValuePairs(Visitor &visitor) } LAHashDependentHashTrackerHash &dependentTrackerHash = m_loaderAllocatorToDependentTrackerHash; - for (LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash.Begin(), itEnd = dependentTrackerHash.End(); + for (typename LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash.Begin(), + itEnd = dependentTrackerHash.End(); it != itEnd; ++it) { @@ -685,7 +686,7 @@ void CrossLoaderAllocatorHash::RemoveAll(TKey key) { LAHashDependentHashTrackerHash *dependentTrackerHash = static_cast(hashKeyToTrackers->_trackerOrTrackerSet)->GetTrackerSet(); - for (LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash->Begin(), + for (typename LAHashDependentHashTrackerHash::Iterator it = dependentTrackerHash->Begin(), itEnd = dependentTrackerHash->End(); it != itEnd; ++it) @@ -747,7 +748,9 @@ template if (keyToValuesHash != NULL) { LoaderAllocator *loaderAllocator = tracker->GetLoaderAllocatorUnsafe(); - for (KeyToValuesHash::Iterator it = keyToValuesHash->Begin(), itEnd = keyToValuesHash->End(); it != itEnd; ++it) + for (typename KeyToValuesHash::Iterator it = keyToValuesHash->Begin(), itEnd = keyToValuesHash->End(); + it != itEnd; + ++it) { KeyValueStoreOrLAHashKeyToTrackers *hashKeyEntry = *it; _ASSERTE(!hashKeyEntry->IsLAHashKeyToTrackers()); @@ -821,7 +824,7 @@ CrossLoaderAllocatorHash::GetDependentTrackerForLoaderAllocator(LoaderAl } NewHolder laDependentKeyToValuesHashHolder = new LADependentKeyToValuesHash(); - LAHashDependentHashTracker::NewTrackerHolder dependentTrackerHolder = + typename LAHashDependentHashTracker::NewTrackerHolder dependentTrackerHolder = new LAHashDependentHashTracker(pLoaderAllocator, laDependentKeyToValuesHashHolder); laDependentKeyToValuesHashHolder.SuppressRelease(); diff --git a/src/coreclr/vm/loaderallocator.hpp b/src/coreclr/vm/loaderallocator.hpp index 36213d641e2e4..90311d3a01ca1 100644 --- a/src/coreclr/vm/loaderallocator.hpp +++ b/src/coreclr/vm/loaderallocator.hpp @@ -782,9 +782,9 @@ class AssemblyLoaderAllocator : public LoaderAllocator #ifndef DACCESS_COMPILE public: - virtual void RegisterDependentHandleToNativeObjectForCleanup(LADependentHandleToNativeObject *dependentHandle) override; - virtual void UnregisterDependentHandleToNativeObjectFromCleanup(LADependentHandleToNativeObject *dependentHandle) override; - virtual void CleanupDependentHandlesToNativeObjects() override; + virtual void RegisterDependentHandleToNativeObjectForCleanup(LADependentHandleToNativeObject *dependentHandle); + virtual void UnregisterDependentHandleToNativeObjectFromCleanup(LADependentHandleToNativeObject *dependentHandle); + virtual void CleanupDependentHandlesToNativeObjects(); #endif }; From 0196f539e9b80632160303aa48ff05b144ea0afe Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 25 Mar 2022 18:28:20 -0700 Subject: [PATCH 3/7] Fix contract --- src/coreclr/vm/codeversion.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index e0cc64a0f42c6..52c026b7da358 100644 --- a/src/coreclr/vm/codeversion.cpp +++ b/src/coreclr/vm/codeversion.cpp @@ -1475,7 +1475,7 @@ HRESULT CodeVersionManager::SetActiveILCodeVersions(ILCodeVersion* pActiveVersio CONTRACTL { NOTHROW; - GC_TRIGGERS; + GC_NOTRIGGER; MODE_PREEMPTIVE; CAN_TAKE_LOCK; PRECONDITION(CheckPointer(pActiveVersions)); @@ -1922,7 +1922,7 @@ HRESULT CodeVersionManager::EnumerateClosedMethodDescs( CONTRACTL { NOTHROW; - GC_TRIGGERS; + GC_NOTRIGGER; MODE_PREEMPTIVE; CAN_TAKE_LOCK; PRECONDITION(CheckPointer(pMD, NULL_OK)); From edffc0c72674dca3cbb446a0143204c8ab7ec390 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 1 Apr 2022 10:03:46 -0700 Subject: [PATCH 4/7] Address feedback --- src/coreclr/inc/shash.h | 11 ++++++----- src/coreclr/inc/shash.inl | 19 +++++-------------- src/coreclr/vm/loaderallocator.cpp | 4 ++-- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index ce137ec5cc3a1..5f9e26b0754d3 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -76,10 +76,9 @@ // // static void OnDestructPerEntryCleanupAction(ELEMENT& e) Called on every element when in hashtable destructor. // s_DestructPerEntryCleanupAction must be set to true if implemented. -// static void OnRemovePerEntryCleanupAction(ELEMENT& e) Called when an element is removed from the hashtable. -// s_RemovePerEntryCleanupAction must be set to true if implemented. When elements -// are removed in the destructor, OnDestructPerEntryCleanupAction() is called -// instead if s_DestructPerEntryCleanupAction is true. +// static void OnRemovePerEntryCleanupAction(ELEMENT& e) Called when an element is removed from the hashtable, including when +// the hashtable is destructed. s_RemovePerEntryCleanupAction must be set to true +// if implemented. // // s_growth_factor_numerator // s_growth_factor_denominator Factor to grow allocation (numerator/denominator). @@ -109,6 +108,8 @@ // // s_DestructPerEntryCleanupAction Set to true if OnDestructPerEntryCleanupAction has non-empty implementation. // s_RemovePerEntryCleanupAction Set to true if OnRemovePerEntryCleanupAction has non-empty implementation. +// Only one of s_DestructPerEntryCleanupAction and s_RemovePerEntryCleanupAction +// may be set to true. // // DefaultHashTraits provides defaults for seldomly customized values in traits classes. @@ -323,7 +324,7 @@ class EMPTY_BASES_DECL SHash : public TRAITS // it is perfectly fine for the element to be a duplicate - if so it // is added an additional time. Returns TRUE if a new empty spot was used; // FALSE if an existing deleted slot. - BOOL Add(element_t *table, count_t tableSize, const element_t &element) const; + BOOL Add(element_t *table, count_t tableSize, const element_t &element); // Utility function to add a new element to the hash table, if no element with the same key // is already there. Otherwise, it will replace the existing element. This has the effect of diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index aa09d14d4a108..5bf0da026cbc2 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -26,6 +26,7 @@ SHash::SHash() #endif static_assert_no_msg(TRAITS::s_supports_remove || !TRAITS::s_supports_autoremove); + static_assert_no_msg(!TRAITS::s_DestructPerEntryCleanupAction || !TRAITS::s_RemovePerEntryCleanupAction); } template @@ -712,7 +713,7 @@ const typename SHash::element_t * SHash::Lookup(PTR_element_t ta } template -BOOL SHash::Add(element_t * table, count_t tableSize, const element_t & element) const +BOOL SHash::Add(element_t * table, count_t tableSize, const element_t & element) { CONTRACT(BOOL) { @@ -746,7 +747,7 @@ BOOL SHash::Add(element_t * table, count_t tableSize, const element_t & if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) { - const_cast *>(this)->RemoveElement(table, tableSize, ¤t); + RemoveElement(table, tableSize, ¤t); table[index] = element; RETURN FALSE; } @@ -835,21 +836,11 @@ void SHash::Remove(element_t *table, count_t tableSize, key_t key) if (!TRAITS::IsDeleted(current)) { - if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) + if ((TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) || + TRAITS::Equals(key, TRAITS::GetKey(current))) { RemoveElement(table, tableSize, ¤t); } - else if (TRAITS::Equals(key, TRAITS::GetKey(current))) - { - if (TRAITS::s_RemovePerEntryCleanupAction) - { - TRAITS::OnRemovePerEntryCleanupAction(current); - } - - table[index] = TRAITS::Deleted(); - m_tableCount--; - RETURN; - } } if (increment == 0) diff --git a/src/coreclr/vm/loaderallocator.cpp b/src/coreclr/vm/loaderallocator.cpp index 0bf3f6da3ee11..cbfda12770007 100644 --- a/src/coreclr/vm/loaderallocator.cpp +++ b/src/coreclr/vm/loaderallocator.cpp @@ -1932,8 +1932,8 @@ void AssemblyLoaderAllocator::CleanupDependentHandlesToNativeObjects() CONTRACTL_END; // Locks under which dependent handles may be used must all be taken here to ensure that a thread using a dependent handle - // would either it cleared, or that the dependent object remains valid under those locks. In particular, any locks used to - // synchronize uses of CrossLoaderAllocatorHash instances must also be taken here. + // would either observe it cleared, or that the dependent object remains valid under those locks. In particular, any locks + // used to synchronize uses of CrossLoaderAllocatorHash instances must also be taken here. CrstHolder jitInlineTrackingMapLockHolder(JITInlineTrackingMap::GetMapCrst()); MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder; From 170fdd0103b5727c2842ede81ebc75d6d85d9b3a Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 1 Apr 2022 13:23:16 -0700 Subject: [PATCH 5/7] Revert changes relevant to forbid-suspend-for-debugger scopes --- src/coreclr/vm/appdomain.hpp | 3 +-- src/coreclr/vm/crst.h | 10 ---------- src/coreclr/vm/threadsuspend.cpp | 20 +++++--------------- 3 files changed, 6 insertions(+), 27 deletions(-) diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp index 37d7dc5cc60c8..fbe728ff7e1c0 100644 --- a/src/coreclr/vm/appdomain.hpp +++ b/src/coreclr/vm/appdomain.hpp @@ -1177,8 +1177,7 @@ class BaseDomain // To be used when the thread may enter cooperative GC mode while holding the lock. The thread enters a // forbid-suspend-for-debugger region along with acquiring the lock, such that it would not suspend for the debugger while - // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock. This holder cannot - // be used when there may be GC allocation inside its scope (see the base class for more information). + // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock. class DomainCacheCrstHolderForGCCoop : private CrstAndForbidSuspendForDebuggerHolder { public: diff --git a/src/coreclr/vm/crst.h b/src/coreclr/vm/crst.h index b67b6c2708621..a8f2065703d6e 100644 --- a/src/coreclr/vm/crst.h +++ b/src/coreclr/vm/crst.h @@ -395,16 +395,6 @@ friend class Crst; // This just exists to convert legacy OS Critical Section patterns over to holders. typedef DacHolder UnsafeCrstInverseHolder; - // This holder acquires the lock in preemptive GC mode and puts the thread in a forbid-suspend-for-debugger region. It is - // used when cooperative GC mode would be entered while the lock is held, which can normally suspend for the debugger and - // cause FuncEvals to deadlock upon trying to acquire the lock. Using this holder prevents the thread from suspending for - // the debugger upon GC mode switches inside the forbid region. - // - // However, the forbid region does not prevent the thread from waiting for a pending GC upon allocating from the GC. For - // instance, some other thread may have started a GC and suspended for the debugger, then this thread allocates from the GC - // and needs to perform a GC, so it waits for the other GC to complete. At that point debugger suspension cannot complete - // because this thread is stuck inside the forbid region. So, this holder should not be used in cases that may allocate from - // the GC. class CrstAndForbidSuspendForDebuggerHolder { private: diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 7b76ebf0c48ad..d37e6d32632c3 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2113,18 +2113,9 @@ void Thread::RareDisablePreemptiveGC() // Note IsGCInProgress is also true for say Pause (anywhere SuspendEE happens) and GCThread is the // thread that did the Pause. While in Pause if another thread attempts Rev/Pinvoke it should get inside the following and // block until resume - if (( - (GCHeapUtilities::IsGCInProgress() && (this != ThreadSuspend::GetSuspensionThread())) || - (m_State & TS_DebugSuspendPending) || - (m_State & TS_StackCrawlNeeded) - ) && - - // When in a forbid-suspend-for-debugger region and the thread waits for an in-progress GC, it would become stuck in the - // forbid region until the GC is complete. In the meantime, the debugger may need to have the runtime suspend for the - // debugger first, perhaps in a GC-unsafe point. This thread would not be able to suspend for the debugger and would - // cause a deadlock during debugger suspension. When in the forbid region, allow disabling preemptive GC and bypass the - // above heuristics. - !IsInForbidSuspendForDebuggerRegion()) + if ((GCHeapUtilities::IsGCInProgress() && (this != ThreadSuspend::GetSuspensionThread())) || + ((m_State & TS_DebugSuspendPending) && !IsInForbidSuspendForDebuggerRegion()) || + (m_State & TS_StackCrawlNeeded)) { STRESS_LOG1(LF_SYNC, LL_INFO1000, "RareDisablePreemptiveGC: entering. Thread state = %x\n", m_State.Load()); @@ -2190,10 +2181,9 @@ void Thread::RareDisablePreemptiveGC() // However, it is possible for the current thread to become the GC // thread while in this loop. This happens if you use the COM+ // debugger to suspend this thread and then release it. - _ASSERTE(!IsInForbidSuspendForDebuggerRegion()); if (! ((GCHeapUtilities::IsGCInProgress() && (this != ThreadSuspend::GetSuspensionThread())) || - (m_State & TS_DebugSuspendPending) || - (m_State & TS_StackCrawlNeeded)) ) + ((m_State & TS_DebugSuspendPending) && !IsInForbidSuspendForDebuggerRegion()) || + (m_State & TS_StackCrawlNeeded)) ) { break; } From b133366da2d433886668bae5e3ad6422b80fc886 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 6 Apr 2022 12:08:19 -0700 Subject: [PATCH 6/7] Fix a couple of bugs --- src/coreclr/inc/shash.h | 2 +- src/coreclr/inc/shash.inl | 5 ++--- src/coreclr/vm/crossloaderallocatorhash.inl | 7 +++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coreclr/inc/shash.h b/src/coreclr/inc/shash.h index 5f9e26b0754d3..c00fc72c17e64 100644 --- a/src/coreclr/inc/shash.h +++ b/src/coreclr/inc/shash.h @@ -201,7 +201,7 @@ class EMPTY_BASES_DECL SHash : public TRAITS // Pointer-based flavor to replace an existing element (allows efficient access to tables of structures) - void ReplacePtr(const element_t *elementPtr, const element_t &newElement); + void ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction = true); // Add an element to the hash table. This will never replace an element; multiple // elements may be stored with the same key. diff --git a/src/coreclr/inc/shash.inl b/src/coreclr/inc/shash.inl index 5bf0da026cbc2..65ca26ce18fc3 100644 --- a/src/coreclr/inc/shash.inl +++ b/src/coreclr/inc/shash.inl @@ -101,7 +101,7 @@ const typename SHash::element_t * SHash::LookupPtr(key_t key) co } template -void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement) +void SHash::ReplacePtr(const element_t *elementPtr, const element_t &newElement, bool invokeCleanupAction) { CONTRACT_VOID { @@ -112,14 +112,13 @@ void SHash::ReplacePtr(const element_t *elementPtr, const element_t &new PRECONDITION(elementPtr < m_table + m_tableSize); PRECONDITION(!TRAITS::IsNull(*elementPtr)); PRECONDITION(!TRAITS::IsDeleted(*elementPtr)); - PRECONDITION(elementPtr == LookupPtr(TRAITS::GetKey(*elementPtr))); PRECONDITION(!TRAITS::IsNull(newElement)); PRECONDITION(!TRAITS::IsDeleted(newElement)); PRECONDITION(TRAITS::Equals(TRAITS::GetKey(newElement), TRAITS::GetKey(*elementPtr))); } CONTRACT_END; - if (TRAITS::s_RemovePerEntryCleanupAction) + if (TRAITS::s_RemovePerEntryCleanupAction && invokeCleanupAction) { TRAITS::OnRemovePerEntryCleanupAction(*elementPtr); } diff --git a/src/coreclr/vm/crossloaderallocatorhash.inl b/src/coreclr/vm/crossloaderallocatorhash.inl index 85c7d4b07a53c..9037da4f36a4a 100644 --- a/src/coreclr/vm/crossloaderallocatorhash.inl +++ b/src/coreclr/vm/crossloaderallocatorhash.inl @@ -418,6 +418,7 @@ void CrossLoaderAllocatorHash::Add(TKey key, TValue value, LoaderAllocat { if (hashKeyToTrackers != NULL) { + delete hashKeyToTrackers->_laLocalKeyValueStore; hashKeyToTrackers->_laLocalKeyValueStore = keyValueStore; } else @@ -437,10 +438,12 @@ void CrossLoaderAllocatorHash::Add(TKey key, TValue value, LoaderAllocat if (hashKeyToTrackers == NULL) { // Nothing has yet caused the trackers proxy object to be setup. Create it now, and update the keyToTrackersHash. - // Don't need to use the holder here since there's no allocation before it's put into the hash table. + // Don't need to use the holder here since there's no allocation before it's put into the hash table. The previous + // element was the key-value store, and should not be deleted because its lifetime is being assigned to the new + // LAHashKeyToTrackers, so replace the element without cleanup. hashKeyToTrackers = new LAHashKeyToTrackers(keyValueStore); hashKeyEntry = hashKeyToTrackers; - keyToTrackersHash.ReplacePtr(hashKeyEntryPtr, hashKeyEntry); + keyToTrackersHash.ReplacePtr(hashKeyEntryPtr, hashKeyEntry, false /* invokeCleanupAction */); } // Must add it to the cross LA structure From df7f0f1e8d55cc5df6d86db026fd3066b468f189 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 7 Apr 2022 16:55:47 -0700 Subject: [PATCH 7/7] Address feedback --- src/coreclr/vm/codeversion.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/codeversion.cpp b/src/coreclr/vm/codeversion.cpp index 52c026b7da358..ffb0b73157a67 100644 --- a/src/coreclr/vm/codeversion.cpp +++ b/src/coreclr/vm/codeversion.cpp @@ -1767,10 +1767,10 @@ PCODE CodeVersionManager::PublishVersionableCodeIfNecessary( { if (doPublish) { - bool mayHaveEntryPointSlotsToBackpatch = pMethodDesc->MayHaveEntryPointSlotsToBackpatch(); - MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder( - mayHaveEntryPointSlotsToBackpatch); - pMethodDesc->TrySetInitialCodeEntryPointForVersionableMethod(pCode, mayHaveEntryPointSlotsToBackpatch); + bool mayHaveEntryPointSlotsToBackpatch2 = pMethodDesc->MayHaveEntryPointSlotsToBackpatch(); + MethodDescBackpatchInfoTracker::ConditionalLockHolder slotBackpatchLockHolder2( + mayHaveEntryPointSlotsToBackpatch2); + pMethodDesc->TrySetInitialCodeEntryPointForVersionableMethod(pCode, mayHaveEntryPointSlotsToBackpatch2); } else {