🤖 Copilot Code Review — PR #123727

Holistic Assessment

Motivation: The PR adds debugger infrastructure for Visual Studio to track runtime-async task timing/duration and integrates TPL event source tracing. This is needed for VS debugging features that show relative start times and durations of async operations. The motivation is sound and the feature is valuable for debugging experience.

Approach: The implementation mirrors the existing s_currentActiveTasks pattern by introducing two new static dictionaries (s_runtimeAsyncTaskTimestamps and s_runtimeAsyncContinuationTimestamps) with lock-based synchronization. It also adds TPL event logging at key lifecycle points. The approach is consistent with how the existing debugger support works in Task.cs.

Summary: ⚠️ Needs Human Review. The code follows existing patterns and appears correct, but there are several concerns that warrant human attention: (1) potential memory leak on exception paths for continuation timestamps, (2) whether the return → break change affects TPL event ordering, and (3) test coverage relies on internal implementation details. The PR has been actively reviewed by @jkotas and appears close to ready.

Detailed Findings

⚠️ Memory — Potential leak of continuation timestamp on exception path

In DispatchContinuations(), when an exception is thrown during curContinuation.ResumeInfo->Resume(...), the code jumps to the catch block before RemoveRuntimeAsyncContinuationTimestamp(curContinuation) is called:

// Line ~502-520 in DispatchContinuations
long timestamp = 0;
if (Task.s_asyncDebuggingEnabled)
{
    timestamp = Task.GetRuntimeAsyncContinuationTimestamp(curContinuation, out long timestampVal) ? timestampVal : Stopwatch.GetTimestamp();
    Task.UpdateRuntimeAsyncTaskTimestamp(this, timestamp);
}
Continuation? newContinuation = curContinuation.ResumeInfo->Resume(curContinuation, ref resultLoc);  // <-- exception here

if (Task.s_asyncDebuggingEnabled)
{
    Task.RemoveRuntimeAsyncContinuationTimestamp(curContinuation);  // <-- skipped on exception
}

The timestamp for curContinuation gets added but never removed if Resume throws. While this only affects debugging scenarios and UnwindToPossibleHandler does call RemoveRuntimeAsyncContinuationTimestamp as it walks the chain, there may be edge cases where entries persist.

Note: This was also flagged by the automated Copilot PR reviewer but marked as "low confidence". The exception path does have cleanup logic in UnwindToPossibleHandler, so this may be intentional or a non-issue in practice. Worth verifying the lifecycle is correct.

⚠️ Ordering — TPL event ordering after return → break change

The return statements were changed to break statements to enable TraceSynchronousWorkEnd to be called at the end:

// After the while(true) loop
if (isTplEnabled)
{
    TplEventSource.Log.TraceSynchronousWorkEnd(CausalitySynchronousWork.Execution);
}

However, TraceSynchronousWorkEnd is now called after TraceOperationEnd in the exception and completion paths. The original design appears to have been that each exit path would directly return. Verify with the TPL event consumer (VS debugger/diagnostics team) that this event ordering is acceptable.

✅ Pattern Consistency — Dictionary initialization pattern

The new timestamp dictionary methods follow the exact same pattern as the existing AddToActiveTasks/RemoveFromActiveTasks:

Dictionary<object, long> continuationTimestamps =
    Volatile.Read(ref s_runtimeAsyncContinuationTimestamps) ??
    Interlocked.CompareExchange(ref s_runtimeAsyncContinuationTimestamps, new Dictionary<object, long>(ReferenceEqualityComparer.Instance), null) ??
    s_runtimeAsyncContinuationTimestamps;

This is the established pattern in Task.cs for lazy dictionary initialization. Good.

✅ Thread Safety — Lock-based synchronization is consistent

The use of lock(dictionary) for all operations matches the existing s_currentActiveTasks pattern. Given this is only enabled when s_asyncDebuggingEnabled is true (debugger-controlled), the locking overhead is acceptable.

✅ Correctness — AsyncDispatcherInfo.Task field initialization

