[Proposal] Async version of CSharp protobuf

[mkosieradzki]

I would like to implement support for async/await versions of all methods dealing with I/O.

Rationale

Mixing async and sync code in C# is very inefective thread-wise. It is a good practise to have entire code top-down async, but the worst case-scenario is when synchronous code calls async code like in case of Stream.Read and Stream.Write.

Today the only way to efficiently use protobuf in async code is to copy buffers to the memory and then perform deserialization synchronously. What makes it virtually impossible to use protobuf in streaming scenarios with near-to-constant memory.

Background

Lately there have been many improvements to the TPL to allow more efficient (allocation-wise) handling of many scenarious especially parser-like where most of methods return synchronously. ValueTask and Task.CompletedTask are the most important of those improvements.

High-level plan

1. Duplicate all I/O-bound methods in the following manner:
   ReturnType MethodName(Arguments)
   to
   async ValueTask MethodNameAsync(Arguments, CancellationToken cancellationToken)

void MethodName(Arguments)
to
async Task MethodNameAsync(Arguments, CancellationToken cancellationToken)

And all references in async methods to those methods with await CalledMethodAsync(..., cancellationToken)

This way we have perfectly correct implementation (but not optimal, especially allocation-wise).

Replace all calls to Stream.Read/Write with proper ReadAsync/WriteAsync

2. After testing this version start optimizations:
   a. Remove constructs like async await = where possible
   b. Try to use as much of ValueTask and Task.CompletedTask as possible.
   c. Try to inline some async calls to avoid generation of separate state-machines

3. Modify code-generator to generate async variants.

I would be happy to create PR for this proposal.

However I believe optimization will be a bit longer process, going far beyond this single PR.

[jskeet]

I would strongly suggest not using ValueTask or any C# 7 features that require additional dependencies. Any dependencies we add - or additional requirements for more recent frameworks - will add a toll in terms of maintenance and may reduce the potential audience. Creating regular async methods for parsing is more reasonable, although it's tricky in terms of the generated code... if any code needs to be generated with async methods, that would probably need to be specified via an option, as otherwise it could very easily break existing users. (And it seems likely - without having looked at this at all yet in detail - that it really would require additions to generated code.

More worryingly, it may require additions to interfaces, which basically can't happen due backwards compatibility. You may well want to look at an IAsyncMessage interface or similar.

[mkosieradzki]

@jskeet Thanks for your answer.

I have created a prototype and it actually uses a separate IAsyncMessage :) interface to prevent breaking compatibility with existing code.

Regular async methods are where the problem lies. One would want to minimize the amount of code in the regular async methods and limit the regular async methods only to the slow-path. Hot path should not do any GC-allocations and my intuition says it is possible. However not without ValueTask.

How would the optimized pattern look

ValueTask<ReturnType> MyMethod(Args) => fastPathPossible ? MyMethodFast(Args) : MyMethodSlow(Args)

ValueTask<ReturnType> MyMethodFast()
{
      ... synchronous fast path
      return new ValueTask<ReturnType>(....);
}

async Task<ReturnType> MyMethodSlow()
{
      ... standard async implementation.
}

It is only possible to efficiently implement Writing without ValueTask because write methods have no return values and we can use Task.CompletedTask in the fast path to avoid heap allocations. BTW. It will require bumping up net45 to net46 AFAIR.

It is impossible to efficiently implement Reading without ValueTask because sooner or later you need to use Task.FromResult() which will do the heap allocation. And there will be at least one heap allocation for EVERY value read. What does not sound reasonable.

ValueTasks should be working without any problems on top of .NET Standard and I see no point in maintaining library version compatible with something older than .NET Standard 1.0. BTW. This project is already depending on .NET Standard 1.0, so I really don't get the point about ValueTasks. Or am I missing something?

Regarding the generated code (the protobuf, not the async ;) ). I think it would be a good idea to add an option like Async that will cause generate IAsyncMessage instead of IMessage to retain the reverse compatibility with existing user and maybe in future make it default if it will make any sense.

[jskeet]

