Skip to content

test#127489

Closed
rcj1 wants to merge 3 commits intomainfrom
revert-127321-juhoyosa/heap2-default
Closed

test#127489
rcj1 wants to merge 3 commits intomainfrom
revert-127321-juhoyosa/heap2-default

Conversation

@rcj1
Copy link
Copy Markdown
Contributor

@rcj1 rcj1 commented Apr 28, 2026

Reverts #127321

Copilot AI review requested due to automatic review settings April 28, 2026 03:20
@rcj1 rcj1 added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Apr 28, 2026
@rcj1 rcj1 changed the title Revert "Change heap dumps to use HEAP2 as the default" test Apr 28, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reverts #127321 by restoring/adjusting DAC memory-region enumeration for several CoreCLR runtime structures and updating heap-dump flag routing in DAC/createdump paths.

Changes:

  • Add additional EnumMemoryRegions traversal for threads, classes/method tables, modules/assemblies, and debugger structures (typically gated on dump flags).
  • Expand module/assembly-related structures enumerated under DAC (including various lookup maps and dispatch/chunk structures).
  • Update DAC/createdump handling to route heap dump requests through CLRDATA_ENUM_MEM_HEAP vs CLRDATA_ENUM_MEM_HEAP2, with environment-variable-based behavior in createdump.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/coreclr/vm/threads.cpp Adds AppDomain enumeration during thread DAC enumeration (flag-gated).
src/coreclr/vm/methodtable.cpp Enumerates DispatchMap for non-mini/triage/heap2 dumps.
src/coreclr/vm/class.cpp Enumerates module and method desc chunks for non-mini/triage/heap2 dumps.
src/coreclr/vm/ceeload.cpp Expands Module::EnumMemoryRegions to include additional module caches/maps under DAC.
src/coreclr/vm/assembly.cpp Adds enumeration of classloader/module/PE assembly for non-heap2 dumps.
src/coreclr/debug/ee/functioninfo.cpp Adds additional module/methoddesc/sequence map enumeration under DAC (flag-gated).
src/coreclr/debug/daccess/enummem.cpp Refactors custom-dump worker logic and broadens heap wrapper to accept HEAP and HEAP2.
src/coreclr/debug/createdump/crashinfo.cpp Adds env-var compatibility/opt-out logic to choose heap fast-path behavior and flags.

Comment on lines +1890 to +1893
if (m_pModule.IsValid())
{
m_pModule->EnumMemoryRegions(flags, true);
}
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

Assembly::EnumMemoryRegions now calls m_pModule->EnumMemoryRegions(), but Module::EnumMemoryRegions also calls m_pAssembly->EnumMemoryRegions(). For a typical assembly/manifest module pair this creates mutual recursion and can lead to unbounded recursion during DAC memory enumeration. Consider removing the module enumeration from Assembly::EnumMemoryRegions (or otherwise breaking the cycle, e.g., a helper that doesn't recurse back to the Assembly).

Suggested change
if (m_pModule.IsValid())
{
m_pModule->EnumMemoryRegions(flags, true);
}

Copilot uses AI. Check for mistakes.
Comment on lines +6878 to +6882
if (flags != CLRDATA_ENUM_MEM_MINI && flags != CLRDATA_ENUM_MEM_TRIAGE && flags != CLRDATA_ENUM_MEM_HEAP2)
{
AppDomain::GetCurrentDomain()->EnumMemoryRegions(flags, true);
}

Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

AppDomain::EnumMemoryRegions is now invoked both in Thread::EnumMemoryRegions and again in Thread::EnumMemoryRegionsWorker for the same flags. Since Worker is always called from EnumMemoryRegions, this duplicates AppDomain/assembly/module enumeration work and can significantly increase dump size/time. Only one of these calls should remain (likely keep the top-level call and remove the Worker call).

Suggested change
if (flags != CLRDATA_ENUM_MEM_MINI && flags != CLRDATA_ENUM_MEM_TRIAGE && flags != CLRDATA_ENUM_MEM_HEAP2)
{
AppDomain::GetCurrentDomain()->EnumMemoryRegions(flags, true);
}

Copilot uses AI. Check for mistakes.
Comment on lines +2438 to +2447
if (flags != CLRDATA_ENUM_MEM_MINI && flags != CLRDATA_ENUM_MEM_TRIAGE && flags != CLRDATA_ENUM_MEM_HEAP2)
{
// Modules are enumerated already for minidumps, save the empty calls.
if (m_module.IsValid())
{
m_module->EnumMemoryRegions(flags, true);
}

}

Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

DebuggerMethodInfoEntry::EnumMemoryRegions already enumerates key.pModule when flags are not MINI/TRIAGE/HEAP2. The newly added module enumeration in DebuggerMethodInfo::EnumMemoryRegions causes the same module to be enumerated again for each DebuggerMethodInfo reached from an entry, adding redundant work/size. Consider removing one of the two module-enumeration sites (preferably keep it at the entry level).

Suggested change
if (flags != CLRDATA_ENUM_MEM_MINI && flags != CLRDATA_ENUM_MEM_TRIAGE && flags != CLRDATA_ENUM_MEM_HEAP2)
{
// Modules are enumerated already for minidumps, save the empty calls.
if (m_module.IsValid())
{
m_module->EnumMemoryRegions(flags, true);
}
}

Copilot uses AI. Check for mistakes.
status = E_INVALIDARG;
}

return S_OK;
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

EnumMemoryRegionsWorkerCustom computes a "status" value (including setting E_INVALIDARG), but unconditionally returns S_OK. This will mask failures/cancellation from callers and can make dump generation report success even when enumeration failed. Return the computed status instead of always returning S_OK.

Suggested change
return S_OK;
return status;

Copilot uses AI. Check for mistakes.
// Custom dumps enumerate the minimal CLR state needed for
// MiniDumpWithFullAuxiliaryState: thread stacks, modules, CLR statics, and any
// memory reached implicitly from those roots.
// Custom Dump. Depending on the value of g_ECustomDumpFlavor, different dump
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

Comment grammar: "different dump will be taken" should be plural (e.g., "different dumps will be taken").

Suggested change
// Custom Dump. Depending on the value of g_ECustomDumpFlavor, different dump
// Custom Dump. Depending on the value of g_ECustomDumpFlavor, different dumps

Copilot uses AI. Check for mistakes.
{
// Since on MacOS all the RW regions will be added for heap dumps by createdump, the
// only thing differentiating a MiniDumpNormal and a MiniDumpWithPrivateReadWriteMemory
// is that the later uses the EnumMemoryRegions APIs. This is kind of expensive on larger
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

Comment typo: "the later uses" should be "the latter uses" (referring to MiniDumpWithPrivateReadWriteMemory vs MiniDumpNormal).

Suggested change
// is that the later uses the EnumMemoryRegions APIs. This is kind of expensive on larger
// is that the latter uses the EnumMemoryRegions APIs. This is kind of expensive on larger

Copilot uses AI. Check for mistakes.
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Diagnostics-coreclr NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants