AsyncRewriter icon indicating copy to clipboard operation
AsyncRewriter copied to clipboard

I wrote the reverse of this: https://github.com/zompinc/sync-method-generator

Open virzak opened this issue 3 years ago • 5 comments

The app I'm working on needs equivalent implementation of multiple routines for sync and async.

Here is the code generator which creates a sync method from async.

Haven't found any other project that attempts anything similar, so just leaving an issue in case anyone is interested.

virzak avatar Oct 31 '22 02:10 virzak

@virzak I stopped working on the AsyncRewriter a long, long time ago. I wrote AsyncRewriter back in a time where synchronously-completing async methods where meaningfully more expensive than their equivalent synchronous methods (for example, ValueTask did not yet exist, so sync-returning async methods caused heap allocations). Nowadays, async methods which return synchronously should be very close in performance to their sync counterparts, so I wouldn't recommend going into the trouble and complexity of source-generating async paths etc. You can instead of async methods accepting and passing down async flag, and perform sync vs. async I/O based on that flag; that allows for a single, unified code path without any source generation.

roji avatar Oct 31 '22 06:10 roji

Even after .NET 6 improvements to FileStream, async operations are 1.7 times slower than sync (used to be 8 times slower) - https://github.com/dotnet/runtime/issues/27047#issuecomment-822508929. It is now considered optimal for Windows and no further improvements are expected. Taking that hit when zooming out of a large binary data is a very noticeable perf hit.

I'm not entirely sure what you mean by passing async flag. Is it running sync over async as pointed out here? And if so, isn't this an antipattern?

private async Task<string> GetStringCoreAsync(bool sync, CancellationToken cancellationToken)
{
    return sync
        ? SomeLibrary.GetString()
        : await SomeLibrary.GetStringAsync(cancellationToken).ConfigureAwait(false);
}

public string GetString()
    => GetStringCoreAsync(true, CancellationToken.None)
        .ConfigureAwait(false)
        .GetAwaiter()
        .GetResult();

public Task<string> GetStringAsync()
    => GetStringAsync(CancellationToken.None);

public Task<string> GetStringAsync(CancellationToken cancellationToken)
    => GetStringCoreAsync(false, cancellationToken);

virzak avatar Oct 31 '22 09:10 virzak

Even after .NET 6 improvements to FileStream, async operations are 1.7 times slower than sync (used to be 8 times slower) - https://github.com/dotnet/runtime/issues/27047#issuecomment-822508929. It is now considered optimal for Windows and no further improvements are expected. Taking that hit when zooming out of a large binary data is a very noticeable perf hit.

I'm not trying to say that async I/O performs the same as sync I/O; only that the overhead of executing sync I/O via a method marked with async should be negligible for almost all cases in modern .NET. That allows you to have a single async method, which internally invokes either sync or async lower-level I/O APIs based on a flag parameter.

I'm not entirely sure what you mean by passing async flag. Is it running sync over async as pointed out here? And if so, isn't this an antipattern?

Yes, the code you posted above is the pattern I was referring to. This is not sync-over-async: public sync APIs perform only sync I/O, and public async APIs perform only async I/O; sync-over-async refers to when a public sync API internally uses async I/O under the hood, which is indeed a bad anti-pattern.

Your code above performs pretty much the same as if you wrote two separate sync/async implementations (or source-generated them). There's a slight overhead associated with the async keyword itself (the compiler rewrites the method as a state machine etc.), but as I wrote above, if done correctly, the overhead should really be negligible in modern .NET (especially when actual I/O does take place).

Note that I went exactly through this with Npgsql (the PostgreSQL database driver I work on). Originally I had separate sync/async paths, and the maintenance burden led me to write the AsyncRewriter in the first place. At some point later, as .NET got much more optimized, ValueTask was introduced etc., I did some benchmarking on how much overhead it would be to simply merge the two paths together, and found it was pretty negligible. I suggest you do the same for your actual application; doing this kind of source generation can be tricky/brittle and you're introducing quite a bit of complexity, so be sure you actually need it.

roji avatar Oct 31 '22 09:10 roji

Thanks @roji,

I guess it's a preference whether one uses a source generator or that pattern. For me the upside is just having one attribute instead of writing features by hand and the downside is inability to edit a readonly generated file, but I could be biased.

Two other scenarios for the source generator are:

  • Converting Memory to Span. The source generator in its current form can eliminate the System.IO.StreamExtensions.Read method. https://github.com/npgsql/npgsql/blob/0d8dd34ae957e26c9e61edaa35219353b3b0bd8e/src/Npgsql/Shims/StreamExtensions.cs#L12-L25
  • The source generator can rewrite IAsyncEnumerable, but how does one go about handling it without it? Here is my best effort... Not sure if there's a better way.
public static IEnumerable<int> IndexOfMaxSoFar<T>(this IEnumerable<T> items) where T : IComparisonOperators<T, T, bool>
{
    async IAsyncEnumerable<T> CreateAsyncEnumerable()
    {
        foreach (var i in items)
        {
            yield return await Task.FromResult(i);
        }
    }
    return IndexOfMaxSoFarAsync(CreateAsyncEnumerable()).ToBlockingEnumerable();
}

public static async IAsyncEnumerable<int> IndexOfMaxSoFarAsync<T>(this IAsyncEnumerable<T> items, IProgress<int>? progress = null, [EnumeratorCancellation] CancellationToken ct = default)
    where T : IComparisonOperators<T, T, bool>
{
    var i = 0;

    T? largestSoFar = default;
    await foreach (var item in items)
    {
        ct.ThrowIfCancellationRequested();

        if ((i & 0x3ff) == 0)
        {
            progress?.Report(i);
        }

        if (largestSoFar is null || largestSoFar < item)
        {
            largestSoFar = item;
            yield return i;
        }

        ++i;
    }
}

virzak avatar Oct 31 '22 14:10 virzak

Yeah, I agree there's some potential for a source generator here... When I was writing the sync-to-async rewriter (long before source generators were a thing), it turned out to be quite brittle and problematic around certain coding patterns, so I was happy to ditch it. Maybe doing it the other way around (async-to-sync) makes things easier in some way.

roji avatar Oct 31 '22 15:10 roji