"ValueTasks should be working without any problems on top of .NET Standard" - not without adding a new dependency. Adding any dependency to a widely-used library is fraught with problems. ValueTask isn't in netstandard1.x at all - it's in netcoreapp1.0, and obviously it's not in net45 either. And no, we're not going to just arbitrarily stop supporting net45. (I'm working in the opposite direction, hoping to support .NET 3.5 and Unity in the future.)

My aim wouldn't be nearly as aggressive as yours of removing all heap allocations. If you really want to do that, just do all the reading asynchronously into a MemoryStream and then just use the synchronous protobuf parsing code. If any of the I/O is asynchronous, you'll end up with the allocations even with ValueTask anyway.

I can see a benefit in having optional async code generated and async code using just Task<T> in the support library - no new dependencies, and by default still only generating the synchronous code to avoid taking a dependency on the C# 5 compiler. But adding dependencies, breaking backwards compatibility for older C# compilers from the generated code, and moving the frameworks we target? They're not a roadmap I'd support.

Of course, the Protobuf team could go ahead with that anyway - I'm just a contributor - but I'd strongly recommend against it.

[mkosieradzki]

I see you point. However I am not sure whether Unity isn't going towards .NET Standard as well (but I don't use Unity).

OK. So we basically expect different things from this library. I expect ultra-high performance serialization library where it's worth to use latest developments in .NET CoreFX. And you expect widely supported and widely compatible. But it is impossible to have both of these goals in a single library.

I had many perf ideas including using for example buffers rental (I have seen new byte[1024] in code way too many times :) - I understand now - buffer rental requires System.Buffers... ), adding streaming serialization support, better struct support.

But this is exactly the opposite direction you want this library to go. I am aware that you are the author of the original code, and I definitely don't want to push against you. So I will probably end up creating an unsupported fork.

BTW. How does MemoryStream avoid heap allocations as it is itself allocated on heap (even on LOH)? And with it's reallocation algorithm it is the most useless of doom class in entire .NET Framework responsible for most OutOfMemoryException people have due to LOH fragmentation. But I get your point about pre-buffering (using something better than MemoryStream) the data and using the current synchronous code. I have even mentioned this approach in the Rationale.

What I wanted, is to use as many stack allocations as possible. C# is not Java and you can do a lot of perfwork here.

To sum up: doing async without ValueTask is basically non-sense in this specific case. If you can't accept ValueTasks, I understand. And I will not pursue this PR here.

[jskeet]

In terms of MemoryStream and the heap allocations: you'd load the data into a single MemoryStream (which could potentially be pooled and reused) - but then you'd know that everything would be synchronous, so you'd just use the sync code instead of async, so you wouldn't end up with lots of tasks being allocated.

I still don't accept that it's pointless to do async without ValueTask, but I wish you well with your private fork. You'll certainly be able to move a lot faster without the limitations that a public, widely used (in a large variety of situations) library has.

[mkosieradzki]

If you really think it's not pointless to do async without ValueTask in this specific case, then I can create a PR with the 1st step of my Proposal (except with Task instead of ValueTask), and then I can do performance optimizations in my fork.

We can then do then some benchmarks to see if they are worth and maybe it will be easier to make an informed decision about ValueTasks based on some hard number.

Does this sound reasonable?

[jskeet]

Yes, that sounds reasonable. I would say that it's possible that even without ValueTask, it would be too complex/disruptive to be worth it, but I suspect it'll be okay. (It's hard to tell before doing...) Please make sure all async parts are in #if !NET35 or equivalent.

[mkosieradzki]

@jskeet
Do you think using partial classes and moving all async code to ,Async.cs is a good idea? I've seen this pattern in multiple projects that wanted to retain .NET 3.5 compat.

[jskeet]

For the support libraries? Yes, that might make sense. For generated code, it'll be much simpler to keep it all in one file.

[mkosieradzki]

I have created a PR to help tracking changes. The current code is focused on correctness - not performance.

Feel free to drop any comments.

[mkosieradzki]

@jskeet What is you opinion should it be a CLI option or File option or both?

I have started with a FileOption. But now I am not sure...

BTW. Even if the async option is enabled I am generating code like this to ensure backwards compatibility.

  #if !NET35
  public sealed partial class MyMessageName: pb::IAsyncMessage<MyMessageName> {
  #else
  public sealed partial class MyMessageName: pb::IMessage<MyMessageName> {
  #endif

It is really important for the pre-generated classes for well-known types.

[jskeet]

Hmm. It should definitely be a CLI option: two users may want to use the same .proto file, with one of them generating async code and one not.

Using the NET35 definition seems fairly reasonable - certainly required for the Google.Protobuf library - but I'd be tempted to do this using partial classes:

public sealed partial class MyMessageName : pb::IMessage<MyMessageName> {
  // Non-async stuff
}

#if !NET35
public sealed partial class MyMessageName : pb::IAsyncMessage<MyMessageName> {
  // Async methods
}
#endif

Aside for anything else, that will mean that the diff between "async-enabled" and "async-disabled" is a single block of code.

I'd be tempted to use another preprocessor symbol as well, so users can define something for other builds where they want non-async-only, e.g.

#if !NET35 && !PROTOBUF_NO_ASYNC
...
#endif

Just thinking aloud here though...

[mkosieradzki]

@jskeet
Regarding options I did both: FileOption and CLI option. ;)

Hmmm really nice idea with this partial (in single file)... I will definitely do this. But I would prefer to defer this in time, because this is a non-breaking change, if you agree.

I was thinking about this preprocessor symbol. However to be honest not sure whether PROTOBUF_NO_ASYNC alone is not a better option than NET35 as there are NET35 implementations of async (AsyncBridge).

Please let me know and I will correct everything to match.
a) !NET35
b) !NET35 && !PROTOBUF_NO_ASYNC
c) !PROTOBUF_NO_ASYNC

BTW. I am currently not adding conditional compilation sections to *.Async.cs because I believe you will skip entire files in NET35/PROTOBUF_NO_ASYNC versions.

[jskeet]

I would advise against making it a file option as well - I really don't think there's any need for it.

Deferring the organizational change: I agree it's non-breaking, but I think it would be worth doing before we get as far as merging. But it can definitely be done after everything else is working...

Compilation symbols: yes, we could easily just go with PROTOBUF_NO_ASYNC. I doubt anyone else is using it for anything, and I think it's self-explanatory enough. The only downside is it being a negative - I prefer concepts to be positive, but we'd probably want it to be a default...

I'm not sure how you were suggesting we skip the entire files for compiling for .NET 3.5 though - while we could do that within the project file, I think it would be cleaner to do it in the files themselves.

[mkosieradzki]

Thanks for your quick answers, they help me progress faster.

1. I have removed the file option
2. I am in process of switching conditions to PROTOBUF_NO_ASYNC
3. I thought about excluding files at project level, but let's do #if for entire file
4. I have refactored codegen to generate to separate classes, unfortunately there is ONE thing I need to do conditionaly in main class. I need to conditionally exclude MessageParser field, because AsyncMessageParser is a separate class in my implementation (to allow better reverse-compat) and I need to conditionaly skip this field (if there is the parser field in the async version of the class, but that's not really painful).
   I am now in the process of regenerating protocols after file option removal.
   And C# protocols after codegen change.

I have started some work with testing - I basically try to reuse the synchronous tests.

[mkosieradzki]

@jskeet
OK. I have introduced the changes discussed. There is still some work to be done in the testing area, but it seems to work..

Let's focus this PR on correctness and do all the perf-work in separate PRs.

[jskeet]

I'll see whether I can get time to review the PR later this week. I've been thinking about the parser part, and I wonder whether it may be feasible to do it all with extension methods on the parser instead. But let's get to that later...

[mkosieradzki]

@jskeet I don't think it's feasible at all, because it requires access to the internal state. It's only feasible as much as some of the synchronous could be moved to an extension class and even if we do it's a terrible moment because I will want to do some perf-work focusing on replacing as many async functions as possible with their synchronous versions (still returning Tasks - don't worry ;) ) (to avoid compiler-generated state-machines and unnecessary allocations).

I have added all the missing documentation.
I have ported many more tests (and implemented AsyncOnlyStreamWrapper - a special facility to help detect unintended synchronous calls), and I have corrected all the errors detected by those tests :).

When my PR passes sanity checks I will now remove the [Do not merge].

I think that the first part is quite complete and ready for review.

[jskeet]

We could provide internal access to whatever state we needed. I'll bear it in mind when I review the PR. I still don't know when I'm going to have time to do it, but I'll have a look at it later this week with any luck.

[mkosieradzki]

@jskeet
Did you have any chance to have a look on this PR?

[jskeet]

Not yet. I started, but had to move onto other things. I'll come back to it when I can.

[jskeet]

I've started looking now, mostly at the class/interface hierarchy around messages and parsers.

Are there reasons why we need to have a single parser for both, rather than having an async parser entirely separate from a sync parser? I wouldn't normally be suggesting that, but basically AsyncMessageParser<T> really wants to derive from both AsyncMessageParser and MessageParser at the moment, which makes things weird.

Separating the two would also leave more room for the option of not generating the synchronous code at all - so you could have:

• Sync only: Parser property, of type MessageParser<T>
• Async only: AsyncParser property, of type AsyncMessageParser<T>
• Both: both properties

We'd need to think about JSON parsing for the async case... I wonder whether that could wait for the moment though.

As you've looked at this more than me, you may have some reason for doing it that way though - in which case, please share :) (There's the slight memory cost of twice as many parsers of course...)

[mkosieradzki]

I also feel that the class hierarchy is a bit sloppy... but had no better idea.

To be honest I feel like it would take a considerable amount of time to get before we get async version to be as fast as synchronous version in a synchronous scenario, but this might happen eventually (I really hope so). But that's a good future-proof idea.

