Consider providing an extension point allowing users to configure serialization libraries other than Google.Protobuf.
Grpc.Core already offers an abstraction layer that we can use.
Issue is for discussing improving it and Google.Protobuf (default serialization layer)
Start from https://github.com/grpc/grpc-dotnet/issues/30#issuecomment-457438814
Consider providing an extension point allowing users to configure serialization libraries other than Google.Protobuf.
Right now Google.Protobuf is used for:
- Determining the methods via its service descriptor
As noted in https://github.com/grpc/grpc-dotnet/issues/21#issuecomment-453960976 this would not work when using other serialization format.
- Serialization/deserialization between .NET messages and protobuf wire format
Two design questions:
- Do we want Grpc.AspNetCore.* to not have a reference to Google.Protobuf. Right now we use types from it related to the descriptor on a grpc service to discover the methods on a service. Changing this is possible but would require changes to what Grpc.Tools generates. See #21
I think Grpc.AspNetCore should not have a dependency on Google.Protobuf as
gRPC's philosophy is to be serialization format agnostic. We do provide easy-to-use Google.Protobuf support as the default choice, but in most implementations we do not take a direct dependency on Google.Protobuf. Support for other serialization formats (Flatbuffers, Bond, Thrift and many others) should be provided as add-on packages (provided by the community) and should not be part of the core libraries as there is many serialization formats out there and each one has its specifics.
- Do we want Grpc.AspNetCore.* to support serialization libraries other than Google.Protobuf? For example, a C# version of FlatBuffers. Providing an interface like SignalR's IHubProtocol would allow Google.Protobuf's serialization logic to be hidden behind an interface, and let users configure their own implementation.
We should make it possible to use other serialization formats and expect users to do that at some point. I'd follow the pattern used by Grpc.Core: The only thing that Grpc.Core currently assumes is that a serialized message will be an instance of a class and that one defines a "Marshaller" that knows how to serialize/deserialize (invoking the protobuf-specific serialization/deserialization is part of the generated code right now: https://github.com/grpc/grpc/blob/b6d8957b0d78f768b858698ba6a79296db448bb2/src/csharp/Grpc.Examples/MathGrpc.cs#L30)
To use a different serialization format, one needs to write their own codegen and possibly their own version of Grpc.Tools package for build integration.
https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Core/Marshaller.cs
An issue with the current Marshaller pattern is it operates on byte arrays. We'd want to have a pattern that doesn't require allocating an array and buffering to memory. For best performance we'd ideally read directly from the request and write directly to the response.
For serialization extensibility, do you expect compatible serializer libraries to always generate a BindService method? Would you recommend BindService as the way Grpc.AspNetCore.Server discovers services and aquires marshallers?
If the answer is yes, I think there are two changes we would want to make:
Update Grpc.Tools to output a BindService method that doesn't take an instance and add a new AddMethod overload, ServiceBinderBase.AddMethod<TRequest, TResponse>(Method<TRequest, TResponse>), that it would call. This relates to https://github.com/grpc/grpc-dotnet/issues/21
Once we can use BindService then we should only depend on Grpc.Core types. Swapping the serialization would only involve changing the marshaller in generated code.
/// <summary>Register service method implementations with a service binder. Useful when customizing the service binding logic.
/// Note: this method is part of an experimental API that can change or be removed without any prior notice.</summary>
/// <param name="serviceBinder">Service methods will be bound by calling <c>AddMethod</c> on this object.</param></param>
public static void BindService(grpc::ServiceBinderBase serviceBinder)
{
serviceBinder.AddMethod(__Method_Div);
serviceBinder.AddMethod(__Method_DivMany);
serviceBinder.AddMethod(__Method_Fib);
serviceBinder.AddMethod(__Method_Sum);
}
(1) is something to figure out now. It will be the main way Grpc.AspNetCore.Server integrates with Grpc.Core so it is something to lock down.
(2) is largely an API detail, and is something that doesn't need to be figured out right away. I'd like to learn more about how gRPC and ASP.NET Core interact before deciding what the best signature would be. ASP.NET Core will soon expose System.IO.Pipelines in HTTP2 and it will provide the best opportunity to maximize performance.
https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Core/Marshaller.cs
An issue with the current Marshaller pattern is it operates on byte arrays. We'd want to have a pattern that doesn't require allocating an array and buffering to memory. For best performance we'd ideally read directly from the request and write directly to the response.
I am aware of that limitation and I've had a solution in progress for a while:
The idea is to switch the "simple marshaller" (which converts class into byte array and back). Into a "contextual marshaller" which also takes context which allow multiple ways of accessing the payload (e.g. accessing ReadOnlySpan with data slice by slice etc or using a rented buffer).
Grpc.Core already uses contextual serializers internally (but they delegate to the old-style byte array conversions for now).
https://github.com/grpc/grpc/pull/16367
https://github.com/grpc/grpc/pull/17167
Serialization/DeserializationContext is not populated yet with the advanced members, but I have a PR in progress. A taste of new DeserializationContext (not yet merged):
https://github.com/grpc/grpc/pull/16371/files#diff-22501e836d94b70164ce50d8b0c0c4e5
Btw, this effort started before we came up with the plan for Grpc.AspNetCore, so it would be good to review if this design works well with the planned APIs for Asp.NET Core pipeline access to requests/responses - let's create a separate issue for this review?
For serialization extensibility, do you expect compatible serializer libraries to always generate a BindService method? Would you recommend BindService as the way Grpc.AspNetCore.Server discovers services and aquires marshallers?
If the answer is yes, I think there are two changes we would want to make:
I think the answer is Yes.
- Update Grpc.Tools to output a BindService method that doesn't take an instance and add a new AddMethod overload,
ServiceBinderBase.AddMethod<TRequest, TResponse>(Method<TRequest, TResponse>), that it would call. This relates to #21
Once we can use BindService then we should only depend on Grpc.Core types. Swapping the serialization would only involve changing the marshaller in generated code./// <summary>Register service method implementations with a service binder. Useful when customizing the service binding logic. /// Note: this method is part of an experimental API that can change or be removed without any prior notice.</summary> /// <param name="serviceBinder">Service methods will be bound by calling <c>AddMethod</c> on this object.</param></param> public static void BindService(grpc::ServiceBinderBase serviceBinder) { serviceBinder.AddMethod(__Method_Div); serviceBinder.AddMethod(__Method_DivMany); serviceBinder.AddMethod(__Method_Fib); serviceBinder.AddMethod(__Method_Sum); }
- Update Marshaller with Serializer/Deserializer properties that allow message data to be written and read without allocating byte arrays.
(1) is something to figure out now. It will be the main way Grpc.AspNetCore.Server integrates with Grpc.Core so it is something to lock down.
I think we can add that soon. If you want, you can also create a PR yourself, here is an example PR that added ServiceBinderBase: https://github.com/grpc/grpc/pull/17157/commits
(2) is largely an API detail, and is something that doesn't need to be figured out right away. I'd like to learn more about how gRPC and ASP.NET Core interact before deciding what the best signature would be. ASP.NET Core will soon expose System.IO.Pipelines in HTTP2 and it will provide the best opportunity to maximize performance.
This is what ContextualSerializer/ContextualDeserializer can help with (see above). Let's review in a separate issue to make sure the (De)SerializerContext has methods that play well with both Grpc.Core and Grpc.AspNetCore (which will use System.IO.Pipelines).
I think we can add that soon. If you want, you can also create a PR yourself, here is an example PR that added ServiceBinderBase: grpc/grpc/pull/17157/commits
Sure, we'll create a PR for it.
This is what ContextualSerializer/ContextualDeserializer can help with (see above). Let's review in a separate issue to make sure the (De)SerializerContext has methods that play well with both Grpc.Core and Grpc.AspNetCore (which will use System.IO.Pipelines).
Great. ASP.NET Core currently doesn't expose pipelines, but it will come in the next preview of ASP.NET Core 3.0. That shouldn't be too far away. We'll look at whether the contextual serializers are the right shape then.
FYI @davidfowl
We'll need another implementation to vet the abstraction. We can try using the other popular .NET protobuf library as a test https://www.nuget.org/packages/protobuf-net/.
I'm not sure that Protobuf.NET supports proto3 (which is the recommended version of .proto format to use and the only one supported by Google.Protobuf for C#).
An alternative would be to try with Flatbuffers https://github.com/google/flatbuffers/tree/master/net/FlatBuffers that have the advantage of zero-overhead serialization/deserialization so could be useful for performance testing.
But it terms of vetting the abstraction, I think just the ability to expose a "generic" service (e.g. a method handler that gives access to the actual binary payloads instead of serialized/deserialized messages would be good enough). E.g. here: https://github.com/grpc/grpc/blob/1064c6f663a2735fa2f4230dd8d278ae66d344b7/src/csharp/Grpc.IntegrationTesting/ServerRunners.cs#L92
I've renamed this issue. Grpc.AspNetCore should use Grpc.Core's abstraction.
This is what ContextualSerializer/ContextualDeserializer can help with (see above). Let's review in a separate issue to make sure the (De)SerializerContext has methods that play well with both Grpc.Core and Grpc.AspNetCore (which will use System.IO.Pipelines).
I think we should have a meeting about this: @jtattermusch, @davidfowl, @JunTaoLuo and I.
My high-level thoughts after some research:
Stream. Unless we want to make large changes to that code base, or switch to another serializer, then the Grpc.Core API will need logic to interop with streamsResponse.PipeBody, an IBufferWriter<byte>.IBufferWriter<byte>.Request.BodyPipe.ReadAsync() to get the message header with the message length, then read the complete message into a ReadOnlySequence<byte>.IBufferWriter<byte>.ReadOnlySequence<byte>. Async happens when reading the request into the sequence.I've renamed this issue. Grpc.AspNetCore should use Grpc.Core's abstraction.
This is what ContextualSerializer/ContextualDeserializer can help with (see above). Let's review in a separate issue to make sure the (De)SerializerContext has methods that play well with both Grpc.Core and Grpc.AspNetCore (which will use System.IO.Pipelines).
I think we should have a meeting about this: @jtattermusch, @davidfowl, @JunTaoLuo and I.
My high-level thoughts after some research:
- Google.Protobuf serialization is based around
Stream. Unless we want to make large changes to that code base, or switch to another serializer, then the Grpc.Core API will need logic to interop
with streams
A bit more detail here:
The two main classes are https://github.com/protocolbuffers/protobuf/blob/master/csharp/src/Google.Protobuf/CodedInputStream.cs and CodedOutputStream. All the operations are actually implemented over a byte array (including when reading/writing to from stream), the streams are only used to read/write the next chunk of data once the buffer gets full. Using a stream is not necessary, one can provide her own buffer directly (doesn't really help us in our situation, but it's good to know). The problem with using Span<> instead of a byte array here is that all the parsing state is kept in the CodedInputStream instance (and Span<> can't be stored there).
Grpc.Server's most efficient way to write: write directly to
Response.PipeBody, anIBufferWriter<byte>.
- It will be simple to create a Stream implementation that writes to a
IBufferWriter<byte>.- Getting the message length will need to be part of the marshaller API. It needs to be written first.
- Google.Protobuf writing is sync. An async flush can happen after the message has been written to the buffer writer.
Grpc.Server's most efficient way to read:
Request.BodyPipe.ReadAsync()to get the message header with the message length, then read the complete message into aReadOnlySequence<byte>.
- It will be simple to create a Stream implementation that wraps a
IBufferWriter<byte>.- Google.Protobuf reading is sync from the stream wrapping
ReadOnlySequence<byte>. Async happens when reading the request into the sequence.Updating Grpc.Tools output needs to be updated
- Today ContextualSerializer/ContextualDeserializer use the array methods.
Let's go over this in our next weekly sync.
Hmm, yeah I looked at the implementation of CodedInputStream and CodedOutputStream. They are built around byte arrays, either as the final place for data or a buffer to then write to a stream. Reading and writing directly with pipelines isn't possible with the current Google.Protobuf API. Unfortunately those types are both sealed.
@JamesNK https://github.com/mkosieradzki/protobuf/tree/spans-pr-rebased - as far as I remember - this specific branch in my forked repo contains implementation that is compatible with pipelines and is pretty much optimized, but it modifies both codegen and the sdk.
It should be also significantly faster, because it exercises aggressive inlining instead of virtual calls.
Thanks for the link.
The CodedOutputStream changes are understandable just from reading the code. Nice job!
CodedInputStream is more complicated. I want to debug it at runtime to understand it fully.
+1 on discussing this at sync next week. Getting performance to 100% is lower priority that functionality for the first preview, but a plan forward would be good 馃憤
After playing with this for a little bit, we can continue to use the marshaller for now but we need some changes to it. Instead of taking a byte[] it needs to take a Span<byte> or ReadOnlySequence<byte> or both. Being a byte[] today forces the caller to allocate which is unfortunate.
ReadOnlySequence<byte> for reading. IBufferWriter<byte> for writing.
If you look at the new pipelines code you can see the exact points where the marshaller could slot in and use those types seamlessly.
We need to consider targeting for this feature. IBufferWriter<byte> only ships in the box with new versions of netcoreapp. Google.Protobuf will either need to target netcoreapp2.x or (more likely) have netstandard2.0 target that brings in the System.Memory package.
Because Grpc.Tools supports older versions of .NET, codegen will still continue to output serialization logic for byte[] by default, and there would be an opt-in flag on <Protobuf> to enable the new serialization. Our templates can have this flag set by default.
Interesting project.
I'm already implements gRPC based framework that does not require .proto file and use MessagePack insteadof Protocol Buffers.
https://github.com/Cysharp/MagicOnion
Therefore, it is desirable that the serialization has a structure that does not depend on proto.
I'm also author of MessagePack-Csharp, currently AArnott working support Span and others. https://github.com/AArnott/MessagePack-CSharp/issues/9
// byte[] based formatter(ver 1.x) to span based formatter(ver 2.x)
public interface IMessagePackFormatter<T>
{
void Serialize(IBufferWriter<byte> writer, T value, IFormatterResolver resolver);
T Deserialize(ref ReadOnlySequence<byte> byteSequence, IFormatterResolver resolver);
}
If you change the marshaller, this can be supported directly.
@mkosieradzki Are you still willing to help out here?
@davidfowl yes. What kind of help is needed? My protobuf fork should be compatible with those requirements.
We're going to look at perf some more now that a lot of basic functionality is working.
@mkosieradzki I think your PR goes a long way to adding the right features to protobuf. Are you able to rebase it on the latest version of google/protobuf?
@JamesNK I will try to revisit and rebase it soon. If I fail to find some time during week, I will definitely do this on the upcoming Saturday.
That would be great. I'm going to put together some microbenchmarks for serialization/deserialization performance.
I'm also going to see about getting benchmarks in the ASP.NET Core and protobuf perf environments. That should track performance over time.
@JamesNK I started going through the merge, it's not that easy, because it seems that legacy proto2 support has been added to the C# version, after creating the initial PR. So it's not only merging, but also re-implementing parts of proto2 support.
Please let me know, whether you need my code updated to the latest branch, because you need features like proto2 support (I see scenarios related to gRPC where this will be useful), or just to have the latest version. It will allow me to prioritize my work.
I will also need to know if the intention is to have this code merged back into protobuf, in that case we need to go back with @jtattermusch to the painful PR process, or do we need a span-friendly fork that works.
For the PoC purposes - this branch should be good enough: https://github.com/mkosieradzki/protobuf/tree/spans-pr-rebased
Lets figure out a plan before we spend a great amount of time on this. I'll bring this up in a meeting tomorrow and we'll figure out the best way to make improvements in Google.Protobuf and Grpc.Core.
From discussion between the MS, gRPC and Protobuf teams...
@mkosieradzki I think we'll measure against your PR, but for now we're going to go with new implementations of the streams.
@JamesNK I think that ref struct is a good pick (I was considering it as well).
Performance-wise the most important thing is to avoid virtual calls on the serialization/deserialization hot-path (and of course async ;) ).
My suggestion would be:
Span parameter)Here's what it'll look like look at this message pack PR https://github.com/AArnott/MessagePack-CSharp/pull/27 and https://github.com/AArnott/MessagePack-CSharp/pull/40
One of my crazy ideas ;) was to hide the performance regressions from spans in .NET Framework behind the benefits from virtcall elimination - this way no one would notice a regression :)
it seems that legacy proto2 support has been added to the C# version
hide the performance regressions from spans in .NET Framework behind the benefits from virtcall elimination - this way no one would notice a regression :)
I like your thinking, @mkosieradzki. I'm considering some optimizations to MessagePack to "hide" regressions as well that otherwise are showing up in this transition. Of course it would be great to have even better perf from the transition than before, at least on .NET Core. But in the MessagePack case at least, @neuecc did such a good job of optimizing it that it's hard for it to get much better. ;-p
Before proceeding to implementation let's agree on the API design (the actual implementation is a lot of code and that can easily obscures the important concepts).
Here's some proposal for the parsing API (to be iterated upon):
// TODO: class name TBD (CodedInputReader, CodedInputContext, CodedInputByRefReader, WireFormatReader)
public ref struct CodedInputReader
{
// to avoid accessing first slice from inputSequence all the time,
// also to allow reading from single span.
ReadOnlySpan<byte> immediateBuffer;
ReadOnlySequence<byte> inputSequence; // struct!
// maybe we need bufferPos to avoid incrementing totalBytesRetired all the time..
// int bufferPos;
public CodedInputReader(ReadOnlySpan<byte> buffer)
{
this.immediateBuffer = buffer;
this.inputSequence = default;
}
public CodedInputReader(ReadOnlySequence<byte> input)
{
this.immediateBuffer = default;
this.inputSequence = input;
}
// TODO: add constructor that allows reading from a single span, buts calls a refill callback
// if span has been exhausted to allow supplying the next buffer slice?
// (= same as read only sequence, but allows supplying buffer slices reading lazily)
// internal state of coded input stream
private uint lastTag;
private uint nextTag;
private bool hasNextTag;
private int totalBytesRetired;
private int currentLimit; // initialize to maxInt!
private int recursionDepth;
// parsing limits
private readonly int recursionLimit;
private readonly int sizeLimit;
// exposed methods that copy potentially large amount of data:
// TODO: figure out an opt-in way to avoid copying and/or allocation
// ReadBytes()
// ReadString();
}
// TODO: add constructor that allows reading from a single span, buts calls a refill callback
// if span has been exhausted to allow supplying the next buffer slice?
// (= same as read only sequence, but allows supplying buffer slices reading lazily)
You could but I wouldn't start with that.
We may also want to make it possible to return a state object like the Utf8JsonReader does:
@ahsonkhan Can you lend your experience here with the JsonReader.
See the other state also.
>
// exposed methods that copy potentially large amount of data:
// TODO: figure out an opt-in way to avoid copying and/or allocation
// ReadBytes()
// ReadString()
You could expose the raw input data slices similar to what the Utf8JsonReader does and let the caller transcode themselves:
https://github.com/dotnet/corefx/blob/cc42bb95b0877a653d4f556a0839d261ea3563b7/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L83-L104
And if you are slicing ReadOnlySequence you likely want to track the SequencePositions as well:
https://github.com/dotnet/corefx/blob/cc42bb95b0877a653d4f556a0839d261ea3563b7/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L42-L45
We may also want to make it possible to return a state object like the Utf8JsonReader does
I am not too familiar with the types of input CodedInputReader would accept, but when it comes to returning/accepting a state for partial reading (and to support re-entrancy), you may also want an isFinalBlock bool.
Additionally, you would likely need to know if you are at the end or not:
C#
private bool IsLastSpan => _isFinalBlock && (_isSingleSegment || _isLastSegment);
private int totalBytesRetired;
Is this the amount of bytes read from the input so far?
// exposed methods that copy potentially large amount of data: // TODO: figure out an opt-in way to avoid copying and/or allocation // ReadBytes() // ReadString()You could expose the raw input data slices similar to what the Utf8JsonReader does and let the caller transcode themselves:
https://github.com/dotnet/corefx/blob/cc42bb95b0877a653d4f556a0839d261ea3563b7/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L83-L104
That sounds like a good start, but in our case, the caller will be the Google.Protobuf assembly itself, so we would also need to figure out an alternative way of exposing the bytestring in the message api (this would be an advanced use case for very performance-oriented users I assume).
And if you are slicing
ReadOnlySequenceyou likely want to track theSequencePositionsas well:
https://github.com/dotnet/corefx/blob/cc42bb95b0877a653d4f556a0839d261ea3563b7/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L42-L45We may also want to make it possible to return a state object like the Utf8JsonReader does
I am not too familiar with the types of input CodedInputReader would accept, but when it comes to returning/accepting a state for partial reading (and to support re-entrancy), you may also want an
isFinalBlockbool.Additionally, you would likely need to know if you are at the end or not:
private bool IsLastSpan => _isFinalBlock && (_isSingleSegment || _isLastSegment);
I like the idea of being able to save the parsing state using *State and making the parser re-entrant, but the problem is that unlike JSON parse, it's not enough to just remember the parsing state with a few booleans and a BitState - the parser will be recursively descending into parsing different protobuf messages and we would need to remember the exact IMessage instances on the stack (which probably involves allocating some data structure to remember these) and it might end up being too complex.
private int totalBytesRetired;
Is this the amount of bytes read from the input so far?
See comment here:
https://github.com/protocolbuffers/protobuf/blob/2d0183ab58706a919f138e6920e33e3b76eb62f6/csharp/src/Google.Protobuf/CodedInputStream.cs#L105
We'd also need to prototype the solution for maintaining backwards compatibility.
Here's what we talked about offline:
netstandardX.Y (exact version TBD, the minimum requirement for System.Memory is netstandard1.1)#if as c# 7.2 is needed to compile ref structs in the generated code)CC @jskeet
When used with gRPC the protobuf serializer doesn't need to be re-entrant. gRPC will prefix the message with its size in bytes so it is simple to read from the request pipeline until we have all the necessary data to deserialize the message.
Which is why we have a max buffer size 馃槃
This is DONE with gRPC 2.32.0 and Protobuf 3.13.0 馃帀 馃コ 馃巿 馃嵃
@JamesNK What's a good entry point for grokking the final design (e.g. docs, examples, tests, etc.)?
Most helpful comment
Interesting project.
I'm already implements gRPC based framework that does not require .proto file and use MessagePack insteadof Protocol Buffers.
https://github.com/Cysharp/MagicOnion
Therefore, it is desirable that the serialization has a structure that does not depend on proto.
I'm also author of MessagePack-Csharp, currently AArnott working support Span and others. https://github.com/AArnott/MessagePack-CSharp/issues/9
If you change the marshaller, this can be supported directly.