Skip to content

[cDAC] Implement DacDbi heap walking#128463

Merged
rcj1 merged 18 commits into
mainfrom
copilot/implement-dacdbi-api
Jun 1, 2026
Merged

[cDAC] Implement DacDbi heap walking#128463
rcj1 merged 18 commits into
mainfrom
copilot/implement-dacdbi-api

Conversation

Copilot AI commented May 21, 2026

Copy link
Copy Markdown
Contributor
  • Cleans up the native DAC implementation of heap walking. Upon E_FAIL, a consumer of this API keeps trying and trying again to advance the heap walk, hoping that we will encounter a good object or switch to the next segment. Refactored the implementation such that we switch to the next good segment upon reaching heap corruption instead of striding forward by the last known object size - hopefully improving performance and discovering more objects on corrupt heaps. We do need the intermediate E_FAIL to signal to consumers "this is a partial heap".
  • Implements the heap walking algorithm in cDAC, including a linear cache for performance in the large heap enum loop, matching the native implementation .
  • Implements some GC-heap related alignment helpers:
    // Returns the next candidate object address within a heap segment.
    TargetPointer GetPotentialNextObjectAddress(
        TargetPointer currentAddress,
        ulong currentObjectSize,
        GCHeapSegmentInfo segment) => throw new NotImplementedException();

    // Aligns an object's raw size to the alignment required by its containing segment.
    ulong AlignObjectSize(ulong size, GCSegmentClassification generation) => throw new NotImplementedException();
  • Implement APIs CreateHeapWalk, WalkHeap, and DeleteHeapWalk

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 21, 2026 23:46
Copilot AI review requested due to automatic review settings May 21, 2026 23:46
@rcj1 rcj1 changed the title Inline per-object MT/size reads in DacDbi heap walk with a linear page cache [cDAC] Implement DacDbi heap walkign May 21, 2026
@rcj1 rcj1 changed the title [cDAC] Implement DacDbi heap walkign [cDAC] Implement DacDbi heap walking May 21, 2026
@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.

Copilot AI review requested due to automatic review settings May 22, 2026 02:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 extends the cDAC-based DacDbi implementation to support GC heap walking, adds GC contract helpers needed to mirror the native DAC walking logic (object-size alignment and allocation-context skipping), and refactors the native DAC heap walker to advance to the next valid segment/object on corruption while still signaling partial-heap results via E_FAIL.

Changes:

  • Implement CreateHeapWalk, WalkHeap, and DeleteHeapWalk in the managed (cDAC) DacDbiImpl, including a linear read cache and per-segment enumeration.
  • Add IGC.GetPotentialNextObjectAddress and IGC.AlignObjectSize (and implement them in GC_1) to support heap-walk stepping/alignment parity with native.
  • Refactor native DacHeapWalker to advance to the next valid object/segment after corruption and fix allocation-context bookkeeping.
Show a summary per file
File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Adds managed heap-walk handle lifecycle + iterator-based walking with a small page cache and alloc-context handling.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GC/GC_1.cs Implements contract helpers for alloc-context skipping and SOH/LOH/POH alignment needed by the walker.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IGC.cs Extends the IGC contract interface with two new helper methods used by heap walking.
src/coreclr/debug/daccess/dacimpl.h Updates DacHeapWalker private method naming to match the new “advance to next valid object” behavior.
src/coreclr/debug/daccess/dacdbiimpl.cpp Reworks heap-walk advancement logic to skip corrupt segments cleanly (while surfacing partial-heap) and fixes global alloc context counting.
docs/design/datacontracts/GC.md Documents the new IGC APIs and the intended algorithms for stepping/alignment.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 8

Comment thread docs/design/datacontracts/GC.md
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 22, 2026 03:34
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 3

Copilot AI review requested due to automatic review settings May 22, 2026 03:44
@rcj1 rcj1 marked this pull request as ready for review May 22, 2026 03:45
@rcj1 rcj1 requested review from hoyosjs, max-charlamb and noahfalk May 22, 2026 03:45
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot finished work on behalf of rcj1 May 22, 2026 03:52
@rcj1 rcj1 marked this pull request as ready for review May 22, 2026 05:51
@rcj1 rcj1 requested review from hoyosjs, max-charlamb and noahfalk May 22, 2026 05:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 5

@max-charlamb max-charlamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It'd be good to add at least a single dump test verifying that we can successfully get the Heap items.

Comment thread src/coreclr/debug/daccess/dacdbiimpl.cpp Outdated
Comment thread src/coreclr/debug/daccess/dacdbiimpl.cpp Outdated
@rcj1

rcj1 commented May 28, 2026

Copy link
Copy Markdown
Contributor

It'd be good to add at least a single dump test verifying that we can successfully get the Heap items.

Sure, we can at least verify a handful of items. A more thorough validation against the legacy with all the heap items is done in the internal tests. This test won’t really be useful for diagnosing partial failure, as we can’t know a priori everything on the heap, but if it completely fails then it would be a signal.

Copilot AI review requested due to automatic review settings May 29, 2026 17:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 9/9 changed files
  • Comments generated: 6

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 29, 2026 17:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 9/9 changed files
  • Comments generated: 1

@rcj1

rcj1 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

/ba-g timeout

@rcj1 rcj1 merged commit a0dcd39 into main Jun 1, 2026
123 of 128 checks passed
@rcj1 rcj1 deleted the copilot/implement-dacdbi-api branch June 1, 2026 20:57
@noahfalk

noahfalk commented Jun 1, 2026

Copy link
Copy Markdown
Member

@janvorli - fyi for contracts relating to GC

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants