Remove the uniprocessor (non-TLS) allocation helpers - #132665
Draft
EgorBo wants to merge 2 commits into
Draft
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. |
EgorBo
force-pushed
the
remove-notls-allocators
branch
2 times, most recently
from
August 23, 2026 00:49
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>
EgorBo
force-pushed
the
remove-notls-allocators
branch
from
August 23, 2026 00:50
545eb43 to
40d1a36
Compare
Contributor
|
Tagging subscribers to this area: @anicka-net, @dotnet/gc |
jkotas
reviewed
Aug 23, 2026
|
|
||
| #endif // FEATURE_USE_SOFTWARE_WRITE_WATCH_FOR_GC_HEAP | ||
|
|
||
| // Kept permanently zeroed for GC data contract c1 compatibility. See gcheaputilities.h. |
Member
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?
Member
Author
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 affinity don'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.g_global_alloc_contextitself stays, permanently zeroed, so that theGlobalAllocContextglobal required by versionc1of 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 #115102.