Skip to content

Remove the uniprocessor (non-TLS) allocation helpers - #132665

Merged
EgorBo merged 8 commits into
dotnet:mainfrom
EgorBo:remove-notls-allocators
Aug 28, 2026
Merged

EgorBo merged 8 commits into
dotnet:mainfrom
EgorBo:remove-notls-allocators

Conversation

@EgorBo

@EgorBo EgorBo commented Aug 23, 2026

Copy link
Copy Markdown
Member

Removes the Windows x86/x64 uniprocessor allocation helpers (RhpNewFast_UP and friends), which allocate from a single global ee_alloc_context under a hand-rolled spin lock instead of the thread-local one, purely to skip the TLS access.

  • Barely reachable. It needs GetSystemInfo() to report exactly one processor, plus workstation GC and no CPU groups. That check never consults GetCurrentProcessCpuCount(), so container CPU limits (--cpus=1, job-object rate control) and process affinity don't trigger it - only a genuinely 1-vCPU machine does.
  • Known broken. GCStress is unsafe on this path and can corrupt the heap - GCStress is not safe when not using per-thread allocation contexts #7670, filed in 2017, never fixed (the backlog bot eventually closed it), papered over with assert(UseThreadAllocationContexts()) in four places. RuntimeTypeHandle::InternalAllocNoChecks_FastPath also just bails out when it's active.
  • Untested. GCUseGlobalAllocationContext is #if DEBUG only and set nowhere in the repo, and no Windows CI queue is single-core (all helix windows images are 2-4 cores).
  • Not free. ~370 lines of hand-written x86/x64 asm, a lock whose asm half uses a non-atomic inc, and special cases in alloc context enumeration, GCStress and suspension.
  • Little to gain. The TLS access it avoids is 5 inline instructions on x64 and 4 on x86 (INLINE_GET_ALLOC_CONTEXT_BASE) - and it replaces them with a read-modify-write on a shared global plus an unlock store.

Everything now goes through the thread allocation context. GCUseGlobalAllocationContext is removed as well.

Diagnostics

g_global_alloc_context goes away with the helpers, so the runtime no longer exports the GlobalAllocContext data-descriptor global. The APIs built on it stay and report an empty context, which is what the runtime always has:

  • ISOSDacInterface12::GetGlobalAllocationContext (both the DAC and the cDAC) and IGC.GetGlobalAllocationContext return null / null, and heap walking no longer carves out a global context.
  • The cDAC's GC c1 implementation reads the global with TryReadGlobalPointer, the same way it already reads the other conditionally emitted GC globals (GlobalFreeHugeRegions, GCHeapMarkArray, CurrentGCState, ...), so it still reports a real context when the target does define it - e.g. dumps from earlier .NET 11 previews.
  • GC.md documents the global as conditional and the API as returning nulls when it is absent.

That makes this an amendment of GC c1 rather than a new contract version. datacontracts_design.md allows contract versions to be amended, and only freezes the supported set from a major release's first RC onwards. The GC contract is new in .NET 11 - it does not exist in release/10.0, which is why the GC dump tests carry [SkipOnVersion("net10.0", ...)] - and GlobalAllocContext itself was only added in February by #124805, so no shipped runtime advertises a GC contract at all. If the diagnostics team would rather not touch c1, the alternative is a c2 that drops the global.

Follow-up to #115102.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@EgorBo
EgorBo force-pushed the remove-notls-allocators branch 2 times, most recently from 12e2b02 to 545eb43 Compare August 23, 2026 00:49
Removes the Windows x86/x64 uniprocessor allocation helpers
(RhpNewFast_UP and friends), which allocate from a single global
ee_alloc_context under a hand-rolled spin lock instead of the
thread-local one, purely to skip the TLS access.