The diff shows asyncDispatcherInfo.Task = this; is added in DispatchContinuations(), correctly initializing the new field.

💡 Style — NotifyDebuggerOfRuntimeAsyncState empty method

The empty method with [MethodImpl(MethodImplOptions.NoOptimization)] is a common pattern for debugger integration (a "breakpoint target"). The pragma suppressions are appropriate. However, consider adding a brief comment explaining this is a debugger integration point:

// Provides a stable point for debuggers to inspect runtime async state.
[MethodImpl(MethodImplOptions.NoOptimization)]
public void NotifyDebuggerOfRuntimeAsyncState()
{
}

💡 Test — Relies on reflection into private fields

The test uses reflection to access internal implementation details:

typeof(Task).GetField("s_asyncDebuggingEnabled", BindingFlags.NonPublic | BindingFlags.Static).SetValue(null, true);
// ...
int taskCount = ((Dictionary<int, long>)typeof(Task).GetField("s_runtimeAsyncTaskTimestamps", BindingFlags.NonPublic | BindingFlags.Static).GetValue(null)).Count;

This is fragile and will break if the implementation changes. However, given this is testing debugger-only infrastructure with no public API surface, this approach may be acceptable. The test correctly uses RemoteExecutor to isolate the debugger-enabled state.

✅ Platform Handling — #if !MONO guards

The new timestamp dictionaries and methods are correctly guarded with #if !MONO since runtime-async is a CoreCLR feature. The test file is also correctly excluded from Mono builds via the csproj condition.

💡 Comment Typo — "Debuggger"

There's a typo in the Task constructor comments:

_ = s_asyncDebuggingEnabled; // Debuggger depends on Task cctor being run when any Task gets created

Should be "Debugger".

Cross-cutting Analysis

Sibling types: The changes are isolated to the runtime-async infrastructure. The existing TPL task debugging (s_currentActiveTasks, AddToActiveTasks, RemoveFromActiveTasks) is untouched and the new code follows the same patterns.

Callers: The new timestamp methods are only called from AsyncHelpers.CoreCLR.cs and are guarded by Task.s_asyncDebuggingEnabled, which is set by the debugger. Normal runtime paths are unaffected.

Summary

The PR is well-structured and follows existing patterns. The main concerns are:

1. Verify continuation timestamp cleanup on exception paths is complete
2. Confirm TPL event ordering change is acceptable
3. Minor typo fix

Assuming #1 and #2 are verified, the PR appears ready to merge.

1. and 3. have been fixed, and 2. is consistent with current implementation of async state machine boxes.

@333fred Is this a known Roslyn issue?

No, please file a bug.

I don’t believe this is an action item, as the failing legs are mono

It failed on Mono legs only since we pass different options to the compiler in Mono builds. It is still a bug. Roslyn should not crash.

Opened dotnet/roslyn#82397 (repros on vanilla .NET 11 P1 SDK).

