Skip to content

[release/8.0] Use unordered_map in DacInstanceManager in lieu of hand-rolled hash table #125487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hoyosjs wants to merge 1 commit into from
Closed

[release/8.0] Use unordered_map in DacInstanceManager in lieu of hand-rolled hash table #125487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/coreclr/debug/daccess/daccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,7 +1547,7 @@ DacInstanceManager::Add(DAC_INSTANCE* inst)
#ifdef _DEBUG
bool isInserted = (m_hash.find(inst->addr) == m_hash.end());
#endif //_DEBUG
DAC_INSTANCE *(&target) = m_hash[inst->addr];
DAC_INSTANCE* &target = m_hash[inst->addr];
_ASSERTE(!isInserted || target == NULL);
if( target != NULL )
Comment on lines +1550 to 1552
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this code path, the later "verify descending order" debug check is ineffective because target is overwritten with inst before the size comparison, so it will always compare inst->size to itself. If the goal is to validate ordering against the previous entry for this address, capture the old value (e.g., via inst->next) and compare against that before overwriting target.

Copilot uses AI. Check for mistakes.
{
Expand Down Expand Up @@ -2021,7 +2021,6 @@ DacInstanceManager::ClearEnumMemMarker(void)
void
DacInstanceManager::ClearEnumMemMarker(void)
{
ULONG i;
DAC_INSTANCE* inst;

DacInstanceHashIterator end = m_hash.end();
Expand Down
31 changes: 6 additions & 25 deletions src/coreclr/debug/daccess/dacimpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@
#define __DACIMPL_H__

#include "gcinterface.dac.h"

//---------------------------------------------------------------------------------------
// Setting DAC_HASHTABLE tells the DAC to use the hand rolled hashtable for
// storing code:DAC_INSTANCE . Otherwise, the DAC uses the STL unordered_map to.

#define DAC_HASHTABLE

#ifndef DAC_HASHTABLE
#pragma push_macro("return")
#undef return
Expand Down Expand Up @@ -736,9 +734,8 @@ class DacInstanceManager
// This has the benefit of scaling to different workloads appropriately (as opposed to having a
// fixed number of buckets).

class DacHashCompare : public std::hash_compare<TADDR>
struct DacInstanceHashFunc
{
public:
// Custom hash function
// The default hash function uses a pseudo-randomizing function to get a random
// distribution. In our case, we'd actually like a more even distribution to get
Expand All @@ -755,29 +752,13 @@ class DacInstanceManager
// The default pseudo-randomizing function also requires a call to ldiv which shows up as
// a 3%-5% perf hit in most perf-sensitive scenarios, so this should also always be
// faster.
inline size_t operator()(const TADDR& keyval) const
std::size_t operator()(const TADDR& keyval) const noexcept
{
return (unsigned)(keyval >>DAC_INSTANCE_HASH_SHIFT);
return (std::size_t)(keyval >>DAC_INSTANCE_HASH_SHIFT);
}

// Explicitly bring in the two-argument comparison function from the base class (just less-than)
// This is necessary because once we override one form of operator() above, we don't automatically
// get the others by C++ inheritance rules.
using std::hash_compare<TADDR>::operator();

#ifdef NIDUMP_CUSTOMIZED_DAC_HASH // not set
//this particular number is supposed to be roughly the same amount of
//memory as the old code (buckets * number of entries in the old
//blocks.)
//disabled for now. May tweak implementation later. It turns out that
//having a large number of initial buckets is excellent for nidump, but it
// is terrible for most other scenarios due to the cost of clearing them at
// every Flush. Once there is a better perf suite, we can tweak these values more.
static const size_t min_buckets = DAC_INSTANCE_HASH_SIZE * 256;
#endif

};
typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacHashCompare > DacInstanceHash;

typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacInstanceHashFunc> DacInstanceHash;
Copy link
Member

@jkotas jkotas Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been limiting use of standard C++ library in our shipping binaries:

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/clr-code-guide.md#-2114-limit-usage-of-standard-template-types-in-shipping-executables

This is probably the first use of unordered_map in the shipping binaries. What's our confidence level that it won't cause unexpected issues?

typedef DacInstanceHash::value_type DacInstanceHashValue;
typedef DacInstanceHash::iterator DacInstanceHashIterator;
DacInstanceHash m_hash;
Expand Down
Loading