Support missing for contextual deserializer/serializer

[mgravell]

Right now, the code directly hooks into the legacy .Serializer / .Deserializer, which provides access to the Func<T, byte[]> etc, however IMO this violates the API.

The primary API is now the contextual serializer, by which I mean:

• Grpc.Core[.Api] always uses the contextual serializer (except for some tests)
• the contextual serializer always works (it is emulated when initialized from the legacy API), where-as the legacy serializer does not; see Marshaller.cs 44-47

// gRPC only uses contextual serializer/deserializer internally, so emulating the legacy
// (de)serializer is not necessary.
this.serializer = (msg) => { throw new NotImplementedException(); };
this.deserializer = (payload) => { throw new NotImplementedException(); };

This has two significant problems:

1. any custom framework that tries to use the contextual serializer will not work from grpc-dotnet
2. any improvements made to the core implementation of Marshaller will not be automatically exposed

For context on 2, see this PR, which does an in-place upgrade on how regular protoc-style protobuf deserializes, using the contextual API to significantly reduce buffer allocations. If grpc-dotnet does not use the contextual API: it will not benefit if/when those changes get merged.

I am aware of #30, but IMO there are two different things:

• Serialization API - efficiency #30 is about making better use of these APIs in the future
• this report is about a fundamental bug that breaks the API today

You can also probably infer from this that I'm keen on using the contextual API, and generally improving this space - presumably looking into a new writer API too.

So, I guess my main point here is: grpc-dotnet should not use the legacy API - it should switch to the contextual API. I can offer some help with that if it would be welcome.

---

As a caveat / disclosure thing: I will acknowledge the remark:

/// Note: experimental API that can change or be removed without any prior notice.

but... if the API surface changes, consumers need to update anyway. I don't personally see this as a reason not to consume this API.

[mgravell]

I've added a PR to the core that (if accepted) will fix "1" above; to fix "2" requires taking the experimental API

[JamesNK]

the contextual serializer always works (it is emulated when initialized from the legacy API), where-as the legacy serializer does not; see Marshaller.cs 44-47

Is there overhead in using the emulation?

[mgravell]

no, basically; it shapes the API a little differently, mostly - but the emulation layer hides that; the writer API hasn't really been touched yet; the reader API only has 2 methods:

• give me the payload as a byte[]
• give me the payload as a ROS

In either case, the emulated layer just stashes the byte[] that you already provide, and re-exposes it directly (zero copy etc) as either the byte[] or ROS; no allocations etc. So, with the PR as written - again: "no"

For context: the Google implementation almost always uses emulation in the other direction (i.e. protobuf subscribes to the legacy API, the implementation consumes the emulated contextual API); as a side effect of this, if the PR above gets accepted into the core, that'll mean that if you leave your code alone, regular protobuf will be using the direct non-emulated route, and only my custom bindings would be using the emulated API.

[mgravell]

Based in feedback on the PR, the PR seems unlikely to be merged

[jtattermusch]

grpc-dotnet should switch to using Marshaller.ContextualSerializer and Marshaller.ContextualDeserializer internally instead of Marshaller.Serializer/Deserializer (there's a few occurrences in *CallHandler classes and in the managed client).

• this will allow using both simple and contextual serializers/deserializers (contextual API is required if one wants to benefit from more efficient ROS based deserilialization, so it's the right choice for the future)
• conversion from simple serializer (currently part of generated code of grpc_csharp_plugin) happens with basically no overhead (e.g. see here https://github.com/grpc/grpc/blob/3914de0b49925116ad15007d1feddbb6690ea14b/src/csharp/Grpc.Core.Api/Marshaller.cs#L91).
• the only overhead is creating an instance of Serialization/DeserializationContext to pass to the serializer/deserializer functions. The object can be cached thread-locally if creating a new instance on each invocation is a concern
  https://github.com/grpc/grpc/blob/3914de0b49925116ad15007d1feddbb6690ea14b/src/csharp/Grpc.Core/Internal/DefaultDeserializationContext.cs#L31

[JamesNK]

Ok! I think caching will probably be at a higher level (the entire call context) but that will be in the future. An extra allocation per message isn't the end of the world.

[mgravell]

You and me have very different views on allocations :)

Right now I'm on a treasure-hunt for avoidable allocations in the Grpc.Core/Grpc.Core.Api path (the latter of which you should benefit from indirectly); I would do the same for Grpc-Dotnet, but ... it is kinda awkward to work with right now unless I keep separate machines/VMs for preview6 and preview7 - and even then, it isn't the easiest build since the dependency-tree is fast-moving.

But in all seriousness; if you can avoid an alloc per call (without bending the code too far out of shape), IMO you should avoid that alloc per call; they add up surprisingly quickly, and can quickly swamp the necessary allocations (i.e. the actual data payloads); on a high-throughput server allocations and GC can quickly dominate, or at least make a server more expensive than it needs to be (which can mean in terms of overhead on a box, or actual hosting costs for cloud/hosted services), even if performance is still acceptable.

[JamesNK]

I have plans to remove almost all non-message allocations on the server. I don't want to micro-optimize for the serialization context if code will be thrown away post 3.0.

[JamesNK]

I created an issue for reusing the ServerCallContext. If you're interested in improving performance then you're welcome to take a look 😋

#369

https://twitter.com/JamesNK/status/1107374097846034432