/ba-g known issues (#110173 #123982) and timeouts

I did some measurements and this regressed suspension/resumption performance by about 7%:

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;

namespace OSRPerf;

public class Program
{
    static void Main()
    {
        NullAwaiter na = new NullAwaiter();

        Task t = Foo(10_000_000, na);
        while (!t.IsCompleted)
        {
            na.Continue();
        }
    }

    static int s_value;
    static async Task Foo(int n, NullAwaiter na)
    {
        for (int i = 0; i < n; i++)
        {
            s_value += i;
        }

        Stopwatch timer = Stopwatch.StartNew();
        for (int i = 0; i < 10_000_000; i++)
        {
            await na;
        }
        Console.WriteLine("Took {0:F1} ms", timer.Elapsed.TotalMilliseconds);
    }

    private class NullAwaiter : ICriticalNotifyCompletion
    {
        public Action Continue;

        public NullAwaiter GetAwaiter() => this;

        public bool IsCompleted => false;

        public void GetResult()
        {
        }

        public void UnsafeOnCompleted(Action continuation)
        {
            Continue = continuation;
        }

        public void OnCompleted(Action continuation)
        {
            throw new NotImplementedException();
        }
    }
}

Before: 792.9 ms
After: Took 848.6 ms
(With DOTNET_TieredCompilation=0)

Not trying to say anything by this, just keeping track of some of the papercuts.

I will add some additional instrumentation into the dispatcher loop as well, mainly to track activity for an optimized "TPL-Lite" event source, but will make sure to combine all checks into simple conditions that will be highly predictable inside the hot path, but since we will need "instrumentation" in this very hot loop an alternative would be to duplicate the DispatchContinuations with added profiling/eventing checks, then the only change to the current DispatchContinuations would be an highly predictable branch at the start of the method and potentially a check when a continuation complete if the flag changed, and if that happens hit a cold path that would handle next continuation as suspended, breaking out of DispatchContinuations. That would at least make sure we won't get perf regressions inside the DispatchContinuations method at the cost of duplicating that code, ~100 lines. I assume this method won't change that much going forward, so maybe it's acceptable to have the duplication to protect the performance of the very hot dispatcher loop going forward. Having two versions will also make sure they get optimized differently by the JIT, since the profiled version will have a very different execution flow compared to the regular one. @jakobbotsch, thoughts?

Found a way to put this into a close to zero cost abstraction, so instead of duplicating the code there will be instrumentation points in the dispatcher loop, using generics + struct to produce two native version of the dispatch loop method, where the one without profiling support will optimize away all the instrumentation points producing a native version without instrumentation, while the instrumented version will get optimized codegen based on profiling scenarios. I can take a look if some of the changes done into the dispatcher loop in this PR could be refactored into the same pattern, reducing the impact of the change. I will also run the sample provided in this PR above to measure overhead.

I will add some additional instrumentation into the dispatcher loop as well, mainly to track activity for an optimized "TPL-Lite" event source, but will make sure to combine all checks into simple conditions that will be highly predictable inside the hot path, but since we will need "instrumentation" in this very hot loop an alternative would be to duplicate the DispatchContinuations with added profiling/eventing checks, then the only change to the current DispatchContinuations would be an highly predictable branch at the start of the method and potentially a check when a continuation complete if the flag changed, and if that happens hit a cold path that would handle next continuation as suspended, breaking out of DispatchContinuations. That would at least make sure we won't get perf regressions inside the DispatchContinuations method at the cost of duplicating that code, ~100 lines. I assume this method won't change that much going forward, so maybe it's acceptable to have the duplication to protect the performance of the very hot dispatcher loop going forward. Having two versions will also make sure they get optimized differently by the JIT, since the profiled version will have a very different execution flow compared to the regular one. @jakobbotsch, thoughts?

Found a way to put this into a close to zero cost abstraction, so instead of duplicating the code there will be instrumentation points in the dispatcher loop, using generics + struct to produce two native version of the dispatch loop method, where the one without profiling support will optimize away all the instrumentation points producing a native version without instrumentation, while the instrumented version will get optimized codegen based on profiling scenarios. I can take a look if some of the changes done into the dispatcher loop in this PR could be refactored into the same pattern, reducing the impact of the change. I will also run the sample provided in this PR above to measure overhead.

I like the idea, please do feel free to open a PR to try that out. I think it would make it close to zero cost in performance, at the cost of some size, since there is a dispatcher for every RuntimeAsyncTask instantiation. But the trade-off sounds good to me and I expect that we can factor a large part of the dispatcher out to avoid most of the size cost.

I implemented the generic valuetype version of the DispatchContinuations loop that includes capabilities to "upgrade" to instrumented version on entry and after continuation completion. I optimized as much as possible on what gets left in the none instrumented version of DispatchContinuations and looks like I recovered most of the perf lost in this PR. I compared the generated assembler using DOTNET_TieredCompilation=0 running the same test scenario as above:

I have most prepared, need to look at some tests when enabling the instrumentation + code size when enabled, but it will probably land close to the old metrics above for that version of the dispatch loop.

I will build on this to add more instrumentation logic for async profiling, but now all will happen in the instrumented version of the DispatchContinuations loop, not affecting the performance of none instrumented scenarios.