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.

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

Use new protobuf serialization/deserialization API in C# marshallers #23485

Merged
merged 2 commits into from
Aug 26, 2020

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Jul 15, 2020

Based on #23855 (protobuf 3.13.0 support the new C# (de)serialization)

Update grpc stub codegen to use the new serialization / deserialization API (see protocolbuffers/protobuf#7576 and protocolbuffers/protobuf#7351)

Inspired by #21653, but this PR is simpler and supersedes it.

@jtattermusch jtattermusch added lang/C# release notes: yes Indicates if PR needs to be in release notes labels Jul 15, 2020
@jtattermusch
Copy link
Contributor Author

@JamesNK @JunTaoLuo feel free to review to changes to the C# generator (the rest of the stuff is bumping protobuf dependency and regenerating code).

@JamesNK
Copy link
Member

JamesNK commented Jul 15, 2020

Exciting.

Can you confirm my understanding of how it will work:

  • There is no longer use_buffer_serialization flag to protoc. Protobuf APIs that use IBufferWriter<byte>/ReadOnlySequence<byte> are always generated in marshaller and are the default
  • If you want to use Grpc.Tools vNext with Google.Protobuf 3.xx (which won't new APIs and will fail to compile) then you need to define GRPC_DISABLE_PROTOBUF_BUFFER_SERIALIZATION in your csproj
  • GOOGLE_PROTOBUF_REFSTRUCT_COMPATIBILITY_MODE is for someone who is using Gprc.Tools vNext + Google.Protobuf 4.0 + a very old .NET Framework version

@jtattermusch
Copy link
Contributor Author

Exciting.

Can you confirm my understanding of how it will work:

  • There is no longer use_buffer_serialization flag to protoc. Protobuf APIs that use IBufferWriter<byte>/ReadOnlySequence<byte> are always generated in marshaller and are the default

Yes, and I like that we don't need a protoc flag because that would add a whole lot of complexity.
But I feel slightly nervous about the missing check in __Helper_DeserializeMessage.

  • If you want to use Grpc.Tools vNext with Google.Protobuf 3.xx (which won't new APIs and will fail to compile) then you need to define GRPC_DISABLE_PROTOBUF_BUFFER_SERIALIZATION in your csproj

Yes, if you're using Grpc.Tools and upgrade to vNext, that will automatically give you a new version of both grpc_csharp_plugin and protoc so code will be regenerate for both messages and the stub. But it's true that you'd have to manually upgrade Google.Protobuf (because Grpc.Tools doesn't have a way of forcing upgrade of the Runtime).

  • GOOGLE_PROTOBUF_REFSTRUCT_COMPATIBILITY_MODE is for someone who is using Gprc.Tools vNext + Google.Protobuf 4.0 + a very old .NET Framework version

Yes, but If you are using GOOGLE_PROTOBUF_REFSTRUCT_COMPATIBILITY_MODE, you'll still need to define GRPC_DISABLE_PROTOBUF_BUFFER_SERIALIZATION (because currently we're not checking message is IBufferMessage in the parsing helper) - this is probably something that we should fix.

I'd still like to benchmark to see what's the impact on performance (we can now avoid some copies and allocations, which should be very noticeable especially for large messages).

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Looks good to me. When it is checked in I can use the preview package from the nightly feed to measure results.

src/compiler/csharp_generator.cc Outdated Show resolved Hide resolved
@jtattermusch
Copy link
Contributor Author

I started an adhoc package build: sponge/2438292f-625d-4905-87fd-6ca36d137e8c (I'll post the public link to nugets later).

@jtattermusch
Copy link
Contributor Author

sponge/2438292f-625d-4905-87fd-6ca36d137e8c

The link to download adhoc-built nugets:
https://storage.googleapis.com/grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/windows/grpc_build_packages/13038/20200720-062833/github/grpc/artifacts/csharp_nugets_windows_dotnetcli.zip

@jtattermusch
Copy link
Contributor Author

I created a PR for upgrade to 3.13.0 #23855.

@jtattermusch
Copy link
Contributor Author

This is basically blocked by #23855.

@jtattermusch
Copy link
Contributor Author

@apolcyn this is now ready for final review. Note that most commits are coming from #23855 (I can rebase once the PR is merged).

@jtattermusch
Copy link
Contributor Author

#23855 has been merged, so I rebased. The tests were passing before so this should work well.

@jtattermusch
Copy link
Contributor Author

CC @markdroth once a get an LGTM from @apolcyn this PR is good to merge.

@JamesNK
Copy link
Member

JamesNK commented Aug 25, 2020

sponge/2438292f-625d-4905-87fd-6ca36d137e8c

The link to download adhoc-built nugets:
storage.googleapis.com/grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/windows/grpc_build_packages/13038/20200720-062833/github/grpc/artifacts/csharp_nugets_windows_dotnetcli.zip

I tested this build with grpc-dotnet and the test app run successfully.

Once this PR is merged I'll update grpc-dotnet to use the latest nightly build of Grpc.Tools, and run all unit tests with it.

@jskeet
Copy link
Contributor

jskeet commented Sep 16, 2020

Yes, but If you are using GOOGLE_PROTOBUF_REFSTRUCT_COMPATIBILITY_MODE, you'll still need to define GRPC_DISABLE_PROTOBUF_BUFFER_SERIALIZATION (because currently we're not checking message is IBufferMessage in the parsing helper) - this is probably something that we should fix.

Does that explain googleapis/gax-dotnet#420? Currently I don't have information about what's broken, but I'm expecting it's something like:

  • Generate message X with protoc 3.13, depending on common protos
  • Generate gRPC stubs with gRPC 2.32
  • Serialization fails because the common protos don't have the expected methods

Once I've got more information, I'll probably try to reproduce it and file a new issue - I think it's a bit of a problem if all dependencies have to be regenerated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/C# priority/P0/RELEASE BLOCKER release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants