Skip to content

gc: collapse single-bucket handle table walks #128718

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
max-charlamb wants to merge 3 commits into
base: main
Choose a base branch
from
+297 −442
Open

gc: collapse single-bucket handle table walks #128718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c57747
gc: collapse single-bucket handle table walks
May 28, 2026
f8a05bb
Simplify comments per review feedback
Jun 4, 2026
d047171
Merge branch 'main' into gc/collapse-handle-table-walks
max-charlamb Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 19 additions & 36 deletions src/coreclr/gc/handletable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,50 +989,33 @@ uint32_t HndCountHandles(HHANDLETABLE hTable)
uint32_t HndCountAllHandles(BOOL fUseLocks)
{
uint32_t uCount = 0;
int offset = 0;

// get number of HandleTables per HandleTableBucket
int n_slots = getNumberOfSlots();

// fetch the pointer to the head of the list
struct HandleTableMap * walk = &g_HandleTableMap;
HandleTableBucket* bucket = g_HandleTableMap.pBuckets != NULL ? g_HandleTableMap.pBuckets[0] : NULL;

@janvorli janvorli May 28, 2026

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.

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

@max-charlamb max-charlamb May 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@janvorli janvorli May 28, 2026

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.

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

@max-charlamb max-charlamb Jun 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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.
  3. 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.

@janvorli janvorli Jun 3, 2026

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.

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

if (bucket == NULL)
return 0;

// walk the list
while (walk)
{
int nextOffset = walk->dwMaxIndex;
int max = nextOffset - offset;
PTR_PTR_HandleTableBucket pBucket = walk->pBuckets;
PTR_PTR_HandleTableBucket pLastBucket = pBucket + max;
// loop through the HandleTables inside the bucket and accumulate handle counts
HHANDLETABLE* pTable = bucket->pTable;
HHANDLETABLE* pLastTable = pTable + n_slots;

// loop through each slot in this node
for (; pBucket != pLastBucket; ++pBucket)
// if the 'fUseLocks' flag is set, acquire the lock for this handle table before
// calling HndCountHandles() - this will prevent dwCount from being modified and
// it will also prevent any of the main caches from being rebalanced
if (fUseLocks)
{
for (; pTable != pLastTable; ++pTable)
{
// if there is a HandleTableBucket in this slot
if (*pBucket)
{
// loop through the HandleTables inside this HandleTableBucket,
// and accumulate the handle count of each HandleTable
HHANDLETABLE * pTable = (*pBucket)->pTable;
HHANDLETABLE * pLastTable = pTable + n_slots;

// if the 'fUseLocks' flag is set, acquire the lock for this handle table before
// calling HndCountHandles() - this will prevent dwCount from being modified and
// it will also prevent any of the main caches from being rebalanced
if (fUseLocks)
for (; pTable != pLastTable; ++pTable)
{
CrstHolder ch(&(Table(*pTable)->Lock));
uCount += HndCountHandles(*pTable);
}
else
for (; pTable != pLastTable; ++pTable)
uCount += HndCountHandles(*pTable);
}
CrstHolder ch(&(Table(*pTable)->Lock));
uCount += HndCountHandles(*pTable);
}

offset = nextOffset;
walk = walk->pNext;
}
else
{
for (; pTable != pLastTable; ++pTable)
uCount += HndCountHandles(*pTable);
}

//return the total number of handles in all HandleTables
Expand Down
Loading
Loading