C# - support byte[]-segment deserializers **without** changing the calling API

[mgravell]

Incomplete: still working on getting the tests working locally; dropping this here to show direction

First of several steps intended to reduce allocations when deserializing and serializing.

Scope: this set of changes focuses on reducing allocations during deserialization over all supported platforms using the existing generated code, i.e. no protoc changes. This is achieved by:

• adding a new LeasedBuffer concept
• adding a new API to the deserialization context that allows access to the payload as a leased buffer
• implement this by leasing an array from the shared pool, linearizing the data into this buffer, and then (when complete) returning the buffer to the pool
• exploiting the pattern that protoc emits, i.e. when protoc uses the marshaller constructor that would use the Func<byte[], T>, automatically detect and switch over to a Func<byte[],int,int,T>, making use of the new leased buffer if a match is found

This still involves a copy, so isn't perfect, but it is far less allocatey

Future ideas:

• deserialization: make available as ReadOnlyMemory<byte> (zero copy) when it is a single range (this is a pretty common case, and easy to optimize for); presumably TryGetMemory or similar?
• deserialization: make available as Stream backed by the underlying segment; stream API still involves a copy out, but it should be better than double-copying etc, and many deserializers support Stream
• deserialization: need to understand why gRPC chooses not to use ReadOnlySequence<byte> in more platforms (it should work, etc)
• serialization (haven't started thinking about yet)

[jtattermusch]

I've experimented with similar concept in the past ( PayloadAsRentedBuffer() ) which was using ArrayPool.Shared.Rent internally, but I decided not to add that method as I wasn't sure it was providing much extra value.
Once you're on netstandard2.0 (= GRPC_CSHARP_SUPPORT_SYSTEM_MEMORY is true), the can provide a deserializer that can easily perform pooling (custom or using ArrayPool) and read the content from PayloadAsReadOnlySequence(). This doesn't help .NET desktop users, but my assumption was is that most users that would care about performance would already be using .NET core.

[mgravell]

I don't think that final assumption is valid: many folks care deeply but are still attempting gradual migration of large code-bases. But it may be moot - see other comment

[jtattermusch]

Btw, any reason for not using IMemoryOwner instead of defining a custom type?

[mgravell]

IMemoryOwner doesn't expose an array, but: it is probably not important as I think we can lose this API completely given the context I now understand about ROS-byte

[jtattermusch]

• deserialization: need to understand why gRPC chooses not to use ReadOnlySequence<byte> in more platforms (it should work, etc)

the problem is that for Span<> you need C# 7.2 and we do have users that only support C# 6, so for now we stayed safe and only opted for the System.Memory dependency and Span<> when on a platform that definitely has support for C# 7.2 (nestandard2.0 in our case)

[jtattermusch]

+1 to the idea of trying to make use the foo.Bar(byte[], int, int) parser without requiring any codegen changes,
I think that is something that's worth investigating. But we also need to give this a bit more thought so that it doesn't have any unwanted consequences (e.g. protobuf is not the only serialization format supported and we don't know what this code would do when other serialization formats are being used).

[mgravell]

We could perhaps further restrict it to where the first parameter matches name and the second parameters are named offset and count? I think that's pretty unambiguous about the intent...

[mgravell]

• deserialization: need to understand why gRPC chooses not to use ReadOnlySequence<byte> in more platforms (it should work, etc)

the problem is that for Span<> you need C# 7.2 and we do have users that only support C# 6, so for now we stayed safe and only opted for the System.Memory dependency and Span<> when on a platform that definitely has support for C# 7.2 (nestandard2.0 in our case)

That would be a legitimate reason for not emitting code to consume this API in protoc (without an opt-in), but it doesn't really impact the library if there's an API that some consumers can't use fully (they can actually still use a lot of it, though). In particular, the framework and language are completely separate and independent: you can be targeting either from either, just about.

So: if that's the history, can I suggest an easier solution here that would allow me to remove almost all of these changes?

1. make the read-only-sequence API available on all target platforms
2. use that API along with try/finally in the "try to use a byte[]/int/int" code to use a shared buffer
3. and... that's it!

Thoughts?

[mgravell]

Oof, except maybe xamarin. Dammit xamarin, y u so hard! But: if xamarin targets netstandard1.*, Spans could still be enabled on .net framework.

Edit: wait, I might be thinking of unity. Xamarin might be fine after all! One of them is .. awkward

[jtattermusch]

Oof, except maybe xamarin. Dammit xamarin, y u so hard! But: if xamarin targets netstandard1.*, Spans could still be enabled on .net framework.

Edit: wait, I might be thinking of unity. Xamarin might be fine after all! One of them is .. awkward

Unity is the one that's awkward. They are only providing netstandard2.0 and C# 7.2 support since very recently (the current "package" for unity targets net45)) and there's issues with things like reflection.

[mgravell]

@jtattermusch so I'm guessing that unity is using the netstandard1.5 build? in which case, would you be open to extending GRPC_CSHARP_SUPPORT_SYSTEM_MEMORY to net45 ? this would allow all .NET Framework users to use the new APIs.

Going back to your earlier point about folks who are interested in perf: I do entirely agree that those folks aren't targeting netstandard1.5; they might still be stuck on .NET Framework, though. And: a .NET Framework 4.7.2 application targeting a lib that exposes <TargetFrameworks>net45;netstandard1.5;netstandard2.0</TargetFrameworks> will be using the net45 build.

I can be sold on the idea that trying to expose it in netstandard1.5 helps almost nobody, so if not doing that helps with unity etc, then; fine!

[mgravell]

@jtattermusch I've pushed the simplified version of the code, which assumes we can extend GRPC_CSHARP_SUPPORT_SYSTEM_MEMORY into net45; much cleaner, IMO; I also made the reflection code more picky, i.e. check the parameter names. Now all I need to do is get grpc_csharp_ext.dll to build, so I can check it works :)

edit: I got the MarshallerTest tests working; my local build isn't loving the grpc_csharp_ext.x64.dll that I've managed to generate, though, so the actual native tests... not so happy. Unrelated to this, though :/

[jtattermusch]

@jtattermusch so I'm guessing that unity is using the netstandard1.5 build? in which case, would you be open to extending GRPC_CSHARP_SUPPORT_SYSTEM_MEMORY to net45 ? this would allow all .NET Framework users to use the new APIs.

Actually right now Unity uses the net45 assembly:

grpc/src/csharp/build_unitypackage.bat

Line 46 in da3784b

copy /Y Grpc.Core\bin\Release\net45\Grpc.Core.dll unitypackage\unitypackage_skeleton\Plugins\Grpc.Core\lib\net45\Grpc.Core.dll || goto :error

We might be able to switch unity over to netstandard2.0, but I haven't tried that yet and it would also mean that users would only be able use Unity versions starting from 2018.3 https://docs.unity3d.com/2018.3/Documentation/Manual/dotnetProfileSupport.html

Going back to your earlier point about folks who are interested in perf: I do entirely agree that those folks aren't targeting netstandard1.5; they might still be stuck on .NET Framework, though. And: a .NET Framework 4.7.2 application targeting a lib that exposes <TargetFrameworks>net45;netstandard1.5;netstandard2.0</TargetFrameworks> will be using the net45 build.

I can be sold on the idea that trying to expose it in netstandard1.5 helps almost nobody, so if not doing that helps with unity etc, then; fine!

[jtattermusch]

The problem with supporting Span<> based APIs in net45 profile is that users might be on a system that doesn't support C# 7.2 and even thoug framework and compiler used are two separated things, on C# 6 systems the ref struct types (e.g. Span) will be marked as "obsolete" and prevent compilation as soon as they are referenced. So far I haven't figured our a reliable way to test that we accidentally don't break these users (e.g. if Span is exposed as a method argument for a method exposed on DeserializationContext, does that only prevent the user from referencing that method or can it prevent the user from using the class entirely in some situations? The latter would be a problem).

Also, if Span is used internally in a library build using C# 7.2 (Grpc.Core in our case), can that codepath be safely executed on an old system (that has no knowledge of C# 7+ or ref structs)?
Unity further complicates this because it needs to do transpiling of the C# code to c++ on some platforms and I'm not sure if that would work if ref structs are used.

If we address these questions sufficiently, I'll be more than happy to add the Span<> based optimizations to the net45 build too.

[mgravell]

@jtattermusch I understand the details of how Span<T> is hacked in, but : that doesn't actually make it an unusable API. To illustrate this: I've used it in this PR, without a single usage of Span<T> , to achieve efficient use of pooled buffers. ROS can be useful even without Span<T>!

I did not propose any API changes that added spans etc onto DeserializationContext, but my understanding is that if someone is using a down-level C# version, they indeed won't be able to use Span<T> directly - but that should only impact the specific APIs that actively involve span. If you're explicitly worried about that, I can take on the leg-work of formally confirming that.

As for old systems: Span<T> didn't require any runtime changes; there is no new IL concept for this. The actual feature being used here has existed in the runtime for ages, but was never exposed in C# before Span<T>. There are rules about where that feature is allowed, which the older compiler doesn't know because it was never in C# before, which is why it was marked [Obsolete] (to prevent old compilers from allowing it to be used in invalid contexts, through the pragmatic step of preventing the old compilers from allowing it in any contexts). But the runtime is absolutely fine with it; code that involves Span<T> will run just fine on old runtimes.

The transpiling question is a good one. It would be good to understand what targets we're worried about there, so that it can be investigated.

[jtattermusch]

It might make sense to split this PR into two independent parts:

• extending GRPC_CSHARP_SUPPORT_SYSTEM_MEMORY support to net45
• avoiding allocations during deserialization by looking for array-segment for parse overload and feeding the buffer from ROS (which is doable whenever GRPC_CSHARP_SUPPORT_SYSTEM_MEMORY is enable).

[mgravell]

It might make sense to split this PR into two independent parts:

sure, I can do that

[jtattermusch]

nit: Is it really worth adding #if NET45 (which is not great for readability) as opposed to just using a static member always?

[mgravell]

fixed next push

[jtattermusch]

nit: in the Grpc codebase we are currently not using the s_ prefix for static readonly members (we use capital).

[mgravell]

fixed next push

[jtattermusch]

nit: System.Memory brings System.Buffers transitively, it it worth adding it explicitly?

[mgravell]

that was a hangover from when I wasn't using System.Memory for this; no longer needed, removing

[mgravell]

@jtattermusch nits and csproj-split: done

[erikmav]

Light ping on this PR - for large buffer deserialization I'd find the latest version of this very useful to have in the API to help avoid copies - possible to complete this?

[stale]

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

[jtattermusch]

Looks like this is no longer relevant now that #23485 and corresponding Protobuf improvements
protocolbuffers/protobuf#7576 and protocolbuffers/protobuf#7351 are in place?

[mgravell]

Agreed, thanks

…

On Mon, 19 Oct 2020, 10:04 Jan Tattermusch, ***@***.***> wrote: Looks like this is no longer relevant now that #23485 <#23485> and corresponding Protobuf improvements protocolbuffers/protobuf#7576 <protocolbuffers/protobuf#7576> and protocolbuffers/protobuf#7351 <protocolbuffers/protobuf#7351> are in place? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19460 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMH2HEXMQKJHT6KTN53SLP6JZANCNFSM4H3GNZXA> .