Skip to content

Memory Diagnostics: GC Bookkeeping - #84454

Merged
leculver merged 27 commits into
dotnet:mainfrom
leculver:memoryRegionEnumeration
Apr 17, 2023
Merged

Memory Diagnostics: GC Bookkeeping#84454
leculver merged 27 commits into
dotnet:mainfrom
leculver:memoryRegionEnumeration

Conversation

@leculver

@leculver leculver commented Apr 6, 2023

Copy link
Copy Markdown
Contributor

Adds a way for debuggers to tag GC related memory as owned by the CLR.

As part of our ongoing work to improve (and fix) native and managed memory diagnostics (dotnet/diagnostics#3736) we need the debugger to be able to find GC-related memory that's been VirtualAlloc'ed. We are specifically seeing a lot of investigations coming in where being able to know what memory regions account for GC bookkeeping data will let us quickly eliminate (or blame) the GC in memory related investigations.

GC related changes:

  • The goal is to expose enough information to the dac to be able to enumerate the bounds of the HandleTable, GCBookkeeping structures, and all free regions which have not yet been released.
  • I tried to keep HandleTable changes to a minimum, so I've rearranged the header structure so that we don't have to reorganize HandleTable fields. I still had to move HandleTable::pSegmentList to be before the critical section though.
  • Exposes card_table_info to the dac so we can get the total size reserved and the link to the next region.
  • Changes bookkeeping_covered_start to be present in all cases (not just when USE_REGIONS is defined...even though it's not needed for anything other than diagnostics when not using regions). This is the actual memory base we get back from VirtualAlloc, so along with card_table_info we know the entire range of memory that bookkeeping data will fall within.
  • Exposes all free regions to the dac (global_free_huge_regions, free_regions, and global_regions_to_decommit), as the dac currently has no way to get that information before this change. This will let us tag allocated, but not currently used, GC Regions in the debugger.

Dac/SOS related changes:

  • Added a function to call Flush() while under the dac lock. The lack of this is particularly painful for ClrMD, which calls into the dac on multiple threads. (This is unrelated to the rest of the changes.)
  • Added ISOSMemoryEnum - Enumerates regions of memory.
  • Added GetHandleTableMemoryRegions - enumerates the exact bounds of HandleTableSegments.
  • Added GetGCFreeRegions enumerates the address of regions which have been freed but not released. The addresses from this can be passed to GetSegmentData to find the regions' bounds.
  • Added GetGCBookkeepingMemoryRegions enumerates the entire region of memory where GC Bookkeeping data could live. The GC reserves a huge chunk of memory, and only parts of it are committed. The range of memory enumerated by this is the reserved area. Drilling down into specific committed segments requires combining this data with !address or VirtualQuery.

Manual testing looks good so far, so I'm putting this up for code review. I'm marking it NO-MERGE until I've completed more extensive testing.

@leculver leculver added enhancement Product code improvement that does NOT require public API changes/additions NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Diagnostics-coreclr area-GC-coreclr labels Apr 6, 2023
@leculver
leculver requested review from Maoni0, cshung and mikem8361 April 6, 2023 23:38
@ghost ghost assigned leculver Apr 6, 2023
@ghost

ghost commented Apr 6, 2023

Copy link
Copy Markdown

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

Issue Details

Adds a way for debuggers to tag GC related memory as owned by the CLR.

As part of our ongoing work to improve (and fix) native and managed memory diagnostics (dotnet/diagnostics#3736) we need the debugger to be able to find GC-related memory that's been VirtualAlloc'ed. We are specifically seeing a lot of investigations coming in where being able to know what memory regions account for GC bookkeeping data will let us quickly eliminate (or blame) the GC in memory related investigations.

GC related changes:

  • The goal is to expose enough information to the dac to be able to enumerate the bounds of the HandleTable, GCBookkeeping structures, and all free regions which have not yet been released.
  • I tried to keep HandleTable changes to a minimum, so I've rearranged the header structure so that we don't have to reorganize HandleTable fields.
  • Exposes card_table_info to the dac so we can get the total size reserved and the link to the next region.
  • Changes bookkeeping_covered_start to be present in all cases (not just when USE_REGIONS is defined...even though it's not needed for anything other than diagnostics when not using regions). This is the actual memory base we get back from VirtualAlloc, so along with card_table_info we know the entire range of memory that bookkeeping data will fall within.
  • Exposes all free regions to the dac (global_free_huge_regions, free_regions, and global_regions_to_decommit), as the dac currently has no way to get that information before this change. This will let us tag allocated, but not currently used, GC Regions in the debugger.

Dac/SOS related changes:

  • Added a function to call Flush() while under the dac lock. The lack of this is particularly painful for ClrMD, which calls into the dac on multiple threads. (This is unrelated to the rest of the changes.)
  • Added ISOSMemoryEnum - Enumerates regions of memory.
  • Added GetHandleTableMemoryRegions - enumerates the exact bounds of HandleTableSegments.
  • Added GetGCFreeRegions enumerates the address of regions which have been freed but not released. The addresses from this can be passed to GetSegmentData to find the regions' bounds.
  • Added GetGCBookkeepingMemoryRegions enumerates the entire region of memory where GC Bookkeeping data could live. The GC reserves a huge chunk of memory, and only parts of it are committed. The range of memory enumerated by this is the reserved area. Drilling down into specific committed segments requires combining this data with !address or VirtualQuery.

Manual testing looks good so far, so I'm putting this up for code review. I'm marking it NO-MERGE until I've completed more extensive testing.

Author: leculver
Assignees: -
Labels:

enhancement, NO-MERGE, area-Diagnostics-coreclr, area-GC-coreclr

Milestone: -

@leculver leculver removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 8, 2023
@leculver

leculver commented Apr 8, 2023

Copy link
Copy Markdown
Contributor Author

This is now fully tested with USE_REGIONS and with segments on both x86 and x64. It all works great! So, this checkin is ready to merge (and ready for review!), but I'm expecting some spirited rounds of debates on some of the changes. =)

I attempted to loosen the requirement of pulling in HandleTable constants to the GC/Dac interface by moving HandleTableSegment::pNextSegment to the front of that class/structure. I found that this quickly causes asserts and failures because things take a hard dependency on the layout of handle table segments. If there's pushback to taking a dependency on HANDLE_BLOCKS_PER_SEGMENT/HANDLE_MASKS_PER_SEGMENT like we (already) did with HANDLE_MAX_INTERNAL_TYPES then I can dig in further here.

Comment thread src/coreclr/inc/sospriv.idl Outdated
Comment thread src/coreclr/debug/daccess/request.cpp Outdated
@Maoni0

Maoni0 commented Apr 10, 2023

Copy link
Copy Markdown
Member

@cshung could you please CR this?

@leculver could you please tell me what kind of testing you've done for this?

this is only applicable for regions, yes? if so we should make the clear.

@leculver

Copy link
Copy Markdown
Contributor Author

This is applicable for both regions and segments. I've tested both and it works.

For testing, I've done extensive testing of the new debugging APIs for x86 and x64 (which covers Segments and Regions). I set up several small repro programs to get free Regions, as well as testing the multiple card_table case for Segments.

I have not done any GC specific testing. If you all could provide some instructions on GC testing I'd be happy to go do that. Otherwise, it's passing standard checkin tests.

@leculver

Copy link
Copy Markdown
Contributor Author

@cshung This is ready for final testing/review. I've tested this on x86/x64 (segments/regions) with both workstation and server GC. I was able to set breakpoints in strategic places in the gc to test the various pieces of this change. I also confirmed that the total memory that ClrMD/SOS now reports is equal to the sizes saved by the GC when a hard limit is set.

Let me know if you have any other questions, feedback, or if you'd like me to do any other testing. Otherwise this is all ready for final review and checkin whenever you are. Thanks!

@leculver leculver closed this Apr 14, 2023
@leculver leculver reopened this Apr 14, 2023
@leculver
leculver force-pushed the memoryRegionEnumeration branch from b4fb793 to 59c57f0 Compare April 15, 2023 02:08

@cshung cshung 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.

In general, this change is great, thanks for doing this.

I have made some suggested changes here to the GC code to do some renaming, can you please incorporate the changes?

On that change, I ran the code over the GC infrastructure for validation and found no bugs or regressions. You can see the test report there.

Other comments listed here are just nice to have and non-blocking, this is good to go.

Comment thread src/coreclr/debug/daccess/daccess.cpp
Comment thread src/coreclr/debug/daccess/daccess.cpp
Comment thread src/coreclr/debug/daccess/daccess.cpp
Comment thread src/coreclr/debug/daccess/request_svr.cpp
- bookkeeping_covered_start is now compiled into all versions of the GC, instead of just with USE_REGIONS.  This allows us to find the base address of the allocated memory for GC Bookkeeping.
- Added dac enumeration of GC Bookkeeping.
We should never have missing fields in the dac.  This is a leftover from
previous code.
@leculver
leculver force-pushed the memoryRegionEnumeration branch from 49e28be to 9abf2f3 Compare April 17, 2023 19:05
@leculver
leculver merged commit add51b8 into dotnet:main Apr 17, 2023
@leculver
leculver deleted the memoryRegionEnumeration branch April 17, 2023 21:10
@ghost ghost locked as resolved and limited conversation to collaborators May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants