SDLPrinter.Print in addition to SDLPrinter.PrintAsync
nightroman opened this issue · 8 comments
Is your feature request related to a problem? Please describe.
Having only SDLPrinter.PrintAsync is inconvenient in synchronous scenarios.
Not usually recommended Result or GetAwaiter().GetResult() seemingly works
(so far? by chance?) but produces the code analysis issue / warning CA2012:
ValueTask instances should not have their result directly accessed unless the
instance has already completed. Unlike Tasks, calling Result or
GetAwaiter().GetResult() on a ValueTask is not guaranteed to block until the
operation completes. If you can't simply await the instance, consider first
checking its IsCompleted property (or asserting it's true if you know that to
be the case).
The recommended above safe way of getting ValueTask result is somewhat convoluted.
Describe the solution you'd like
The synchronous SDLPrinter.Print free of potential issues would be handy.
Agreed. Unfortunately this would duplicate almost all of the code in both ASTVisitor.cs and SDLPrinter.cs. FYI if you’re writing to a string builder or MemoryStream, I believe GetAwaiter().GetResult() is entirely safe to use because it will run synchronously regardless. But if you’re writing to a file stream or network stream, then there may be multithreading implications (although unlikely for most use cases).
And it would necessitate duplicating all of the relevant tests for the AST walker and SDL printer. It's just a whole lot of code duplication to properly add synchronous support.
Perhaps a temporary solution might be to add a Print method that accepts a StringBuilder (and/or returns a string). Perhaps also add an overload that accepts a MemoryStream and Encoding to write to a memory stream. These methods could simply use GetAwaiter().GetResult() under the hood but would be known to execute synchronously and hence be free from ill effects.
Would replacing ValueTask with Task be a solution, too? I am not that familiar with ValueTask. Quick googling tells it is used in order to improve performance and should be used only when it really improves performance. Because ValueTask introduces some trade-offs (as we see here, for example). Also, in some cases (they say) Task may actually perform better.
Perhaps a temporary solution might be to add a
StringBuilder(and/or returns a string). Perhaps also add an overload that accepts aMemoryStreamandEncodingto write to a memory stream. These methods could simply useGetAwaiter().GetResult()under the hood but would be known to execute synchronously and hence be free from ill effects.
It looks like a good solution, straightforward for the end user, with all tedious caveats resolved internally.
ValueTask<T> reduces allocations when the method runs synchronously. Typically this does not occur when using asynchronous methods such as writing to a file or network stream, but even then might if reading/writing cached data. However in this case where the underlying operation is often (or typically) writing to a string builder, there are notable performance benefits to ValueTask<T>.
Note that performance benefits of ValueTask over Task are very rare, as typically the static instance of Task.CompletedTask is returned from synchronously-completed asynchronous methods that return Task. However in this library, ValueTask was used throughout for consistency.
While there are some limitations with ValueTask, there are no benefits or consequences to ValueTask over Task as it relates to executing asynchronous code synchronously.
You may be interested in these relevant articles on the use of ValueTask and allocations:
- https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/
- https://devblogs.microsoft.com/dotnet/async-valuetask-pooling-in-net-5/
Notably, while .NET Framework might require up to 4 allocations the first time await is encountered, and 2 for each additional await in the same method, .NET Core reduces this to 1 (per method I believe), so the benefits of using ValueTask on .NET Core is less pronounced than on .NET Framework (but still exist).
That means, whereas in .NET Framework we have at least four allocations for the first suspension and often at least two allocations for each subsequent suspension, in .NET Core we have one allocation for the first suspension (worst case two, if custom awaiters are used), and that’s it.
Just FYI
@Shane32 Thank you for taking an action on this. Looking forward to the next version with Print(s).