DO NOT MERGE: add span_based_serialization design #6874
Conversation
csharp/span_based_serialization.md
Outdated
|
|
||
| ## Code duplication | ||
|
|
||
| TODO: testing (how to test both APIs without twice the effort). |
There was a problem hiding this comment.
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.
csharp/span_based_serialization.md
Outdated
|
|
||
| TODO: testing (how to test both APIs without twice the effort). | ||
|
|
||
| ## Other considerations |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Yes. I don’t think this is a problem
Co-Authored-By: James Newton-King <james@newtonking.com>
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
csharp/span_based_serialization.md
Outdated
|
|
||
| TODO: testing (how to test both APIs without twice the effort). | ||
|
|
||
| ## Other considerations |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Some solutions to eliminating large ByteString allocations: #4206 (comment)
I think this is an issue to look at separately.
Co-Authored-By: James Newton-King <james@newtonking.com>
jtattermusch
changed the title
|
CC @haberman can you take a look? |
|
|
||
| ## Concern: Code duplication and maintainability | ||
|
|
||
| While the logical structure of the (de)serialization code for both old and new approaches is very similar, due to the different nature of the object holding the parsing state (`class` vs `ref struct`), the parsing primitives need to be defined twice (e.g. ReadRawVarint64 exist in two copies, one under CodedInputStream and one user CodedInputReader) and that adds some extra overhead when: |
There was a problem hiding this comment.
@haberman according to you, how much of a concern is this? Is there a precedent for this within protobuf (having 2 sets of code for serialization/parsing in one language)?
There was a problem hiding this comment.
We had this as a transitional state when Gerben was rewriting the parser, but that was just temporary. Java is in a state where there are two very different models for generated code (one table-based), but I'm not sure how much extra runtime come
Looking at the CL, I do worry that this is an awful lot of new code. If I filter out all the generated code, this CL is still +7,000 lines of code, which is worrying.
$ git diff --stat master ':!csharp/src/Google.Protobuf.Test.TestProtos' ':!csharp/src/Google.Protobuf.Benchmarks/WrapperBenchmarkMessages.cs' ':!csharp/src/Google.Protobuf/Reflection/Descriptor.cs' ':!csharp/src/Google.Protobuf/WellKnownTypes' ':!csharp/src/Google.Protobuf.Benchmarks/Benchmarks.cs' ':!csharp/src/Google.Protobuf.Benchmarks/BenchmarkMessage1Proto3.cs' ':!csharp/src/AddressBook/Addressbook.cs'
.gitignore | 3 +
Makefile.am | 15 +-
conformance/Makefile.am | 1 +
csharp/generate_protos.sh | 11 +-
csharp/src/Google.Protobuf.Benchmarks/Google.Protobuf.Benchmarks.csproj | 10 +-
csharp/src/Google.Protobuf.Benchmarks/GoogleMessageBenchmark.cs | 114 ++++++++++
csharp/src/Google.Protobuf.Benchmarks/Program.cs | 3 +-
csharp/src/Google.Protobuf.Benchmarks/SerializationBenchmark.cs | 118 +++++++++-
csharp/src/Google.Protobuf.Benchmarks/SerializationConfig.cs | 16 +-
csharp/src/Google.Protobuf.Benchmarks/WrapperBenchmark.cs | 47 +++-
csharp/src/Google.Protobuf.Conformance/Conformance.cs | 276 ++++++++++++++++++++++-
csharp/src/Google.Protobuf.Conformance/Google.Protobuf.Conformance.csproj | 5 +
csharp/src/Google.Protobuf.Conformance/Program.cs | 89 ++++++--
csharp/src/Google.Protobuf.Test/Buffers/ArrayBufferWriter.cs | 215 ++++++++++++++++++
csharp/src/Google.Protobuf.Test/Buffers/MaxSizeHintBufferWriter.cs | 76 +++++++
csharp/src/Google.Protobuf.Test/Buffers/ReadOnlySequenceFactory.cs | 130 +++++++++++
csharp/src/Google.Protobuf.Test/{CodedInputStreamExtensions.cs => CodedInputExtensions.cs} | 18 +-
csharp/src/Google.Protobuf.Test/CodedInputReaderTest.cs | 419 ++++++++++++++++++++++++++++++++++
csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs | 425 ++++++++++------------------------
csharp/src/Google.Protobuf.Test/CodedInputTestBase.cs | 279 +++++++++++++++++++++++
csharp/src/Google.Protobuf.Test/CodedOutputStreamTest.cs | 398 ++++++++++++++------------------
csharp/src/Google.Protobuf.Test/CodedOutputTestBase.cs | 294 ++++++++++++++++++++++++
csharp/src/Google.Protobuf.Test/CodedOutputWriterTest.cs | 314 ++++++++++++++++++++++++++
csharp/src/Google.Protobuf.Test/Collections/RepeatedFieldTest.cs | 55 ++++-
csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs | 14 +-
csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs | 20 +-
csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs | 206 +++++++++++++----
csharp/src/Google.Protobuf.Test/Google.Protobuf.Test.csproj | 4 +
csharp/src/Google.Protobuf.Test/JsonParserTest.cs | 4 +
csharp/src/Google.Protobuf.Test/JsonTokenizerTest.cs | 4 +
csharp/src/Google.Protobuf.Test/MessageParsingHelpers.cs | 145 ++++++++++++
csharp/src/Google.Protobuf.Test/UnknownFieldSetTest.cs | 24 +-
csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs | 150 ++++++------
csharp/src/Google.Protobuf.Test/testprotos.pb | Bin 330948 -> 327492 bytes
csharp/src/Google.Protobuf/ByteString.cs | 7 +-
csharp/src/Google.Protobuf/CodedInputReader.cs | 1339 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
csharp/src/Google.Protobuf/CodedOutputWriter.cs | 702 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
csharp/src/Google.Protobuf/Collections/MapField.cs | 121 +++++++++-
csharp/src/Google.Protobuf/Collections/RepeatedField.cs | 87 ++++++-
csharp/src/Google.Protobuf/ExtensionRegistry.cs | 4 +-
csharp/src/Google.Protobuf/ExtensionSet.cs | 48 +++-
csharp/src/Google.Protobuf/ExtensionValue.cs | 57 ++++-
csharp/src/Google.Protobuf/FieldCodec.cs | 480 ++++++++++++++++++++++++++++++++++++---
csharp/src/Google.Protobuf/Google.Protobuf.csproj | 21 +-
csharp/src/Google.Protobuf/IBufferMessage.cs | 57 +++++
csharp/src/Google.Protobuf/IMessage.cs | 2 +-
csharp/src/Google.Protobuf/MessageExtensions.cs | 25 ++
csharp/src/Google.Protobuf/MessageParser.cs | 81 +++++++
csharp/src/Google.Protobuf/SequenceReader.cs | 417 ++++++++++++++++++++++++++++++++++
csharp/src/Google.Protobuf/UnknownField.cs | 53 +++++
csharp/src/Google.Protobuf/UnknownFieldSet.cs | 105 ++++++++-
src/google/protobuf/compiler/csharp/csharp_bootstrap_unittest.cc | 4 +-
src/google/protobuf/compiler/csharp/csharp_field_base.cc | 2 +-
src/google/protobuf/compiler/csharp/csharp_field_base.h | 2 +
src/google/protobuf/compiler/csharp/csharp_generator.cc | 2 +
src/google/protobuf/compiler/csharp/csharp_map_field.cc | 13 ++
src/google/protobuf/compiler/csharp/csharp_map_field.h | 2 +
src/google/protobuf/compiler/csharp/csharp_message.cc | 159 ++++++++++---
src/google/protobuf/compiler/csharp/csharp_message.h | 3 +
src/google/protobuf/compiler/csharp/csharp_message_field.cc | 10 +
src/google/protobuf/compiler/csharp/csharp_message_field.h | 2 +
src/google/protobuf/compiler/csharp/csharp_options.h | 7 +-
src/google/protobuf/compiler/csharp/csharp_primitive_field.cc | 10 +
src/google/protobuf/compiler/csharp/csharp_primitive_field.h | 2 +
src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.cc | 13 ++
src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.h | 2 +
src/google/protobuf/compiler/csharp/csharp_repeated_message_field.cc | 13 ++
src/google/protobuf/compiler/csharp/csharp_repeated_message_field.h | 2 +
src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.cc | 13 ++
src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.h | 2 +
src/google/protobuf/compiler/csharp/csharp_wrapper_field.cc | 32 +++
src/google/protobuf/compiler/csharp/csharp_wrapper_field.h | 4 +
72 files changed, 6986 insertions(+), 821 deletions(-)
The doubled generated code seems like a potential concern for code size also. I guess C# probably isn't used much in any kind of mobile (phone) deployment, but probably some people eventually have code size concerns?
A few specific questions:
- Once
Spanis available everywhere, could the old code simply become a client of the new code? - Is it possible that some of the code can be shared by using stateless functions where all input and output is represented as explicit parameters?
- Is it necessary to expose the new reader/writer types to the user? Ideally the user could take advantage of the new code without needing to construct a
CodedInputReaderfor example, but could just directly pass theReadOnlySequence<byte>or whatever that they want to parse from.
There was a problem hiding this comment.
We had this as a transitional state when Gerben was rewriting the parser, but that was just temporary. Java is in a state where there are two very different models for generated code (one table-based), but I'm not sure how much extra runtime come
Looking at the CL, I do worry that this is an awful lot of new code. If I filter out all the generated code, this CL is still +7,000 lines of code, which is worrying.
Yes, it's a big PR. Here's some more insights:
-
A lot of the code changes is tests - we've added a lot of new tests and where possible, we modified the tests to test both old and the new parser at the same time (so some line changes in the tests are just moving tests around - e.g. from CodedInputStreamTest to CodedInputTestBase where it can be used to ). Many of the changes in the tests are mechanic - just making sure that both serializer/parser implementations get tested
-
The changes in the codegen plugin (src/google/protobuf/compiler/csharp) are very mechanic and barely have any logic
-
The actual new logic is basically two files - CodedInputReader and CodedOutputWriter ("new" version of CodedInputStream and CodedOuputStream) and adding support for those in things like Maps, RepeatedFields etc. Altogether this is ~2500 lines but CodedInputReader and CodedOuputWriter is modeled after CodedInputStream and CodedOutputStream and the high-level logic stays the same. It's the low-level read from buffer operations and parsing the primitives that's different.
$ git diff --stat master ':!csharp/src/Google.Protobuf.Test.TestProtos' ':!csharp/src/Google.Protobuf.Benchmarks/WrapperBenchmarkMessages.cs' ':!csharp/src/Google.Protobuf/Reflection/Descriptor.cs' ':!csharp/src/Google.Protobuf/WellKnownTypes' ':!csharp/src/Google.Protobuf.Benchmarks/Benchmarks.cs' ':!csharp/src/Google.Protobuf.Benchmarks/BenchmarkMessage1Proto3.cs' ':!csharp/src/AddressBook/Addressbook.cs' .gitignore | 3 + Makefile.am | 15 +- conformance/Makefile.am | 1 + csharp/generate_protos.sh | 11 +- csharp/src/Google.Protobuf.Benchmarks/Google.Protobuf.Benchmarks.csproj | 10 +- csharp/src/Google.Protobuf.Benchmarks/GoogleMessageBenchmark.cs | 114 ++++++++++ csharp/src/Google.Protobuf.Benchmarks/Program.cs | 3 +- csharp/src/Google.Protobuf.Benchmarks/SerializationBenchmark.cs | 118 +++++++++- csharp/src/Google.Protobuf.Benchmarks/SerializationConfig.cs | 16 +- csharp/src/Google.Protobuf.Benchmarks/WrapperBenchmark.cs | 47 +++- csharp/src/Google.Protobuf.Conformance/Conformance.cs | 276 ++++++++++++++++++++++- csharp/src/Google.Protobuf.Conformance/Google.Protobuf.Conformance.csproj | 5 + csharp/src/Google.Protobuf.Conformance/Program.cs | 89 ++++++-- csharp/src/Google.Protobuf.Test/Buffers/ArrayBufferWriter.cs | 215 ++++++++++++++++++ csharp/src/Google.Protobuf.Test/Buffers/MaxSizeHintBufferWriter.cs | 76 +++++++ csharp/src/Google.Protobuf.Test/Buffers/ReadOnlySequenceFactory.cs | 130 +++++++++++ csharp/src/Google.Protobuf.Test/{CodedInputStreamExtensions.cs => CodedInputExtensions.cs} | 18 +- csharp/src/Google.Protobuf.Test/CodedInputReaderTest.cs | 419 ++++++++++++++++++++++++++++++++++ csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs | 425 ++++++++++------------------------ csharp/src/Google.Protobuf.Test/CodedInputTestBase.cs | 279 +++++++++++++++++++++++ csharp/src/Google.Protobuf.Test/CodedOutputStreamTest.cs | 398 ++++++++++++++------------------ csharp/src/Google.Protobuf.Test/CodedOutputTestBase.cs | 294 ++++++++++++++++++++++++ csharp/src/Google.Protobuf.Test/CodedOutputWriterTest.cs | 314 ++++++++++++++++++++++++++ csharp/src/Google.Protobuf.Test/Collections/RepeatedFieldTest.cs | 55 ++++- csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs | 14 +- csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs | 20 +- csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs | 206 +++++++++++++---- csharp/src/Google.Protobuf.Test/Google.Protobuf.Test.csproj | 4 + csharp/src/Google.Protobuf.Test/JsonParserTest.cs | 4 + csharp/src/Google.Protobuf.Test/JsonTokenizerTest.cs | 4 + csharp/src/Google.Protobuf.Test/MessageParsingHelpers.cs | 145 ++++++++++++ csharp/src/Google.Protobuf.Test/UnknownFieldSetTest.cs | 24 +- csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs | 150 ++++++------ csharp/src/Google.Protobuf.Test/testprotos.pb | Bin 330948 -> 327492 bytes csharp/src/Google.Protobuf/ByteString.cs | 7 +- csharp/src/Google.Protobuf/CodedInputReader.cs | 1339 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ csharp/src/Google.Protobuf/CodedOutputWriter.cs | 702 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ csharp/src/Google.Protobuf/Collections/MapField.cs | 121 +++++++++- csharp/src/Google.Protobuf/Collections/RepeatedField.cs | 87 ++++++- csharp/src/Google.Protobuf/ExtensionRegistry.cs | 4 +- csharp/src/Google.Protobuf/ExtensionSet.cs | 48 +++- csharp/src/Google.Protobuf/ExtensionValue.cs | 57 ++++- csharp/src/Google.Protobuf/FieldCodec.cs | 480 ++++++++++++++++++++++++++++++++++++--- csharp/src/Google.Protobuf/Google.Protobuf.csproj | 21 +- csharp/src/Google.Protobuf/IBufferMessage.cs | 57 +++++ csharp/src/Google.Protobuf/IMessage.cs | 2 +- csharp/src/Google.Protobuf/MessageExtensions.cs | 25 ++ csharp/src/Google.Protobuf/MessageParser.cs | 81 +++++++ csharp/src/Google.Protobuf/SequenceReader.cs | 417 ++++++++++++++++++++++++++++++++++ csharp/src/Google.Protobuf/UnknownField.cs | 53 +++++ csharp/src/Google.Protobuf/UnknownFieldSet.cs | 105 ++++++++- src/google/protobuf/compiler/csharp/csharp_bootstrap_unittest.cc | 4 +- src/google/protobuf/compiler/csharp/csharp_field_base.cc | 2 +- src/google/protobuf/compiler/csharp/csharp_field_base.h | 2 + src/google/protobuf/compiler/csharp/csharp_generator.cc | 2 + src/google/protobuf/compiler/csharp/csharp_map_field.cc | 13 ++ src/google/protobuf/compiler/csharp/csharp_map_field.h | 2 + src/google/protobuf/compiler/csharp/csharp_message.cc | 159 ++++++++++--- src/google/protobuf/compiler/csharp/csharp_message.h | 3 + src/google/protobuf/compiler/csharp/csharp_message_field.cc | 10 + src/google/protobuf/compiler/csharp/csharp_message_field.h | 2 + src/google/protobuf/compiler/csharp/csharp_options.h | 7 +- src/google/protobuf/compiler/csharp/csharp_primitive_field.cc | 10 + src/google/protobuf/compiler/csharp/csharp_primitive_field.h | 2 + src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.cc | 13 ++ src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.h | 2 + src/google/protobuf/compiler/csharp/csharp_repeated_message_field.cc | 13 ++ src/google/protobuf/compiler/csharp/csharp_repeated_message_field.h | 2 + src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.cc | 13 ++ src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.h | 2 + src/google/protobuf/compiler/csharp/csharp_wrapper_field.cc | 32 +++ src/google/protobuf/compiler/csharp/csharp_wrapper_field.h | 4 + 72 files changed, 6986 insertions(+), 821 deletions(-)The doubled generated code seems like a potential concern for code size also. I guess C# probably isn't used much in any kind of mobile (phone) deployment, but probably some people eventually have code size concerns?
A few specific questions:
- Once
Spanis available everywhere, could the old code simply become a client of the new code?
The problem is that Span<> is a ref struct (a struct that is always passed as a reference - a new concept in C#) and as such it can only live on the stack (which also acts safety measure if you're e.g. accessing native memory). So you cannot have regular classes such CodedInputStream hold an instance of Span - that makes it difficult to retrofit the "old" parser to use the CodedInputReader logic internally (currently not possible without degrading the parser's performance).
At some point in the distant future, we could recommend everyone moving to CodedInputReader and CodedOuputWriter, make CodedInputStream and CodedOuputStream "legacy" and make them use the new parsing logic at the expense of worse performance, but still maintaining API compatibility.
- Is it possible that some of the code can be shared by using stateless functions where all input and output is represented as explicit parameters?
We've spent a lot of time investigating this and did as much de-duplication as seemed practical.
- Is it necessary to expose the new reader/writer types to the user? Ideally the user could take advantage of the new code without needing to construct a
CodedInputReaderfor example, but could just directly pass theReadOnlySequence<byte>or whatever that they want to parse from.
The CodedInputReader and CodedOutputWriter types need to be public as they are referenced by the generated code (and they hold the intermediate parsing/serialization state - same idea as CodedInputStream/CodedOuputStream).
Most users won't use these types directly, and would instead call e.g. SomeMessage.Parser.ParseFrom(ReadOnlySequence)
| - fixing bugs (some bug might need to be fixed in two locations) | ||
| - writing tests (in some cases, tests need to be duplicated, because behavior with both Stream/Buffer serialization need to be tested) | ||
|
|
||
| ## Integration with generated gRPC stubs |
There was a problem hiding this comment.
@JunTaoLuo @JamesNK we discussed during our sync-up
There was a problem hiding this comment.
I came up with these possible scenario and would like to double check that my understanding of how things will work is accurate. Can we discuss these and confirm my assumptions are correct?
scenario 1: user imports a pre-compiled pre-generated code from .proto (from before the feature was introduced or generated without --use_buffer_serialization)
- user will be able to use non-buffer serialization/parsing without any noticeable change
- user won't be able to use buffer serialization for the messages contained in the nuget package
- if other .proto depends on messages from the nuget package, user will not be able to regenerate the code with --use_buffer_serialization, because they would end up with duplicate definitions of the types (imagine a.proto imports b.proto and the nuget ships a pre-compiled version of b.proto).
scenario 2: user imports a nuget with pre-generated code that was generated with --use_buffer_serialization but user hasn't enabled --use_buffer_serialization in their own code)
- user will need to upgrade protobuf runtime in their project (the imported nuget needs to depend on protobuf runtime that supports buffer serialization)
- user will be able to use non-buffer serialization/parsing without any noticeable change
- if user regenerates their protos with --use_buffer_serialization, they will be able to use buffer serialization/parsing for all the messages (regardless wheter imported from nuget, or from user's project).
rollback plan: in case things go wrong with the buffer serialization/deserialization feature, we need to have a rollback plan that makes sure that things are backwards compatible (=users that haven't started using the new features won't be impacted at all if we decide to rollback the feature).
- is just rolling back the PR and regenerating code enough?
|
scenario 1:
scenario 2:
|
|
is just rolling back the PR and regenerating code enough?: I think yes. Rolling back will undo the new APIs that are added. There shouldn't be any impact on user's using existing APIs. And this feature for code generation is opt-in. |
To facilitate conversation and to document design decisions, I've created this doc.
Feedback/additions are welcome.
Prototype PR: #5888
Issue: ttps://github.com/grpc/grpc-dotnet/issues/30