[runtime-async] Fix timestamp instrumentation#127662
Merged
Merged
Conversation
rcj1
changed the title
Contributor
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the runtime-async async-debugger instrumentation so that newly-created continuation chains get fresh timestamps at suspension time (instead of inheriting the timestamp of the currently-running continuation), and removes now-unneeded timestamp propagation logic tied to follow-up scheduling.
Changes:
- Removed the
Task.TryAddRuntimeAsyncContinuationChainTimestamps(Continuation chain, Continuation timestampSource)overload to avoid propagating an “old” timestamp to newly allocated continuation nodes. - Simplified
AsyncDebugger.HandleSuspended/InstrumentedHandleSuspendedto always timestamp based on the suspended chain head (sentinel’sNext) without a separate timestamp source. - Removed the
SuspendRuntimeAsyncContext(ref AsyncDispatcherInfo, ...)/SuspendAsyncContext(ref AsyncDispatcherInfo, ...)path that previously timestamped the remaining chain when a follow-up continuation was queued.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs | Removes the timestamp-source-based chain timestamping helper, leaving chain timestamping to use a fresh suspension-time timestamp. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs | Updates runtime-async instrumentation to timestamp suspended chains without using a “source continuation”, and removes the follow-up-action timestamping/suspend helper calls. |
Member
|
You will need to rebase and test this on top of #127238. |
Member
|
We should probably also add some test to see if we could guard this specific scenario as well. |
Contributor
Author
|
/ba-g timeouts |
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.
We need to ensure that new continuations are timestamped at suspension time. In an app with an async chain starting with, say, Main,
curContinuationandnewContinuationwill both belong to Main upon resuming Main in DispatchContinuations. The async chain int_runtimeAsyncAwaitState.SentinelContinuation!.Nexthas been built by the JIT viaAsyncSuspendand contains newly allocated continuations for the methods above Main in the stack. These should receive a new timestamp, not that of Main.I don't think we need to add timestamps in
QueueContinuationFollowUpActionIfNecessary; they have already been added inHandleSuspended.Fixes the issue of all the timestamps being the same.
Benchmark #123727 (comment):
New: 387.4 ms
Old: 404.2 ms