You very often have synchronous parsing even in full async async scenarios for example sub-message parsing (Any-type).

OK. It was 2 week ago so I don't remember all the details, but I will try to recall most important reasons:

1. We would need to keep both parsers in FieldCodec, GeneratedClrTypeInfo
2. Single parser is a bit more convenient
3. Memory is not an issue here: because it's like an additional pointer because it would not even save a single field, because for performance reasons event class factory is duplicated...

On the other hand I don't like original class hierarchy as well. For example I don't see a good reason to have MessageParser<T> inherit MessageParser instead of implementing IMessageParser. actually going this way would allow to save memory on Func<IMessage> factory.

So my recommendation would be:

1. make MessageParser => IMessageParser interface
2. make AsyncMessageParser => IAsyncMessageParser interface
3. make AsyncMessageParser impelement both interfaces (even if it involves a bit copy-and-paste)

And here we save yet-another-class-factory - with much cleaner hierarchy.

Async JSON parsing should be quite trivial to implement as it is reflection based...

UPDATE:
Even in case we decide to use a single implementation for both sync and async synchronous interface SHOULD stay (most work would be done in Coded*Stream then), as it is very useful even in full-async scenarios.

UPDATE2:
It's my personal hate towards the class inheritance in general but we can then make AsyncMessageParser<T> inherit MessageParser<T>.

I have also recalled yet another reason for a single parser field. I was thinking about scenarios where some messages are generated with async and some are not and idea behind creating separate parser class, but placing it on the same Parser Property was to allow nice cooperation between those 2 versions.

UPDATE3:
From library user-perspective MessageParser seems to be quite transparant in terms class vs interface because constructor is internal so no one can really neither subclass nor instantiate it and this class is only instantiated for MessageParser<T>. I feel like MessageParser class is a typical inheritance hierarchy level with no actual value-added. Just introducing an additional cost with a duplicate factory-field.

[jskeet]

We can't just start removing classes from the hierarchy without that being a breaking change requiring a new major version - that's simply not going to happen.

We could build the async parts based on interfaces though, so AsyncMessageParser<T> could implement IAsyncMessageParser and IAsyncMessageParser<T>. I don't think there's ever a need to have a bare AsyncMessageParser.

As for the existence of the non-generic MessageParser - while I agree it could have been an interface, it's necessary in some form or other, for reflection. I can't remember offhand why I didn't make it an interface (given that that would be my normal preference too).

I think it's worth trying a few different ideas here... if we want to keep the single parser per message, I think creating an IAsyncMessageParser interface is a good start.

[mkosieradzki]

I definitely agree. Maybe it should be considered during next major version?

However that's definitely good point we are not bound by this limitation for AsyncMessageParser - so that's a good idea to start with IAsyncMessageParser. and go through the MessageParser hierarchy today and maybe in the new major version end-up replacing MessageParser with IMessageParser. OK. I will update my PR.

Let's keep fingers crossed I will not need to regenerate descriptor-protos to pass the tests.

[jskeet]

There's a lot we might consider for the next major version, but I wouldn't personally expect that to be before 2020.

[mkosieradzki]

;) fair enough

[mkosieradzki]

I agree. The only reason to tackle them together is that both require significant API redesign and both redesigns might be affecting each other. However IMO it's way to early to tackle Span<T>. I think that the proper moment would be after JIT and compiler optimizations are in place and we will be able to collect meaningful perf data.

So let's focus on async only.

Just to be sure (those names are very similar ;) ) this specific issue is for ValueTuple not ValueTask but the other one might be affected as well. Unfortunately System.Buffers is affected too - and I wanted to replace all the local buffer allocations with buffer rentals: dotnet/corefx#24716 .

[mkosieradzki]

I am strongly considering killing my own proposal at least in the current version. Implementing it efficiently will cost a lot of effort while I believe that from the practical standpoint it would be much better idea to implement flow with a following pseudo-code:

async Task<TMessage> ParseDelimitedAsync(Stream stream, int maxAllowedMessageSize)
{
   var buffer = await PrefetchDelimitedAsync(stream, maxAllowedMessageSize);
   return Parse(buffer);
}

TMessage Parse(ReadOnlySpan<byte> buffer)
{
   //Heavy lifting here
}

I think it might be beneficial to implement core parsing using spans without any async or stream reading inside.

This should fit very well recommended scenarios like gRPC where messages should be considerably small and huge/async communication should be done using streaming.

It also should be more efficient in terms of concurrent system and DDoS protection - this approach strongly decreases GC-pressure and working set size of requests being still in-transfer, which cannot be handled,

