Repository navigation
Replace CHashTable and CHashTableAndData with SHash - #136239
AaronRobinsonMSFT merged 13 commits into
Conversation
Add index-based traversal, DAC backing-memory enumeration, and optional retained-capacity clearing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Preserve COM enumeration ownership, patch-slot indices, and pending-evaluation ordering and OOM handling. Validated ICorDebugEnum cloning/reset/skip, ICorDebugBreakpointEnum and ICorDebugStepperEnum enumeration, and ICorDebugEval::NewString. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Use SHash for profiler allocation counts and remove CHashTable, CHashTableAndData, and their unused allocators. Validated ICorProfilerCallback::ObjectsAllocatedByClass and allocation-statistics suppression. Full x64 Checked CoreCLR and Release libraries/host build passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Release interop-safe array storage when element construction fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Enforce trivial copyability and hash-entry layout for byte-managed patch storage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Remove const casts from debugger and profiler table access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Unix VPTR macros declare an int constructor and suppress the implicit default constructor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
CHashTable and CHashTableAndData with SHash
Remove indexed traversal and debugger adapters. Release Cordb entries through the traits cleanup hook. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Remove the callback API and reuse existing slots for replacement. Split trait-controlled cleanup from explicit cleanup overrides, and mark in-place updates as non-cleaning. Validated Checked x64 runtime/DAC/DBI, native EE/DBI checks, and live ICorDebug scenarios. The cleanup-override check fails before the change and passes afterward. Optimized counter code is unchanged by the overload split. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
|
This looks good to me from a cDAC perspective. I queued a runtime-diagnostics pipeline on this branch to verify. |
Keep LookupPtr read-only and replace ReplacePtr and lookup-entry mutation casts with Update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
hoyosjs
left a comment
There was a problem hiding this comment.
I like this better than the old design - that being said to document:
Enumerations will now report the data as seen by the snapshot - so it may miss new or see deleted cordb objects. To me this is ok and a design point. Debuggers might need to adjust to it though.
Use typed patch slots and value initialization; clarify borrowed lookup lifetimes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce4b0890-6559-4b90-b46f-11179e70f973
jkotas
left a comment
There was a problem hiding this comment.
LGTM!
It may be a good idea to run some debugger tests if you have not done it already.
|
@steveisok and @hoyosjs I've shared this PR with our VS peers as an FYI. I've tried a slew of scenarios locally, but obviously not every possible scenario. |
tommcdon
left a comment
There was a problem hiding this comment.
Changes look good to me. I didn't observe any negative impact to debugger scenarios.
Changes
Replace the fixed-bucket debugger and profiler hash tables with growing
SHashimplementations.LookupPtrread-only and use callback-basedUpdatefor existing entries in the shared and NativeAOT SHash implementations. Use native iteration, DAC backing-storage enumeration, and retained-capacity clearing; document borrowed lookup-pointer lifetimes.Removed
Delete
CHashTable,CHashTableAndData<>,HASHENTRY,FREEHASHENTRY,HASHFIND, their pointer typedefs, the unusedCNewData/CNewDataNoThrowallocators, and the profiler'sCHashTableImplwrapper.Validation
Built Windows x64 Checked CoreCLR with Release libraries/host. Ran native debugger/SHash checks, cDAC unit tests, runtime loader and Hot Reload suites, managed stack-trace tests, live debugger scenarios, and profiler allocation/GC tests.
Observed performance
A native Windows x64 Checked
/O2lookup diagnostic compared the migrated DBI table with a synthetic legacy 101-bucket table: one million lookups per round, three rounds. Medians below are ns/lookup; lower is better.These are local synthetic lookup measurements.
Known limitations
Debugger::TrapAllRuntimeThreadsGC contract assertion previously reproduced on the original baseline. The profiler GC test also prints the sameGetObjectGenerationdiagnostics as the baseline, despite passing.ApplyChanges, and NativeAOT GC-stress execution were not exercised locally.Related to #11056; the measurements above do not establish a fix for current ordinary Release exception-formatting performance. This broader migration supersedes #132264.
Resolves #11466
Note
This description includes content generated by GitHub Copilot.