Remove the uniprocessor (non-TLS) allocation helpers - #132665
Conversation
|
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. |
12e2b02 to
545eb43
Compare
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>
545eb43 to
40d1a36
Compare
|
Tagging subscribers to this area: @anicka-net, @dotnet/gc |
|
|
||
| #endif // FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP | ||
|
|
||
| // Kept permanently zeroed for GC data contract c1 compatibility. See gcheaputilities.h. |
There was a problem hiding this comment.
I do not think we want to be keeping cruft in the runtime to avoid versioning the contracts.
I am wondering whether we need to actually the contract just to delete some fields. We only need to change some fields in the contract to be optional. Is it ok to do that without bumping the version ID of the contract?
@max-charlamb @dotnet/dotnet-diag Thoughts?
There was a problem hiding this comment.
Pushed a commit that removes it 11797ce can revert it if we want to keep the compatibility
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa75de3f-c6b8-4734-b051-8d32f405a6b7
|
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. |
There was a problem hiding this comment.
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: removingGlobalAllocContextfrom the c1 schema conflicts with retaining the zeroed symbol and affects existing cDAC readers. Critical, 1 vote.GC_1.cs: makingGlobalAllocContextoptional 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);
- 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>
|
This also fixes #10547 |
There was a problem hiding this comment.
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,SOSDacImplcompares 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 existingEEAllocContextmock descriptor and a presentGlobalAllocContextglobal, 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
GlobalAllocContextfrom the target descriptor while the GC contract is still advertised asc1. An originalc1reader unconditionally callsReadGlobalPointer("GlobalAllocContext"), so it throws against this runtime instead of observing the documented empty context, violating the compatibility requirement indocs/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 definesGlobalAllocContextand has readable nestedPointer/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);
Removes the Windows x86/x64 uniprocessor allocation helpers (
RhpNewFast_UPand friends), which allocate from a single globalee_alloc_contextunder a hand-rolled spin lock instead of the thread-local one, purely to skip the TLS access.GetSystemInfo()to report exactly one processor, plus workstation GC and no CPU groups. That check never consultsGetCurrentProcessCpuCount(), so container CPU limits (--cpus=1, job-object rate control)and process affinitydon't trigger it - only a genuinely 1-vCPU machine does.assert(UseThreadAllocationContexts())in four places.RuntimeTypeHandle::InternalAllocNoChecks_FastPathalso just bails out when it's active.GCUseGlobalAllocationContextis#if DEBUGonly and set nowhere in the repo, and no Windows CI queue is single-core (all helix windows images are 2-4 cores).inc, and special cases in alloc context enumeration, GCStress and suspension.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.
GCUseGlobalAllocationContextis removed as well.Diagnostics
g_global_alloc_contextgoes away with the helpers, so the runtime no longer exports theGlobalAllocContextdata-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) andIGC.GetGlobalAllocationContextreturn null / null, and heap walking no longer carves out a global context.c1implementation reads the global withTryReadGlobalPointer, 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.mddocuments the global as conditional and the API as returning nulls when it is absent.That makes this an amendment of GC
c1rather than a new contract version.datacontracts_design.mdallows 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 inrelease/10.0, which is why the GC dump tests carry[SkipOnVersion("net10.0", ...)]- andGlobalAllocContextitself was only added in February by #124805, so no shipped runtime advertises a GC contract at all. If the diagnostics team would rather not touchc1, the alternative is ac2that drops the global.Follow-up to #115102.