[jskeet]

I think it's definitely worth trying to work out an endpoint in terms of all the possible options, then consider how we want to get there - it would be a real shame to make a change in one direction that then limited us for another option.

I'll be learning a lot more about Span<T> this month, so will have a more informed viewpoint soon...

[Groostav]

I am strongly considering killing my own proposal at least in the current version. Implementing it efficiently will cost a lot of effort...

I just discovered some recently added code we have on our project that blocked the common pool (a major faux pas) on a call to parseDelimitedFrom(). I suspect I am not alone, without some kind of async facilities several people will be abusing blocking conventions with protobuf.

While I'm sure your concerns about doing this properly and elegantly are correct, understand that it will never be perfect, and the longer you delay, the more chances there are for us litle-protobuf-people to screw up our codebases with your API.

Perhaps some code in experimental namespaces that expose reasonably language-agnostic standards like Futures and Tasks that at least provide the outward-facing semantics you want if not the actual implementation you want, is a good idea.

In other words, what if you drop the implementation changes and instead simply add a little static class

static class AsyncExtensions {
   public static Task<T> parseDelimitedAsync(this Parser<T> parser, T message) {
       NotPerfectButTolerableExecutionContext.submit(() => parser.parseDelimeted(message))
   }
}

or, alternatively, implement the semantics you were suggesting around a buffer or perhaps waitForFirstByte or etc, with similar simple extension functions? Then we can get some feedback about what people like from an experimental namespace, and worry about the speed of the implementation later.

[mkosieradzki]

I had some spare time to implement and run tens of experiments in area of parsing using Span<T>, ReadOnlySequence<T>, Pipe and I have made following observations:

1. Parser on Spans is more roughly 10% percent more efficient than existing parser on .NET Core 2.1 preview 2. It also does zero allocations and is able to work using only single externally provided span.
2. Asynchronous parser using non-contiguous memory from Pipe is roughly 3x slower than the original parser. It is also zero alloc.
3. Parser from point 2 with additional heuristic - trying to get contiguous span buffer and then use parser from 1 otherwise 2 is comparable performance-wise to 1. Worst case ~1.7 times slower. It can be also optimized to use have more contiguous memory “hits” by either increasing min buffer size in pipe from 2048 to something larger or by using stackallocated Span. Both are still zero GC alloc as they are using either stack memory or slab allocator from Pipe.
4. I have also tried to measure alternative parser generated abstraction using interface (virtual calls), but their performance impact is huge so a more generic code gen is probably irrelevant.

Conclusions:
According to my reasearch we need to generate three additional methods:

ValueTask SlowMergeFromAsync(CodedInputReader, CancellationToken)
ValueTask MergeFromAsync(CodedInputReader, CancellationToken)
void MergeFrom(ReadOnlySpan<T>)

MergeFromAsync will attempt to acquire a contiguous buffer and pass it to MergeFrom, otherwise fallback to SlowMergeFromAsync.

MergeFrom will be an optimal span-based parser.
SlowMergeFromAsync will be a generated-state machine (async) for asynchronous parsing of huge messages either not fitting a single buffer or streamed too slowly...

I will try to start implementing a proper PR soon, unless you see some downsides to this approach. I think we can also improve in future CodedInputStream implementation to use a Span-based parser where buffer is large enough.

[mkosieradzki]

I forgot describe what a CodedInputReader is: https://github.com/mkosieradzki/protobuf/blob/spans/csharp/src/Google.Protobuf.Pipelines/CodedInputReader.cs

[AaronViviano]

I think its fine to add support for Async as long as it doesn't replace the existing synchronous APIs. The code I write normally has custom threading implementations and I don't want to rely on the thread pool used by Async.

[betmix-matt]

Now that Dotnet 5 and 6 are out and there is consensus about where the language is moving, many of the concerns raised early on in this issue may be resolved.

Definitely would love to see this implemented.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[ibauersachs]

Async reading from streams is still missing and important.

[jskeet]

I've removed the "inactive" label, but I won't have any time to do work here beyond reviewing. I'm quite happy for someone else to put some time in, but:

• Any PR which requires a breaking change will not be accepted; at least not now. (That could be tricky on its own in terms of interfaces.)
• It may take quite a while for any PR to be assessed
• I'm not going to guarantee that a PR will be accepted; without spending more time than I have available right now, I can't even assess whether it's feasible to do this in a way I'd be comfortable with

I have no problem with the basic premise of adding async support - I'd just caution that many features that sound simple to start with prove extremely difficult within the confines of not accepting breaking changes.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago. This issue will be closed and archived after 14 additional days without activity.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.