Stop using DomainAssemblyCache for unmanaged image cache used by p/invokes (#51704)

Author: elinor-fung

src/coreclr/vm/appdomain.cpp

@@ -679,8 +679,6 @@ void BaseDomain::Init()
     m_DomainCacheCrst.Init(CrstAppDomainCache);
     m_DomainLocalBlockCrst.Init(CrstDomainLocalBlock);
 
-    m_InteropDataCrst.Init(CrstInteropData, CRST_REENTRANCY);
-
     // NOTE: CRST_UNSAFE_COOPGC prevents a GC mode switch to preemptive when entering this crst.
     // If you remove this flag, we will switch to preemptive mode when entering
     // m_FileLoadLock, which means all functions that enter it will become
@@ -716,12 +714,6 @@ void BaseDomain::Init()
     m_pMngStdInterfacesInfo = new MngStdInterfacesInfo();
 #endif // FEATURE_COMINTEROP
 
-    // Init the COM Interop data hash
-    {
-        LockOwner lock = {&m_InteropDataCrst, IsOwnerOfCrst};
-        m_interopDataHash.Init(0, NULL, false, &lock);
-    }
-
     m_dwSizedRefHandles = 0;
     if (!m_iNumberOfProcessors)
     {
@@ -2210,9 +2202,8 @@ void AppDomain::Init()
 
     BaseDomain::Init();
 
-// Set up the binding caches
+    // Set up the binding caches
     m_AssemblyCache.Init(&m_DomainCacheCrst, GetHighFrequencyHeap());
-    m_UnmanagedCache.InitializeTable(this, &m_DomainCacheCrst);
 
     m_MemoryPressure = 0;
 
@@ -3778,39 +3769,50 @@ void AppDomain::AddUnmanagedImageToCache(LPCWSTR libraryName, NATIVE_LIBRARY_HAN
     CONTRACTL
     {
         THROWS;
-        GC_TRIGGERS;
+        GC_NOTRIGGER;
         MODE_ANY;
         PRECONDITION(CheckPointer(libraryName));
+        PRECONDITION(CheckPointer(hMod));
         INJECT_FAULT(COMPlusThrowOM(););
     }
     CONTRACTL_END;
-    if (libraryName)
+
+    CrstHolder lock(&m_DomainCacheCrst);
+    
+    const UnmanagedImageCacheEntry *existingEntry = m_unmanagedCache.LookupPtr(libraryName);
+    if (existingEntry != NULL)
     {
-        AssemblySpec spec;
-        spec.SetCodeBase(libraryName);
-        m_UnmanagedCache.InsertEntry(&spec, hMod);
+        _ASSERTE(existingEntry->Handle == hMod);
+        return;
     }
-    return ;
-}
 
+    size_t len = (wcslen(libraryName) + 1) * sizeof(WCHAR);
+    AllocMemHolder<WCHAR> copiedName(GetLowFrequencyHeap()->AllocMem(S_SIZE_T(len)));
+    memcpy(copiedName, libraryName, len);
+
+    m_unmanagedCache.Add(UnmanagedImageCacheEntry{ copiedName, hMod });
+    copiedName.SuppressRelease();
+}
 
 NATIVE_LIBRARY_HANDLE AppDomain::FindUnmanagedImageInCache(LPCWSTR libraryName)
 {
     CONTRACT(NATIVE_LIBRARY_HANDLE)
     {
         THROWS;
-        GC_TRIGGERS;
+        GC_NOTRIGGER;
         MODE_ANY;
-        PRECONDITION(CheckPointer(libraryName,NULL_OK));
+        PRECONDITION(CheckPointer(libraryName));
         POSTCONDITION(CheckPointer(RETVAL,NULL_OK));
         INJECT_FAULT(COMPlusThrowOM(););
     }
     CONTRACT_END;
-    if(libraryName == NULL) RETURN NULL;
 
-    AssemblySpec spec;
-    spec.SetCodeBase(libraryName);
-    RETURN (NATIVE_LIBRARY_HANDLE) m_UnmanagedCache.LookupEntry(&spec, 0);
+    CrstHolder lock(&m_DomainCacheCrst);
+    const UnmanagedImageCacheEntry *existingEntry = m_unmanagedCache.LookupPtr(libraryName);
+    if (existingEntry == NULL)
+        RETURN NULL;
+
+    RETURN existingEntry->Handle;
 }
 
 BOOL AppDomain::RemoveFileFromCache(PEAssembly *pFile)

src/coreclr/vm/appdomain.hpp

@@ -41,22 +41,15 @@ class BaseDomain;
 class SystemDomain;
 class AppDomain;
 class CompilationDomain;
-class AppDomainEnum;
-class AssemblySink;
-class EEMarshalingData;
 class GlobalStringLiteralMap;
 class StringLiteralMap;
 class MngStdInterfacesInfo;
-class DomainModule;
 class DomainAssembly;
-struct InteropMethodTableData;
 class LoadLevelLimiter;
 class TypeEquivalenceHashTable;
-class StringArrayList;
 
 #ifdef FEATURE_COMINTEROP
-class ComCallWrapperCache;
-struct SimpleComCallWrapper;
+class RCWCache;
 class RCWRefCache;
 #endif // FEATURE_COMINTEROP
 
@@ -68,13 +61,6 @@ class RCWRefCache;
 
 GPTR_DECL(IdDispenser,       g_pModuleIndexDispenser);
 
-// This enum is aligned to System.ExceptionCatcherType.
-enum ExceptionCatcher {
-    ExceptionCatcher_ManagedCode = 0,
-    ExceptionCatcher_AppDomainTransition = 1,
-    ExceptionCatcher_COMInterop = 2,
-};
-
 // We would like *ALLOCATECLASS_FLAG to AV (in order to catch errors), so don't change it
 struct ClassInitFlags {
     enum
@@ -1131,17 +1117,11 @@ class BaseDomain
     // Helper method to initialize the large heap handle table.
     void InitPinnedHeapHandleTable();
 
-    //****************************************************************************************
-    //
-    // Hash table that maps a MethodTable to COM Interop compatibility data.
-    PtrHashMap          m_interopDataHash;
-
     // Critical sections & locks
     PEFileListLock   m_FileLoadLock;            // Protects the list of assemblies in the domain
     CrstExplicitInit m_DomainCrst;              // General Protection for the Domain
     CrstExplicitInit m_DomainCacheCrst;         // Protects the Assembly and Unmanaged caches
     CrstExplicitInit m_DomainLocalBlockCrst;
-    CrstExplicitInit m_InteropDataCrst;         // Used for COM Interop compatiblilty
     // Used to protect the reference lists in the collectible loader allocators attached to this appdomain
     CrstExplicitInit m_crstLoaderAllocatorReferences;
 
@@ -1191,17 +1171,6 @@ class BaseDomain
     };
     friend class LockHolder;
 
-    class CacheLockHolder : public CrstHolder
-    {
-    public:
-        CacheLockHolder(BaseDomain *pD)
-            : CrstHolder(&pD->m_DomainCacheCrst)
-        {
-            WRAPPER_NO_CONTRACT;
-        }
-    };
-    friend class CacheLockHolder;
-
     class DomainLocalBlockLockHolder : public CrstHolder
     {
     public:
@@ -1510,13 +1479,11 @@ const DWORD DefaultADID = 1;
 class AppDomain : public BaseDomain
 {
     friend class SystemDomain;
-    friend class AssemblySink;
     friend class AppDomainNative;
     friend class AssemblyNative;
     friend class AssemblySpec;
     friend class ClassLoader;
     friend class ThreadNative;
-    friend class RCWCache;
     friend class ClrDataAccess;
     friend class CheckAsmOffsets;
 
@@ -1989,9 +1956,7 @@ class AppDomain : public BaseDomain
 #ifdef FEATURE_COMINTEROP
 public:
     OBJECTREF GetMissingObject();    // DispatchInfo will call function to retrieve the Missing.Value object.
-#endif // FEATURE_COMINTEROP
 
-#ifdef FEATURE_COMINTEROP
     RCWCache *GetRCWCache()
     {
         WRAPPER_NO_CONTRACT;
@@ -2014,9 +1979,6 @@ class AppDomain : public BaseDomain
     RCWRefCache *GetRCWRefCache();
 #endif // FEATURE_COMINTEROP
 
-    //****************************************************************************************
-    // Get the proxy for this app domain
-
     TPIndex GetTPIndex()
     {
         LIMITED_METHOD_CONTRACT;
@@ -2247,8 +2209,6 @@ class AppDomain : public BaseDomain
     void AddMemoryPressure();
     void RemoveMemoryPressure();
 
-    void UnlinkClass(MethodTable *pMT);
-
     Assembly *GetRootAssembly()
     {
         LIMITED_METHOD_CONTRACT;
@@ -2357,13 +2317,30 @@ class AppDomain : public BaseDomain
     };
 
     AssemblySpecBindingCache  m_AssemblyCache;
-    DomainAssemblyCache       m_UnmanagedCache;
     size_t                    m_MemoryPressure;
 
     ArrayList m_NativeDllSearchDirectories;
     bool m_ForceTrivialWaitOperations;
 
-public:
+private:
+    struct UnmanagedImageCacheEntry
+    {
+        LPCWSTR Name;
+        NATIVE_LIBRARY_HANDLE Handle;
+    };
+
+    class UnmanagedImageCacheTraits : public NoRemoveSHashTraits<DefaultSHashTraits<UnmanagedImageCacheEntry>>
+    {
+    public:
+        using key_t = LPCWSTR;
+        static const key_t GetKey(_In_ const element_t& e) { return e.Name; }
+        static count_t Hash(_In_ key_t key) { return HashString(key); }
+        static bool Equals(_In_ key_t lhs, _In_ key_t rhs) { return wcscmp(lhs, rhs) == 0; }
+        static bool IsNull(_In_ const element_t& e) { return e.Handle == NULL; }
+        static const element_t Null() { return UnmanagedImageCacheEntry(); }
+    };
+
+    SHash<UnmanagedImageCacheTraits> m_unmanagedCache;
 
 #ifdef FEATURE_TYPEEQUIVALENCE
 private:
@@ -2410,38 +2387,6 @@ class AppDomain : public BaseDomain
 
 #endif
 
-#ifdef FEATURE_COMINTEROP
-
-private:
-
-#endif //FEATURE_COMINTEROP
-
-public:
-
-    class ComInterfaceReleaseList
-    {
-        SArray<IUnknown *> m_objects;
-    public:
-        ~ComInterfaceReleaseList()
-        {
-            WRAPPER_NO_CONTRACT;
-
-            for (COUNT_T i = 0; i < m_objects.GetCount(); i++)
-            {
-                IUnknown *pItf = *(m_objects.GetElements() + i);
-                if (pItf != nullptr)
-                    pItf->Release();
-            }
-        }
-
-        // Append to the list of object to free. Only use under the AppDomain "LockHolder(pAppDomain)"
-        void Append(IUnknown *pInterfaceToRelease)
-        {
-            WRAPPER_NO_CONTRACT;
-            m_objects.Append(pInterfaceToRelease);
-        }
-    } AppDomainInterfaceReleaseList;
-
 private:
     //-----------------------------------------------------------
     // Static ICLRPrivAssembly -> DomainAssembly mapping functions.

src/coreclr/vm/assemblyspec.cpp

@@ -1573,92 +1573,6 @@ BOOL AssemblySpecBindingCache::CompareSpecs(UPTR u1, UPTR u2)
     return a1->CompareEx(a2);
 }
 
-/* static */
-BOOL DomainAssemblyCache::CompareBindingSpec(UPTR spec1, UPTR spec2)
-{
-    WRAPPER_NO_CONTRACT;
-
-    AssemblySpec* pSpec1 = (AssemblySpec*) (spec1 << 1);
-    AssemblyEntry* pEntry2 = (AssemblyEntry*) spec2;
-
-    return pSpec1->CompareEx(&pEntry2->spec);
-}
-
-DomainAssemblyCache::AssemblyEntry* DomainAssemblyCache::LookupEntry(AssemblySpec* pSpec)
-{
-    CONTRACT (DomainAssemblyCache::AssemblyEntry*)
-    {
-        INSTANCE_CHECK;
-        THROWS;
-        GC_TRIGGERS;
-        MODE_ANY;
-        POSTCONDITION(CheckPointer(RETVAL, NULL_OK));
-        INJECT_FAULT(COMPlusThrowOM(););
-    }
-    CONTRACT_END
-
-    DWORD hashValue = pSpec->Hash();
-
-    LPVOID pResult = m_Table.LookupValue(hashValue, pSpec);
-    if(pResult == (LPVOID) INVALIDENTRY)
-        RETURN NULL;
-    else
-        RETURN (AssemblyEntry*) pResult;
-}
-
-VOID DomainAssemblyCache::InsertEntry(AssemblySpec* pSpec, LPVOID pData1, LPVOID pData2/*=NULL*/)
-{
-    CONTRACTL
-    {
-        INSTANCE_CHECK;
-        THROWS;
-        GC_TRIGGERS;
-        MODE_ANY;
-        INJECT_FAULT(COMPlusThrowOM(););
-    }
-    CONTRACTL_END
-
-    LPVOID ptr = LookupEntry(pSpec);
-    if(ptr == NULL) {
-
-        BaseDomain::CacheLockHolder lh(m_pDomain);
-
-        ptr = LookupEntry(pSpec);
-        if(ptr == NULL) {
-            AllocMemTracker amTracker;
-            AllocMemTracker *pamTracker = &amTracker;
-
-            AssemblyEntry* pEntry = (AssemblyEntry*) pamTracker->Track( m_pDomain->GetLowFrequencyHeap()->AllocMem(S_SIZE_T(sizeof(AssemblyEntry))) );
-            new (&pEntry->spec) AssemblySpec ();
-
-            pEntry->spec.CopyFrom(pSpec);
-            pEntry->spec.CloneFieldsToLoaderHeap(AssemblySpec::ALL_OWNED, m_pDomain->GetLowFrequencyHeap(), pamTracker);
-
-            // Clear the parent assembly, it is not needed for the AssemblySpec in the cache entry and it could contain stale
-            // pointer when the parent was a collectible assembly that was collected.
-            pEntry->spec.SetParentAssembly(NULL);
-
-            pEntry->pData[0] = pData1;
-            pEntry->pData[1] = pData2;
-            DWORD hashValue = pEntry->Hash();
-            m_Table.InsertValue(hashValue, pEntry);
-
-            pamTracker->SuppressRelease();
-        }
-        // lh goes out of scope here
-    }
-#ifdef _DEBUG
-    else {
-        _ASSERTE(pData1 == ((AssemblyEntry*) ptr)->pData[0]);
-        _ASSERTE(pData2 == ((AssemblyEntry*) ptr)->pData[1]);
-    }
-#endif
-
-}
-
-
-
-
 DomainAssembly * AssemblySpec::GetParentAssembly()
 {
     LIMITED_METHOD_CONTRACT;