Async Profiler: Optimize async dispatcher allocation. - #130877
#130877Async Profiler: Optimize async dispatcher allocation.#130877lateralusX merged 4 commits intodotnet:maindotnet/runtime:mainfrom lateralusX:lateralusX/async-profiler-v1-dispatcher-optimizationlateralusX/runtime:lateralusX/async-profiler-v1-dispatcher-optimizationCopy head branch name to clipboard
Conversation
Add a profiler async state machine box used for async profiler scenarios. New box can act as async dispatcher node, merging the async dispatcher and state machine box into the same allocation, removing the need for any additional allocation in the async profiler path. Pooling scenarios is currently left untouched, but could potentially be improved in the same way in the future if needed. Smaller adjustments to make the async dispatcher wrapper more light weigth when used as fallback.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
Optimizes async-profiler instrumentation for the default (non-pooling) Task/ValueTask state-machine path by eliminating the per-suspension wrapper dispatcher allocation, instead using a profiler-aware state-machine box that can act as its own dispatcher. The change also refactors how dispatcher identity and callstack-walk state are tracked/emitted to support per-segment flattening semantics, with expanded test coverage.
Changes:
- Introduce a profiler-aware
AsyncStateMachineBox<T>subtype that implements dispatcher behavior (avoids extra dispatcher allocations when profiling is enabled). - Refactor async-profiler dispatcher state/IDs and state-machine callstack walking to support per-segment dispatcher identity and flattening.
- Extend async-profiler tests to validate sibling-walk boundaries and per-segment flattening behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Threading.Tasks.Tests/System.Runtime.CompilerServices/AsyncProfilerV1Tests.cs | Adds/extends V1 scenarios asserting sibling callstack isolation and per-segment flattening behavior. |
| src/libraries/System.Runtime/tests/System.Threading.Tasks.Tests/System.Runtime.CompilerServices/AsyncProfilerTests.cs | Adds a shared assertion helper for ensuring callstacks don’t leak “foreign” marker frames across sibling dispatchers. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs | Adjusts dispatcher creation logic in the profiler-enabled await continuation hookup path. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs | Adds the profiler-aware state-machine box and routes box allocation/MoveNext instrumentation based on async instrumentation flags. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncStateMachineDispatcher.cs | Refactors dispatcher state tracking, adds IAsyncStateMachineDispatcher, and updates create/resume/suspend/complete handling for per-segment logic. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncProfiler.cs | Updates async-profiler metadata/state, dispatcher-id retrieval, and callstack-walk leaf detection to align with the new dispatcher model. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncProfiler.CoreCLR.cs | Updates CoreCLR-side parent/dispatcher id capture to use cached dispatcher ids. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs | Sets cached dispatcher id for runtime-async (v2) dispatcher frames. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a9ed6c8fa615d2668ab3eed642d6ee50350323db",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "d92624eaf2fde2f8bff010893bde13663cd0791b",
"last_reviewed_commit": "a9ed6c8fa615d2668ab3eed642d6ee50350323db",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "d92624eaf2fde2f8bff010893bde13663cd0791b",
"last_recorded_worker_run_id": "29687171296",
"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": "a9ed6c8fa615d2668ab3eed642d6ee50350323db",
"review_id": 4730773281
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and well-scoped: when the async profiler is active, the default (non-pooling) Task/ValueTask path previously allocated a separate AsyncStateMachineDispatcher wrapper on every leaf suspension, adding one heap allocation per suspension segment that scaled with suspension frequency.
Approach: Sound. Folding the dispatcher machinery into a strongly-typed AsyncProfilerAsyncStateMachineBox<TStateMachine> subclass (reached only via a NoInlining factory and gated behind instrumentation flags) lets the box that async already allocates double as its own dispatcher, so the common profiler-disabled path stays free of profiler-only state. Moving the per-segment dispatcher state (DispatcherId, LastContinuation, ReachedLastContinuation, CurrentContinuationResumes) into AsyncProfiler.Info, and unifying dispatcher-id retrieval to read the cached id, is consistent with the existing V2 (runtime-async) path. The standalone wrapper is correctly retained for the pooling fallback and the custom-context awaiter path, and DebugFinalizableAsyncStateMachineBox<T> deriving from the profiler box cleanly unifies the finalizer + dispatcher cases.
Summary: ✅ LGTM. The change is internal-only (no public API surface), NativeAOT is unaffected, the re-arm/leaf terminal logic in SuspendOrCompleteContext and CreateDispatcher is internally consistent, per-segment id minting via NewId() shares the Task id counter so it cannot collide with V2 task-id dispatcher ids, and the new tests exercise per-segment flattening, sibling isolation, and lifecycle balance. Area owner has already approved. A couple of low-severity observations below are non-blocking.
Detailed Findings
💡 Robustness — broad catch (Exception) in SuspendOrCompleteContext
AsyncStateMachineDispatcherInfo.SuspendOrCompleteContext now wraps the suspend/complete emission in try { ... } catch (Exception) { } to guarantee the dispatch frame is always popped. This is a deliberate behavior change from the prior InstrumentedMoveNext finally block, which did not swallow. Swallowing all exceptions (best-effort instrumentation) is reasonable, but consider whether swallowing truly unexpected failures here could mask real profiler bugs during development — an assert or debug-only rethrow path might aid diagnosability. Non-blocking.
💡 Finalizer suppression gating in ClearStateUponCompletion
The GC.SuppressFinalize(this) call is now gated behind AsyncInstrumentation.IsActive && LoadFlags(...) plus IsEnabled.Tpl(flags) && TrackAsyncMethodCompletion, rather than the previous unconditional TrackAsyncMethodCompletion check. If instrumentation transitions to inactive between box creation and completion, SuppressFinalize is skipped and a completed DebugFinalizableAsyncStateMachineBox remains on the finalization queue. Correctness is preserved because the finalizer re-checks !IsCompleted before firing IncompleteAsyncMethod, so this is at worst a rare extra finalization, not a spurious event. Worth confirming this matches intent. Non-blocking.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 144.6 AIC · ⌖ 10.7 AIC · ⊞ 10K
|
/ba-g known issue #130961 |
Summary
Eliminates the separate per-suspension async-dispatcher heap allocation on the default (non-pooling)
Task / ValueTaskasync path when the async profiler is active. The state-machine box now doubles as its own dispatcher, so profiling suspend-heavy async code no longer adds an allocation per suspension segment.Background
When the async profiler is enabled, each suspension needs an async dispatcher to carry the profiler's per-segment context identity and emit the create/resume/suspend/complete events. Previously this was a standalone
AsyncStateMachineDispatcherobject allocated as a wrapper around the state-machine box on every leaf suspension - one extra heap allocation per suspension segment, scaling with suspension frequency.Change
• Merged box. Introduces
AsyncProfilerAsyncStateMachineBox<TStateMachine>- anAsyncStateMachineBox<TStateMachine>subclass that also implementsIAsyncStateMachineDispatcher. It carries the dispatcher state and aMoveNextAsDispatcherpath. When the profiler is active,GetStateMachineBoxallocates this box (which the default async path allocates anyway) so the dispatcher piggybacks on it - zero extra allocations.• Per-segment dispatcher id. The id is minted on demand and reset when a segment completes, giving each suspension segment a unique, stable id. The terminal rule emits Suspend (id retained) when the box re-arms itself as a leaf, and Complete (id reset) when the method completes or hands leaf-ship to a child - producing correct flattening of inline resume cascades.
• Fallback preserved. The standalone
AsyncStateMachineDispatcherwrapper is still used only where the box isn't anIAsyncStateMachineDispatcher, so opt-in pooling is unaffected.•
DebugFinalizableAsyncStateMachineBox<T>now derives from the profiler box, andGetStateMachineBoxprioritizes it when TPL async-method-completion tracking is enabled — so a single box provides both the finalizer diagnostics and the dispatcher machinery when the profiler and completion tracking are active simultaneously.• V2 (runtime-async) dispatcher-id retrieval is unified to read the cached id from the dispatcher/context.
Performance
On the default non-pooling async path, the profiler now adds no per-suspension dispatcher allocations - the box that async already allocates serves as the dispatcher (the marginal cost is two extra fields on that box). The dispatch frame is stack-based and the id mint is an interlocked increment, so no additional heap traffic. Pooling (opt-in) retains its wrapper dispatcher. Fully-synchronous async methods allocate no box and are unaffected. All is gated by async profiler flags.
Compatibility
• NativeAOT: zero impact - the merged box, factory, and finalizable box are not included on NativeAOT.
• R2R SPC: The async profiler box is a generic type reached only via the
NoInliningfactory, so it's instantiated per-state-machine-type at JIT time on demand, not pre-baked into R2R images.•
AsyncProfilerAsyncStateMachineBox<TStateMachine>only triggered when async profiler is enabled, so only JIT-compiled on demand. Code is also guarded by instrumentation feature flags, so when disabled, it will be DCE:ed out.• No public API changes; all new members live on internal profiler types.
Testing
New/extended coverage in
AsyncProfilerV1Tests.csand shared parser infra inAsyncProfilerTests.cs, including per-segment flattening (StateMachineAsync_NestedChildResume_FlattensPerSegment), inline vs. pooled re-suspend parity, and single-await lifecycle parity across pooling/non-pooling and critical/notify awaiter variants. Full V1/V2 profiler suites pass.