Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DO NOT MERGE: add span_based_serialization design #6874

Closed
wants to merge 6 commits into from
Closed

DO NOT MERGE: add span_based_serialization design #6874

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
41c92c3
add span_based_serialization design
jtattermusch Nov 12, 2019
1e81823
Apply suggestions from code review
jtattermusch Nov 19, 2019
8ae06c9
Apply suggestions from code review
jtattermusch Jan 6, 2020
bb28883
Update span_based_serialization.md
jtattermusch Jan 6, 2020
47bc281
Update span_based_serialization.md
jtattermusch Jan 7, 2020
d4242bc
Update span_based_serialization.md
jtattermusch Jan 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions csharp/span_based_serialization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# More efficient Protobuf C# serialization/deserialization API

## Background
Currently serialization of protobuf C# messages is done (much like java) through instances of CodedInputStream and CodedOutputStream.
These APIs work well, but there is some inherent overhead due to the fact that they are basically `byte[]` based.
jtattermusch marked this conversation as resolved.
Show resolved Hide resolved
Recently a set of new types has been added to the .NET Framework (Span, ReadOnlySequence, IBufferWriter)
and these new types allow defining new serialization/deserialization APIs, ones that would be much more efficient especially from
the perspective of memory management.

Both gRPC C# implementations (Grpc.Core and grpc-dotnet) already contain functionality that allows exposing request/response payloads
in terms of the new .NET Span APIs in a way that requires no copying of binary payloads and basically allows deserialization directly
from the buffer segments that were received from the network (and serializing directly into the buffer that is going to be sent out
over the network).

## Objective
The goals for this effort:
- keep the existing CodedInputStream and CodedOutputStream (no plans to remove them in the future), serialization/deserialization speed of these APIs should stay unaffected.
- all the changes made need to be backward compatible
- introduce a set of new public APIs for serialization/deserialization that utilize the Span type
- allow future incremental performance optimizations and make them as easy as possible
- ensure the are microbenchmark that allow comparing the two implementations
- as support for the new serialization/deserialization APIs requires generating code that is not compatible with some older .NET frameworks, figure out a schema under which messages with "new serialization API support" and without it can coexist (without causing user friction).
- the effort to maintain the Protobuf C# project should remain approximately the same (should not be negatively affected by introduction of new serialization/deserialization APIs).

Non-goals
- over-optimize the initial version of new serializer/deserializer (we should rather focus on enabling future improvements)
- remove or deprecate the existing serialization/deserialization API

## The Proposal

Key elements:
- Add CodedInputReader and CodedOutputWriter types (both are `ref structs` and thus can hold an instance of `Span`). These two types implement the low-level wire format semantics in terms of the `Span` type (as opposed to `byte[]` used by CodedInputStream and CodedOutputStream).
- Introduce `IBufferMessage` interface (exact name TBD) which inherits from `IMessage` and marks protobuf messages that support the new serialization/deserialization API.
jtattermusch marked this conversation as resolved.
Show resolved Hide resolved
- Introduce a new codegen option (exact name TBD) to enable/disable generating code required by CodedInputReader/CodedInputWriter (the MergeFrom and WriteTo methods). This option will default to disabled. gRPC frameworks will likely provide the ability to explicitly control it, e.g. `<Protobuf Include="..\Shared\benchmark_service.proto" Serialization="Buffer" />`, and/or detect the version of .NET being targeted and automatically enable the option.

jtattermusch marked this conversation as resolved.
Show resolved Hide resolved
A prototype implementation is https://github.com/protocolbuffers/protobuf/pull/5888

## Platform support

- the new serialization API will be supported for projects that support at least `netstandard2.0` and allow use of C# 7.2. On newer .NET frameworks, there might be (and likely will be) further performance benefits.

## Backwards compatibility

## Code duplication

TODO: testing (how to test both APIs without twice the effort).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the test base classes for sharing some tests between the implementations.

Once code gen is complete then tests that serialize/deserialize the entire message can also easily be tested with both. Same story with microbenchmarks.


## Other considerations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New consideration: Messages that wish to use new serialization reader/writer can't use other messages that aren't compiled to also support reader/writer. For this reason, it is important that well known types in Google.Protobuf all support IBufferMessage so they can be used by messages with the new reader/writer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sg, the challenge might be with netstandard1.0 target which is not supported by System.Memory, so we will need to use the PROTOBUF_DISABLE_BUFFER_SERIALIZATION flag in the nestandard1.0 build of the runtime?

Same thing applies to the descriptor.proto types (which aren't classified as WKT, but still ship with the runtime).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I don’t think this is a problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New consideration: can we do something about parsing / serialization of bytes fields? The issue is that if the message contains large ByteString fields, the new parsing API is not going to help much because all the data will be copied into newly allocated byte[] objects. To some extent this is unavoidable, but we should at least give this some thought. AFAIK bytes fields are fairly common in some use cases. e.g. for a hypothetical FileDownloadService, is there a way one could avoid the LOH allocations easily?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new design will save one allocation/copy. Optimizing ByteString is a separate allocation/copy problem.

Renting the underlying storage is one option. However that would require a way for devs to say they are releasing the message.

Create a new issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some solutions to eliminating large ByteString allocations: #4206 (comment)

I think this is an issue to look at separately.


TODO: design for codegen option
TODO: will the generated code contain an #ifdef to enable multi-targeted projects?
jtattermusch marked this conversation as resolved.
Show resolved Hide resolved

TODO: design for Grpc.Tools and whether the span-based parsing will be enabled by default

TODO: how will the old generated code coexist with the span-enabled generated code? how will it work across library boundaries (e.g. a client library contains code generated without the support for fast parsing and we want to use that client library).
jtattermusch marked this conversation as resolved.
Show resolved Hide resolved

## References

This proposal is based on previous work / discussions:
- https://github.com/grpc/grpc-dotnet/issues/30
- https://github.com/protocolbuffers/protobuf/issues/3431
- https://github.com/protocolbuffers/protobuf/pull/4988
- https://github.com/jtattermusch/protobuf/pull/8