SDLPrinter.Print in addition to SDLPrinter.PrintAsync about parser HOT 8 CLOSED

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).

from parser.

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.

from parser.

See:

• #276

from parser.

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.

from parser.

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.

It looks like a good solution, straightforward for the end user, with all tedious caveats resolved internally.

from parser.

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

from parser.

@Shane32 Thank you for taking an action on this. Looking forward to the next version with Print(s).

from parser.

Published in https://github.com/graphql-dotnet/parser/releases/tag/8.2.0

from parser.