gc: collapse single-bucket handle table walks by max-charlamb · Pull Request #128718 · dotnet/runtime

max-charlamb · 2026-05-28T19:04:24Z

Follow-up to #128646. After that PR collapsed the multi-bucket / multi-AppDomain producer in Ref_Initialize and friends, the read side of g_HandleTableMap is dead scaffolding: only pBuckets[0] is ever populated, pNext is permanently NULL, and dwMaxIndex is permanently INITIAL_HANDLE_TABLE_ARRAY_SIZE.

This PR collapses those dead walks in the GC internals to single-bucket access via a tiny helper:

static FORCEINLINE HandleTableBucket* GetGlobalHandleTableBucket()
{
    return g_HandleTableMap.pBuckets != NULL ? g_HandleTableMap.pBuckets[0] : NULL;
}

Scope

src/coreclr/gc/objecthandle.cpp: ~22 walk sites collapsed; Ref_Shutdown and Ref_RemoveHandleTableBucket simplified. Ref_InitializeHandleTableBucket deliberately untouched (reachable through the IGCHandleManager::CreateHandleStore virtual; not worth re-engineering here).
src/coreclr/gc/handletable.cpp: HndCountAllHandles collapsed. Reads g_HandleTableMap.pBuckets[0] directly -- same source of truth the DAC sees -- to avoid a layering inversion through GCHandleStore.

Non-goals

No GC<->DAC ABI change. gcDacVars->handle_table_map and the dac_handle_table_map struct layout (pBuckets, pNext, dwMaxIndex) are intact. No major_version_number bump.
No DAC or cDAC changes. The walking patterns in daccess.cpp (DacHandleWalker and the SOS memory region collector) and in cDAC GC_1 (GetHandles, GetHandleTableMemoryRegions) are left untouched. Those collapses are mechanically straightforward (~50 lines net) and can land in a separate follow-up if reviewers want them.

Why this is safe

Multi-bucket has had no producer in coreclr since #24536 ("Remove concept of AppDomains from the GC", May 2019) -- almost 4 years before the most recent GC DAC major_version_number bump (1 -> 2 in #84454, April 2023). Any standalone GC targeting the current ABI cannot have legitimately produced a multi-bucket layout: the AppDomain semantics it would have modeled aren't exposed by coreclr anymore.

Validation

build.cmd clr.runtime -c Checked: clean.
No behavior change: every walk site now visits exactly the single bucket it always visited in practice.

Note

This PR was drafted with assistance from GitHub Copilot.

Follow-up to dotnet#128646. After that PR the multi-bucket / multi-AppDomain handle table scaffolding has no producer: only g_HandleTableMap.pBuckets[0] is ever populated, pNext is permanently null, and dwMaxIndex is permanently the array size constant. Collapse the dead walk patterns in objecthandle.cpp and the one in handletable.cpp to single-bucket form via a small helper. No DAC/cDAC ABI change -- the on-disk HandleTableMap layout is preserved for gcDacVars->handle_table_map and the cDAC GC_1 reader. The walking patterns in legacy DAC (daccess.cpp) and cDAC (GC_1.cs) are left alone in this change and can be simplified in a separate follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

dotnet-policy-service · 2026-05-28T19:05:35Z

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

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

janvorli · 2026-05-28T19:13:35Z

-    struct HandleTableMap * walk = &g_HandleTableMap;
+    // After collapsing the legacy multi-bucket / multi-AppDomain handle-table-map
+    // scaffolding, there is at most a single bucket slot at index 0.
+    HandleTableBucket* bucket = g_HandleTableMap.pBuckets != NULL ? g_HandleTableMap.pBuckets[0] : NULL;


What is the benefit of keeping the bucket array with single entry instead of just a pointer to a bucket?

Removing the top level HandleTableBucket would break the GC<->EE API, we can do this but it would require a major GC version bump.

I guess we could get rid of it on the GC side entirely and fake it on the interface, but this also seems confusing. My thought was to simplify what we can then remove it entirely the next time we do a major GC version bump.

I am not sure how it would break it. The g_HandleTableMap seems to be accessed only at the GC side of things.

Sorry I mean the gcinterfacedacvars.def versioning. This pointer is accessed by the DAC and would require shimming or a break in the supported GC versions

I see three options:

Break and bump major to 3. Cleanest end state, but it severs compat with every older standalone GC / older DAC pairing. The last break was 3 years ago so I don't really want to do another.

Shim the old slot. Keep g_HandleTableMap as a thin compatibility wrapper that still exposes the old dac_handle_table_map layout (one bucket array of length 1 pointing atg_pMainHandleTableBucket, pNext = nullptr, dwMaxIndex = 1), and additionally add handle_table_bucket at the end of the dacvars struct behind a minor bump. New DAC/cDAC read the new field; old DACs keep working against the shim. Most backcompat-preserving, modest extra code.

Drop the GC-side refactor entirely and keep the change scoped to DAC/cDAC walk simplification only (treat the existing map as effectively single-bucket on the read side). Zero compat risk, but loses the GC simplification.

