Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Most failures are due to test _reflection::Async2Reflection.FromStack that currently asserts that DispatchContinuations is on stack and is currently not aware of the change to a generic method. If we stick with the generic method, then the assert in this test needs to be updated to reflect the name change.

s_asyncDebuggingEnabled implies that these TPL events have been enabled, with two exceptions:

1. Failure to create ETW session (too many existing ETW sessions, etc)
2. Manual shutdown of VS-created ETW session or other exceptional error that causes ETW session to close

We can assume that s_asyncDebuggingEnabled implies required TPL events enabled, and save the setting and checking of these flags. The downside is a tiny amount of overhead spent checking whether TPL events are enabled in these two exceptional cases during debugging - in my opinion, an acceptable tradeoff for simplification and any possible optimization of the hot path.

If someone believes that scenario 1 is not very uncommon, it is possible to change Concord such that they will only be out of sync in case 2.

s_asyncDebuggingEnabled implies that these TPL events have been enabled, with two exceptions:

1. Failure to create ETW session (too many existing ETW sessions, etc)
2. Manual shutdown of VS-created ETW session or other exceptional error that causes ETW session to close

We can assume that s_asyncDebuggingEnabled implies required TPL events enabled, and save the setting and checking of these flags. The downside is a tiny amount of overhead spent checking whether TPL events are enabled in these two exceptional cases during debugging - in my opinion, an acceptable tradeoff for simplification and any possible optimization of the hot path.

If someone believes that scenario 1 is not very uncommon, it is possible to change Concord such that they will only be out of sync in case 2.

Question is if pure TPL will have meaning here, like adding TPL events for runtime async activities in case a client only consumes TPL events to recreate async callstacks, or if that was never the intent with this change and it should only support the debugger features, then we should definitely put all logic under the same flag.

If we could assume the feature will trigger the TPL event source, we could handle it all in the on command event onTPL event source, and we can do the fast check only on flags, maybe that was what you proposed @rcj1? In that case this feature won't light up without the TPL session enabled, but that might be, OK? It would speed up the instrumentation gate check since it will only look at the flags again and ignore s_asyncDebuggingEnabled as part of dispatch loop.

I took this path to get away from duplicating the complete DispatchContinuations method into a regular and instrumented version, reducing the maintenance of ~100 lines of duplicated high performing unsafe code. It can be argued that duplicating the DispatchContinuations is a small price to pay giving a little clearer implementation. If that is something we all agree on and accept, I'm happy to pursue that path as well, but wanted to start with the zero cost abstraction, no duplication path.

I worry that all the AggressiveInlining static abstracts introduce non-trivial startup cost when it comes to compiling all the dispatchers. It should be possible to measure this via some polymorphically recursive async methods that creates a lot of rarely used instantiations.

The other thing with the current approach -- the inversion of control makes the dispatcher quite hard to follow for me. But I can live with this given the improved performance.

I worry that all the AggressiveInlining static abstracts introduce non-trivial startup cost when it comes to compiling all the dispatchers. It should be possible to measure this via some polymorphically recursive async methods that creates a lot of rarely used instantiations.

Doesn't look too bad, I tried a binary counter micro benchmark that loads 4096 of the dispatchers:

using System;
using System.Diagnostics;
using System.Threading.Tasks;

namespace AsyncMicro;

public class Program
{
    static void Main()
    {
        Stopwatch timer = Stopwatch.StartNew();
        new Program().Recurse<byte, byte, byte, byte, byte, byte, byte, byte, byte, byte, byte, byte>(0).GetAwaiter().GetResult();
        Console.WriteLine("Took {0} ms", timer.ElapsedMilliseconds);
    }

    private async Task<V<T11, T10, T9, T8, T7, T6, T5, T4, T3, T2, T1, T0>> Recurse<T11, T10, T9, T8, T7, T6, T5, T4, T3, T2, T1, T0>(int n)
    {
        await Task.Yield();

        //Console.WriteLine(n);
        Task t;
        if (typeof(T0) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, T6, T5, T4, T3, T2, T1, int>(n + 1);
        }
        else if (typeof(T1) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, T6, T5, T4, T3, T2, int, byte>(n + 1);
        }
        else if (typeof(T2) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, T6, T5, T4, T3, int, byte, byte>(n + 1);
        }
        else if (typeof(T3) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, T6, T5, T4, int, byte, byte, byte>(n + 1);
        }
        else if (typeof(T4) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, T6, T5, int, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T5) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, T6, int, byte, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T6) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, T7, int, byte, byte, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T7) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, T8, int, byte, byte, byte, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T8) == typeof(byte))
        {
            t = Recurse<T11, T10, T9, int, byte, byte, byte, byte, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T9) == typeof(byte))
        {
            t = Recurse<T11, T10, int, byte, byte, byte, byte, byte, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T10) == typeof(byte))
        {
            t = Recurse<T11, int, byte, byte, byte, byte, byte, byte, byte, byte, byte, byte>(n + 1);
        }
        else if (typeof(T11) == typeof(byte))
        {
            t = Recurse<int, byte, byte, byte, byte, byte, byte, byte, byte, byte, byte, byte>(n + 1);
        }
        else
        {
            return default;
        }

        await t;

        return default;
    }

    private struct V<T11, T10, T9, T8, T7, T6, T5, T4, T3, T2, T1, T0>
    {
        public int X;
    }
}

The difference is only around 100 ms on this PR.
Base: Took 9205 ms
Diff: Took 9303 ms

(P.S. a simpler binary recursing generic hits some exponential behavior... Need to investigate that.)

The other thing with the current approach -- the inversion of control makes the dispatcher quite hard to follow for me. But I can live with this given the improved performance.

The alternative is to duplicate the DispatchContinuation implementation into two different methods, one default (with upgrade capabilities) and one instrumented version. In the end it depends on whats important, this PR currently take the reduce code duplication path, with slightly more complex dispatcher implementation (due to the instrumentation callbacks), and I think that is fine. Dropping the R2R exclusion reducing one indirect call on the instrumented path will simplify the instrumented path a little more as well, with very small size increase of S.P.C.