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