* Barely reachable. It needs GetSystemInfo() to report exactly one
  processor, plus workstation GC and no CPU groups. That check never
  consults GetCurrentProcessCpuCount(), so container CPU limits
  (--cpus=1, job-object rate control) and process affinity don't trigger
  it - only a genuinely 1-vCPU machine does.
* Known broken. GCStress is unsafe on this path and can corrupt the heap
  (dotnet#7670, filed in 2017, never fixed), papered over with
  assert(UseThreadAllocationContexts()) in four places.
  RuntimeTypeHandle::InternalAllocNoChecks_FastPath also just bails out
  when it's active.
* Untested. GCUseGlobalAllocationContext is #if DEBUG only and set
  nowhere in the repo, and no Windows CI queue is single-core.
* Not free. ~370 lines of hand-written x86/x64 asm, a lock whose asm half
  uses a non-atomic inc, and special cases in alloc context enumeration,
  GCStress and suspension.
* Little to gain. The TLS access it avoids is 5 inline instructions on
  x64 and 4 on x86 (INLINE_GET_ALLOC_CONTEXT_BASE) - and it replaces them
  with a read-modify-write on a shared global plus an unlock store.

Everything now goes through the thread allocation context.
GCUseGlobalAllocationContext is removed as well.

g_global_alloc_context itself stays, permanently zeroed, so that the
GlobalAllocContext global required by version c1 of the GC data contract
keeps resolving. Diagnostics are otherwise untouched: DAC, cDAC and SOS
now consistently report an empty global allocation context, which is what
the runtime always has.

Follow-up to dotnet#115102.

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

Copy link
Copy Markdown
Contributor

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

Comment thread src/coreclr/vm/gcheaputilities.cpp Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: fa75de3f-c6b8-4734-b051-8d32f405a6b7
@EgorBo
EgorBo marked this pull request as ready for review August 24, 2026 11:09
Copilot AI lite review requested due to automatic review settings August 24, 2026 11:09
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

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 pull request removes obsolete Windows x86/x64 uniprocessor allocation helpers and standardizes allocation on thread-local contexts.

Changes:

  • Deletes uniprocessor helpers, locking, configuration, and related runtime branches.
  • Updates GC, DAC/cDAC/SOS behavior, tests, and contract documentation.
  • Represents the global allocation context as empty.

Critical findings remain:

  • datadescriptor.inc: removing GlobalAllocContext from the c1 schema conflicts with retaining the zeroed symbol and affects existing cDAC readers. Critical, 1 vote.
  • GC_1.cs: making GlobalAllocContext optional is incompatible with the advertised c1 contract. Critical, 3 votes.

Reviewed changes

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

Show a summary per file
File Summary
src/native/managed/cdac/tests/DumpTests/WorkstationGCDumpTests.cs Tests an empty global allocation context.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GC/GC_1.cs Critical, 3 votes: Making the c1 global context optional is incompatible with existing contract readers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IGC.cs Documents empty-context semantics.
src/coreclr/vm/threadsuspend.cpp Removes the obsolete allocation-context assertion.
src/coreclr/vm/runtimehandles.cpp Removes the global-context fast-path fallback.
src/coreclr/vm/jitinterfacegen.cpp Selects thread-context helpers unconditionally.
src/coreclr/vm/jitinterface.h Removes uniprocessor helper declarations.
src/coreclr/vm/i386/jitinterfacex86.cpp Removes the global-lock declaration.
src/coreclr/vm/i386/AllocSlow.asm Deletes x86 uniprocessor helpers.
src/coreclr/vm/gcstress.h Removes the obsolete GCStress assertion.
src/coreclr/vm/gchelpers.cpp Routes allocations through thread contexts.
src/coreclr/vm/gcheaputilities.h Removes global-context declarations and mode APIs.
src/coreclr/vm/gcheaputilities.cpp Removes global-context initialization logic.
src/coreclr/vm/gcenv.ee.cpp Enumerates thread allocation contexts.
src/coreclr/vm/gccover.cpp Removes obsolete global-context GCStress assertions.
src/coreclr/vm/datadescriptor/datadescriptor.inc Critical, 1 vote: Removing the c1 descriptor conflicts with the retained zeroed symbol and existing cDAC compatibility.
src/coreclr/vm/amd64/AllocSlow.asm Deletes AMD64 uniprocessor helpers.
src/coreclr/inc/dacvars.h Removes the obsolete DAC variable.
src/coreclr/inc/clrconfigvalues.h Removes the obsolete configuration knob.
src/coreclr/debug/daccess/request.cpp Reports an empty global context.
src/coreclr/debug/daccess/dacdbiimpl.cpp Removes global-context heap walking.
docs/design/datacontracts/GC.md Documents optional global-context support.
docs/design/datacontracts/data-descriptor-meanings.json Updates global-context metadata.
Suppressed comments (1)

src/coreclr/vm/jitinterfacegen.cpp:43

  • On the Windows x86/x64 single-processor workstation configuration, this changes every fast allocation from the dedicated non-TLS helpers to RhpNewFast/the regular array helpers, adding the TLS access that the removed path specifically avoided. The PR description gives instruction counts but no measured comparison for this supported configuration; please add a baseline/changed benchmark or explicitly document the accepted hot-path regression before removing the optimization.
        SetJitHelperFunction(CORINFO_HELP_NEWSFAST, RhpNewFast);
        SetJitHelperFunction(CORINFO_HELP_NEWARR_1_VC, RhpNewArrayFast);
        SetJitHelperFunction(CORINFO_HELP_NEWARR_1_PTR, RhpNewPtrArrayFast);

Comment thread src/coreclr/vm/datadescriptor/datadescriptor.inc
- Match the wording convention used by the other conditionally emitted
  GC globals (CurrentGCState, DynamicAdaptationMode) when documenting
  GlobalAllocContext as no longer emitted.
- Drop the now-vestigial dwNumberOfProcessors assert in
  InitJITAllocationHelpers - its only consumer was the removed
  uniprocessor helper selection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 24, 2026 11:41
@janvorli

Copy link
Copy Markdown
Member

This also fixes #10547

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

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

Suppressed comments (4)

src/coreclr/debug/daccess/request.cpp:5504

  • The native DAC now always reports zero here, but the cDAC path intentionally returns a real global context when reading an earlier target that still defines GlobalAllocContext. In DEBUG, SOSDacImpl compares both results and asserts equality, so using this current DAC with such a target can fail the cross-validation assertion even though cDAC successfully read the target. Make the legacy and cDAC paths use the same target-conditional behavior, or avoid cross-validating this incompatible case.

[!NOTE] This review comment was generated by GitHub Copilot.

    *allocPtr = (CLRDATA_ADDRESS)0;
    *allocLimit = (CLRDATA_ADDRESS)0;

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GC/GC_1.cs:296

  • This compatibility branch is the only support for dumps that still expose GlobalAllocContext, but the added dump test only exercises the absent-global case. Please add a contract-level test with the existing EEAllocContext mock descriptor and a present GlobalAllocContext global, asserting both the returned range and the heap-walking allocation-context path; otherwise regressions in this intentionally retained older-dump behavior will go unnoticed.

[!NOTE] This review comment was generated by GitHub Copilot.

        if (!_target.TryReadGlobalPointer(Constants.Globals.GlobalAllocContext, out TargetPointer? globalAllocContextAddress))

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GC/GC_1.cs:300

  • This removes GlobalAllocContext from the target descriptor while the GC contract is still advertised as c1. An original c1 reader unconditionally calls ReadGlobalPointer("GlobalAllocContext"), so it throws against this runtime instead of observing the documented empty context, violating the compatibility requirement in docs/design/datacontracts/datacontracts_design.md:64. Keep the c1 global resolvable with a zero value, or introduce/use a new GC contract version for the removal.

[!NOTE] This review comment was generated by GitHub Copilot.

        if (!_target.TryReadGlobalPointer(Constants.Globals.GlobalAllocContext, out TargetPointer? globalAllocContextAddress))
        {
            allocPtr = TargetPointer.Null;
            allocLimit = TargetPointer.Null;
            return;

src/native/managed/cdac/tests/DumpTests/WorkstationGCDumpTests.cs:143

  • Only the absent-global path is exercised here. The compatibility path in GC_1.GetGlobalAllocationContext—a target that still defines GlobalAllocContext and has readable nested Pointer/Limit—is not covered, so the advertised pre-removal dump support can regress while this test remains green. Add a mock-target unit test for both branches.

[!NOTE]
AI/Copilot-generated review comment.

        Assert.Equal(TargetPointer.Null, pointer);
        Assert.Equal(TargetPointer.Null, limit);

Comment thread src/coreclr/debug/daccess/request.cpp
Comment thread src/native/managed/cdac/tests/DumpTests/WorkstationGCDumpTests.cs Outdated
EgorBo and others added 2 commits August 25, 2026 19:26
Reverts the removal of g_global_alloc_context and of the GlobalAllocContext
data-descriptor global. Version c1 of the GC data contract requires that
global, and its documented GetGlobalAllocationContext algorithm reads it
unconditionally, so dropping it is a breaking contract change - not an
amendment - even though no shipped tool depends on it yet.

The global stays permanently zeroed, which reads back as an empty allocation
context: exactly what the runtime now always has. DAC, cDAC, SOS, the GC.md
contract description and the dump tests are therefore all unchanged from main,
while every runtime use of the global (allocation, alloc context enumeration,
GC stress, suspension) is still gone with the uniprocessor helpers.

It can be deleted the next time the GC contract takes a breaking change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 25, 2026 17:38

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

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comment thread src/coreclr/vm/gcheaputilities.h Outdated
Addresses review feedback: keep the unused global for c1 compatibility, but
make the future cleanup trivial and discoverable.

- g_global_alloc_context keeps a one-line comment pointing at the cleanup list.
- GC.md and data-descriptor-meanings.json document GlobalAllocContext as
  required in c1 and optional in c2 and later, and GetGlobalAllocationContext
  reports an empty context when the global is absent.
- GC_1 reads the global with TryReadGlobalPointer, so the same implementation
  works against both versions.
- datadescriptor.inc gets a "Pending cleanup" list tracking entries that can
  only be deleted with a contract version bump.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 27, 2026 13:39
Comment thread src/coreclr/vm/datadescriptor/datadescriptor.inc Outdated
@jkotas

jkotas commented Aug 27, 2026

Copy link
Copy Markdown
Member

Can the static be deleted from the fragile DAC in this PR?

src\coreclr\debug\daccess\dacdbiimpl.cpp(6671):gc_alloc_context globalCtx = ((ee_alloc_context)g_global_alloc_context).m_GCAllocContext;
src\coreclr\debug\daccess\request.cpp(5501):gc_alloc_context global_alloc_context = ((ee_alloc_context)g_global_alloc_context).m_GCAllocContext;

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

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/coreclr/vm/datadescriptor/datadescriptor.inc:1796

  • The PR description says the runtime will no longer export the GlobalAllocContext data-descriptor global, but this file still defines CDAC_GLOBAL_POINTER(GlobalAllocContext, &::g_global_alloc_context) (and g_global_alloc_context is still present). Either the description needs updating to reflect that the global remains exported (but is unused/zeroed), or the code needs to remove this descriptor as part of a GC contract version bump.
// Unused by the runtime - see "Pending cleanup" at the end of this file.
CDAC_GLOBAL_POINTER(GlobalAllocContext, &::g_global_alloc_context)

Comment thread docs/design/datacontracts/GC.md
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Copilot AI review requested due to automatic review settings August 27, 2026 16:36

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

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GC/GC_1.cs:296

  • The comment claims that runtimes which allocate only out of thread allocation contexts "do not export the global", but this PR keeps exporting GlobalAllocContext via datadescriptor.inc (and keeps ::g_global_alloc_context for c1 compatibility). Please reword the comment to avoid implying the global is guaranteed to be missing in this runtime configuration.
        // Runtimes which allocate only out of thread allocation contexts do not export the global.
        // It is required in c1 and optional in c2 and later.

docs/design/datacontracts/GC.md:889

  • In the pseudocode, globalAllocContextAddress is declared as TargetPointer? but later used in pointer arithmetic without .Value. This makes the example inconsistent with the actual implementation and could confuse readers copying it.
    // "GlobalAllocContext" is required in c1 and optional in c2 and later: runtimes which
    // allocate only out of thread allocation contexts do not define it. Report an empty
    // context in that case, so that the same implementation serves both versions.
    if (!target.TryReadGlobalPointer("GlobalAllocContext", out TargetPointer? globalAllocContextAddress))
    {

The runtime no longer allocates out of the global allocation context, so the
DAC does not need to read it any more:

- DacHeapWalker no longer reserves an extra AllocInfo slot for it and no
  longer carves it out of the heap walk.
- ClrDataAccess::GetGlobalAllocationContext reports an empty context directly.

The global itself, its data descriptor entry and its dacvars.h entry all stay.
The dacvars.h entry is what reports the global into mini and triage dumps via
enummem.cpp, and the cDAC still reads it there for the GC contract c1
GlobalAllocContext global, so removing it would make GetGlobalAllocationContext
and cDAC heap walking fail on those dump types. Both are noted in the pending
cleanup list so they can go away together when the GC contract is bumped to c2.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c7a32a7b-9ced-4b78-9f0b-6520af0a00f2
Copilot AI review requested due to automatic review settings August 27, 2026 17:05

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/coreclr/vm/datadescriptor/datadescriptor.inc:1796

  • The PR description says the runtime no longer exports the GlobalAllocContext data-descriptor global, but datadescriptor.inc still defines CDAC_GLOBAL_POINTER(GlobalAllocContext, &::g_global_alloc_context) (now marked unused). Please reconcile: either update the PR description to reflect that the entry is retained (but empty/unused) for c1 compatibility, or remove it as part of a GC contract bump (c2+) if the goal is to stop exporting it.
// Unused by the runtime - see "Pending cleanup" at the end of this file.
CDAC_GLOBAL_POINTER(GlobalAllocContext, &::g_global_alloc_context)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/GC/GC_1.cs:296

  • The comment implies that runtimes allocating only out of thread allocation contexts never export GlobalAllocContext, but this PR keeps the global/data-descriptor for GC contract c1 compatibility (it’s just unused/empty). Rewording the comment avoids misleading future maintainers about when the global might be absent.
        // Runtimes which allocate only out of thread allocation contexts do not export the global.
        // It is required in c1 and optional in c2 and later.

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

LGTM

@max-charlamb and @noahfalk Could you please sign-off as well?

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

cDAC changes look good to me but I'd like to make sure @noahfalk is on board with this versioning 'todo' scheme.

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

Looks good to me 👍

(I did consider suggesting an alternate scheme like using an ifdef CDAC_BREAKING_CHANGES but my conclusion was unless the amount of cruft actually starts getting large our choice of tracking mechanism probably doesn't matter too much. Comment in the file is simple and can be connected back to this PR if more context is needed)

@EgorBo
EgorBo enabled auto-merge (squash) August 28, 2026 11:48
@EgorBo

EgorBo commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

/ba-g build-analysis is stuck

@EgorBo
EgorBo merged commit fdb42ef into dotnet:main Aug 28, 2026
135 checks passed
@EgorBo
EgorBo deleted the remove-notls-allocators branch August 28, 2026 11:49
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Aug 29, 2026
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.

7 participants