[release/8.0] Cache debugger patches to speed up x64 stackwalk epilogue/prologue scanning

src/coreclr/debug/daccess/daccess.cpp

@@ -3270,6 +3270,9 @@ ClrDataAccess::Flush(void)
     //
     m_mdImports.Flush();
 
+    // Free cached patch entries for thread unwinding
+    m_patchCache.Flush();
+
     // Free instance memory.
     m_instances.Flush();
 

src/coreclr/debug/daccess/dacfn.cpp

@@ -1505,43 +1505,68 @@ HRESULT DacReplacePatchesInHostMemory(MemoryRange range, PVOID pBuffer)
         return S_OK;
     }
 
+    if (g_dacImpl == nullptr)
+    {
+        return E_UNEXPECTED;
+    }
+
+    // Cache the patches as the target is not running during DAC operations, and hash
+    // table iteration is pretty slow.
+    const SArray<DacPatchCacheEntry>& entries = g_dacImpl->m_patchCache.GetEntries();
+
+    CORDB_ADDRESS address = (CORDB_ADDRESS)(dac_cast<TADDR>(range.StartAddress()));
+    SIZE_T        cbSize  = range.Size();
+
+    for (COUNT_T i = 0; i < entries.GetCount(); i++)
+    {
+        const DacPatchCacheEntry& entry = entries[i];
+
+        if (IsPatchInRequestedRange(address, cbSize, entry.address))
+        {
+            CORDbgSetInstructionEx(reinterpret_cast<PBYTE>(pBuffer), address, entry.address, entry.opcode, cbSize);
+        }
+    }
+
+    return S_OK;
+}
+
+const SArray<DacPatchCacheEntry>& DacPatchCache::GetEntries()
+{
+    Populate();
+    return m_entries;
+}
+
+void DacPatchCache::Populate()
+{
+    SUPPORTS_DAC;
+
+    if (m_isPopulated)
+    {
+        return;
+    }
+
+    // Clear any stale entries from previous failed population attempts
+    m_entries.Clear();
+
     HASHFIND info;
 
     DebuggerPatchTable *      pTable = DebuggerController::GetPatchTable();
     DebuggerControllerPatch * pPatch = pTable->GetFirstPatch(&info);
 
-    // <PERF>
-    // The unwinder needs to read the stack very often to restore pushed registers, retrieve the
-    // return addres, etc.  However, stack addresses should never be patched.
-    // One way to optimize this code is to pass the stack base and the stack limit of the thread to this
-    // function and use those two values to filter out stack addresses.
-    //
-    // Another thing we can do is instead of enumerating the patches, we could enumerate the address.
-    // This is more efficient when we have a large number of patches and a small memory range.  Perhaps
-    // we could do a hybrid approach, i.e. use the size of the range and the number of patches to dynamically
-    // determine which enumeration is more efficient.
-    // </PERF>
     while (pPatch != NULL)
     {
         CORDB_ADDRESS patchAddress = (CORDB_ADDRESS)dac_cast<TADDR>(pPatch->address);
 
         if (patchAddress != NULL)
         {
-            PRD_TYPE opcode = pPatch->opcode;
-
-            CORDB_ADDRESS address = (CORDB_ADDRESS)(dac_cast<TADDR>(range.StartAddress()));
-            SIZE_T        cbSize  = range.Size();
-
-            // Check if the address of the patch is in the specified memory range.
-            if (IsPatchInRequestedRange(address, cbSize, patchAddress))
-            {
-                // Replace the patch in the buffer with the original opcode.
-                CORDbgSetInstructionEx(reinterpret_cast<PBYTE>(pBuffer), address, patchAddress, opcode, cbSize);
-            }
+            DacPatchCacheEntry entry;
+            entry.address = patchAddress;
+            entry.opcode = pPatch->opcode;
+            m_entries.Append(entry);
         }
 
         pPatch = pTable->GetNextPatch(&info);
     }
 
-    return S_OK;
+    m_isPopulated = true;
 }

src/coreclr/debug/daccess/dacimpl.h

@@ -795,6 +795,48 @@ class DacStreamManager;
 #endif // FEATURE_MINIMETADATA_IN_TRIAGEDUMPS
 
 
+//----------------------------------------------------------------------------
+//
+// DacPatchCache - Caches debugger breakpoint patches from the target process.
+//
+// DacReplacePatchesInHostMemory needs to iterate the target's patch hash table
+// to replace breakpoint instructions with original opcodes. This iteration is
+// expensive because it walks all hash buckets and entries and it will hit either a
+// hash lookup in the instance cache or a cache miss + remote read. Since the target
+// is not running during DAC operations, the patches should not change while we are performing
+// memory enumeration, but if they do we'll miss them until the next time Flush() gets called.
+//
+//----------------------------------------------------------------------------
+
+struct DacPatchCacheEntry
+{
+    CORDB_ADDRESS address;
+    PRD_TYPE opcode;
+};
+
+class DacPatchCache
+{
+public:
+    DacPatchCache()
+        : m_isPopulated(false)
+    {
+    }
+
+    const SArray<DacPatchCacheEntry>& GetEntries();
+
+    void Flush()
+    {
+        m_entries.Clear();
+        m_isPopulated = false;
+    }
+
+private:
+    void Populate();
+
+    SArray<DacPatchCacheEntry> m_entries;
+    bool m_isPopulated;
+};
+
 //----------------------------------------------------------------------------
 //
 // ClrDataAccess.
@@ -1207,7 +1249,7 @@ class ClrDataAccess
         CLRDATA_ADDRESS *allocLimit);
 
     // ISOSDacInterface13
-    virtual HRESULT STDMETHODCALLTYPE TraverseLoaderHeap(CLRDATA_ADDRESS loaderHeapAddr, LoaderHeapKind kind, VISITHEAP pCallback);        
+    virtual HRESULT STDMETHODCALLTYPE TraverseLoaderHeap(CLRDATA_ADDRESS loaderHeapAddr, LoaderHeapKind kind, VISITHEAP pCallback);
     virtual HRESULT STDMETHODCALLTYPE GetDomainLoaderAllocator(CLRDATA_ADDRESS domainAddress, CLRDATA_ADDRESS *pLoaderAllocator);
     virtual HRESULT STDMETHODCALLTYPE GetLoaderAllocatorHeapNames(int count, const char **ppNames, int *pNeeded);
     virtual HRESULT STDMETHODCALLTYPE GetLoaderAllocatorHeaps(CLRDATA_ADDRESS loaderAllocator, int count, CLRDATA_ADDRESS *pLoaderHeaps, LoaderHeapKind *pKinds, int *pNeeded);
@@ -1407,6 +1449,7 @@ class ClrDataAccess
     DacInstanceManager m_instances;
     ULONG32 m_instanceAge;
     bool m_debugMode;
+    DacPatchCache m_patchCache;
 
 #ifdef FEATURE_MINIMETADATA_IN_TRIAGEDUMPS
 
@@ -1958,7 +2001,7 @@ class DacMemoryEnumerator : public DefaultCOMImpl<ISOSMemoryEnum, IID_ISOSMemory
 
     virtual ~DacMemoryEnumerator() {}
     virtual HRESULT Init() = 0;
-    
+
     HRESULT STDMETHODCALLTYPE Skip(unsigned int count);
     HRESULT STDMETHODCALLTYPE Reset();
     HRESULT STDMETHODCALLTYPE GetCount(unsigned int *pCount);