Ah, that makes sense. Let's keep your change as is then.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+            // this is the one of Ref_* function performed by single thread in MULTI_HEAPS case, so we need to loop through all HT of the bucket
+            for (int uCPUindex=0; uCPUindex < getNumberOfSlots(); uCPUindex++)
            {
-                  // this is the one of Ref_* function performed by single thread in MULTI_HEAPS case, so we need to loop through all HT of the bucket
-                for (int uCPUindex=0; uCPUindex < getNumberOfSlots(); uCPUindex++)
-                {
-                   HHANDLETABLE hTable = walk->pBuckets[i]->pTable[uCPUindex];
-                    if (hTable)
-                        HndScanHandlesForGC(hTable, VariableTraceDispatcher,
-                                        lp1, (uintptr_t)&info, &type, 1, condemned, maxgen, HNDGCF_EXTRAINFO | flags);
-                }
+               HHANDLETABLE hTable = bucket->pTable[uCPUindex];
+                if (hTable)


github-actions · 2026-07-19T10:14:24Z

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "d0471714380924646dda8da4aeb1890f13141720",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "b2647f6fc4bf5a51e026f7dc880c9685b0947d13",
  "last_reviewed_commit": "d0471714380924646dda8da4aeb1890f13141720",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "b2647f6fc4bf5a51e026f7dc880c9685b0947d13",
  "last_recorded_worker_run_id": "29682994733",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "d0471714380924646dda8da4aeb1890f13141720",
      "review_id": 4730589433
    }
  ]
}

github-actions

Holistic Review

Motivation: This is a follow-up to #128646, which removed the last producer of a multi-bucket / multi-AppDomain g_HandleTableMap layout in the GC handle-table code. Since coreclr removed AppDomain semantics from the GC in #24536 (May 2019), only g_HandleTableMap.pBuckets[0] is ever populated, pNext is permanently NULL, and dwMaxIndex is permanently INITIAL_HANDLE_TABLE_ARRAY_SIZE. The read side of the map was therefore dead scaffolding, and this PR removes it.

Approach: A small FORCEINLINE helper, GetGlobalHandleTableBucket(), returns g_HandleTableMap.pBuckets != NULL ? g_HandleTableMap.pBuckets[0] : NULL. Roughly 22 while (walk) { for (i in INITIAL_HANDLE_TABLE_ARRAY_SIZE) if (pBuckets[i]) ... walk = walk->pNext; } sites in objecthandle.cpp are collapsed to a single if (bucket != NULL) over that one bucket, and Ref_Shutdown / Ref_RemoveHandleTableBucket are simplified accordingly. handletable.cpp's HndCountAllHandles is collapsed to read pBuckets[0] directly. The on-disk HandleTableMap / HandleTableBucket DAC layout is deliberately preserved (the static_asserts at the bottom of objecthandle.cpp still hold), and the DAC/cDAC walking code is intentionally left for a separate follow-up. Ref_InitializeHandleTableBucket is intentionally untouched.

Summary: This is a faithful, mechanical simplification. Each rewritten site visits exactly the single bucket (and its getNumberOfSlots() slots) it always visited in practice, so there is no behavior change on the current-ABI configuration, and the loop-body scanning logic (slot striding via getSlotNumber/getThreadCount, lock acquisition in HndCountAllHandles, the single-thread MULTI_HEAPS loops) is preserved verbatim. The Ref_RemoveHandleTableBucket rewrite correctly reduces the old index-range search to HandleTableIndex == 0 && pBuckets[0] == pBucket, matching how buckets are assigned in Ref_InitializeHandleTableBucket (index always 0). The NULL-map guards (pBuckets != NULL) added throughout are appropriate for shutdown ordering. No GC<->DAC ABI change and no major_version_number bump, consistent with the preserved struct layout. The safety argument for dropping multi-bucket support is sound: no producer has existed since 2019, well before the last GC DAC ABI bump.

Verdict: LGTM. The change is a low-risk, self-consistent dead-code removal with clear rationale, and the deliberate deferral of the DAC/cDAC collapses is reasonable and clearly documented. I found no actionable correctness, performance, or convention issues in the changed lines.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 93.8 AIC · ⌖ 10.7 AIC · ⊞ 10K

Copilot AI review requested due to automatic review settings May 28, 2026 19:04

Copilot started reviewing on behalf of max-charlamb May 28, 2026 19:04 View session

github-actions Bot added the area-GC-coreclr label May 28, 2026

dotnet-policy-service Bot assigned max-charlamb May 28, 2026

max-charlamb added the NO-REVIEW Experimental/testing PR, do NOT review it label May 28, 2026

Copilot AI reviewed May 28, 2026

janvorli reviewed May 28, 2026

View reviewed changes

This was referenced May 29, 2026

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

XHarness package install failure on iOS due to devicectl NSPOSIXErrorDomain error 49 #123796

Open

max-charlamb removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jun 4, 2026

Simplify comments per review feedback

f8a05bb

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

max-charlamb marked this pull request as ready for review June 4, 2026 17:04

Copilot AI review requested due to automatic review settings June 4, 2026 17:04

Copilot started reviewing on behalf of max-charlamb June 4, 2026 17:05 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread src/coreclr/gc/objecthandle.cpp

Comment thread src/coreclr/gc/objecthandle.cpp

Comment thread src/coreclr/gc/objecthandle.cpp

Comment thread src/coreclr/gc/handletable.cpp

build-analysis Bot mentioned this pull request Jun 5, 2026

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

max-charlamb requested a review from rcj1 June 22, 2026 16:31

max-charlamb requested a review from kkokosa July 13, 2026 20:56

Merge branch 'main' into gc/collapse-handle-table-walks

d047171

Copilot AI review requested due to automatic review settings July 14, 2026 14:26

Copilot started reviewing on behalf of max-charlamb July 14, 2026 14:27 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

github-actions Bot reviewed Jul 19, 2026

View reviewed changes

Uh oh!

Conversation

max-charlamb commented May 28, 2026

Scope

Non-goals

Why this is safe

Validation

Uh oh!

dotnet-policy-service Bot commented May 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

janvorli May 28, 2026

Choose a reason for hiding this comment

Uh oh!

max-charlamb May 28, 2026

Choose a reason for hiding this comment

Uh oh!

janvorli May 28, 2026

Choose a reason for hiding this comment

Uh oh!

max-charlamb Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

janvorli Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Holistic Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jul 19, 2026 •

edited

Loading