Try using unordered_map hash in DacInstanceManager#123737
Try using unordered_map hash in DacInstanceManager#123737hoyosjs wants to merge 2 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
To fix gcc error, line 1549:
- DAC_INSTANCE *(&target) = m_hash[inst->addr];
+. DAC_INSTANCE*& target = m_hash[inst->addr];There was a problem hiding this comment.
Pull request overview
This PR switches DacInstanceManager’s instance lookup structure to the std::unordered_map-based implementation by default, and updates the custom hashing hook to avoid reliance on non-portable hash_compare.
Changes:
- Disable the hand-rolled DAC hashtable by default (
DAC_HASHTABLE) so the unordered_map path is used. - Replace
std::hash_compare-based hashing with a portable unary hash functor forstd::unordered_map. - Minor cleanup in
daccess.cpp(reference declaration simplification; remove unused local).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/coreclr/debug/daccess/dacimpl.h |
Switches default to unordered_map and replaces the hasher implementation with a portable functor. |
src/coreclr/debug/daccess/daccess.cpp |
Small cleanups in the unordered_map codepath (reference syntax; remove unused variable). |
| }; | ||
| typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacHashCompare > DacInstanceHash; | ||
| // typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacHashCompare > DacInstanceHash; | ||
| // Start the experiment without bringing in hash_compare which was moved to stdex and isn't portable. |
There was a problem hiding this comment.
The comment says stdex, but the MSVC extension namespace for hash_compare is stdext. This typo makes the rationale harder to understand; please correct the namespace in the comment.
| // Start the experiment without bringing in hash_compare which was moved to stdex and isn't portable. | |
| // Start the experiment without bringing in hash_compare which was moved to stdext and isn't portable. |
|
|
||
| }; | ||
| typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacHashCompare > DacInstanceHash; | ||
| // typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacHashCompare > DacInstanceHash; |
There was a problem hiding this comment.
Please remove the commented-out typedef line. Keeping dead/commented-out code in the header adds noise and makes it unclear which declaration is intended to be authoritative.
| // typedef std::unordered_map<TADDR, DAC_INSTANCE*, DacHashCompare > DacInstanceHash; |
| struct DacHashCompare | ||
| { |
There was a problem hiding this comment.
DacHashCompare is used purely as the hash functor for std::unordered_map (it doesn't define a comparison). Consider renaming it to something like DacInstanceHash/DacHasher to better reflect its role.
| // 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 | ||
| // #define DAC_HASHTABLE |
There was a problem hiding this comment.
Commenting out #define DAC_HASHTABLE changes the default behavior for all builds. If the intent is to make this a configurable experiment, consider moving the default selection to the build system (e.g., a feature flag/define) rather than leaving a commented-out macro in the header.
No description provided.