[C#] Support Span<T> instead of / on top of T[] for array getters/setters

[damageboy]

Hi,
I'm opening this issue to start a minimal discussion before I go ahead and attempt to implement a change to the code generation that sbe does for C#.

C# has support for Span<T> which can be used to encapsulate various types of array of T where those can be:

• Unmanaged memory
• stack allocated memory
• Existing managed / GC memory

If we take the generated code for Car from: https://github.com/real-logic/simple-binary-encoding/blob/master/csharp/.nuget/examples/baseline/Car.cs

Here's a short relevant excerpt:

https://github.com/real-logic/simple-binary-encoding/blob/8ec67b1c28ce70b845ad43839f309dc7bf7b5473/csharp/.nuget/examples/baseline/Car.cs#L964-L986

We can clearly see that SBE currently generates Get/Set method pairs that can take as input/output a managed array of the underlying type (byte in case of a string).

I would like to either replace or add additional Get/Set methods that accept a Span<byte> in this case, or a Span<T> in the general case.

Adding such Getters/Setters would allow users to copy strings / arrays out of the message into stack allocated memory, for example, without requireing to have a managed buffer as it currently required, for example in the example usage for the same sample:
https://github.com/real-logic/simple-binary-encoding/blob/8ec67b1c28ce70b845ad43839f309dc7bf7b5473/csharp/.nuget/examples/CarExample.cs#L184

https://github.com/real-logic/simple-binary-encoding/blob/8ec67b1c28ce70b845ad43839f309dc7bf7b5473/csharp/.nuget/examples/CarExample.cs#L259

I think this could be a nice first step in improving SBE's friendliness for high-perf / zero allocation users.

As a second step I ALSO think it would be great if additional GetXXX() methods would be introduced returning a Span<T> pointing to the internal memory, but I would first want to get this done before opening a separate issue on that matter and implementing that.

Any thoughts?

[billsegall]

All contributions are welcome. I have no problem with additional API. Purging the old is problematic for compatibility. It would be good to have test cases and also the examples updated so that in time it might be possible to deprecate any old api if we choose to. Bill.

…

On Thu, Sep 27, 2018 at 8:19 PM damageboy ***@***.***> wrote: Hi, I'm opening this issue to start a minimal discussion before I go ahead and attempt to implement a change to the code generation that sbe does for C#. C# has support for Span<T> <https://docs.microsoft.com/en-us/dotnet/api/system.span-1?view=netcore-2.1> which can be used to encapsulate various types of array of T where those can be: - Unmanaged memory - stack allocated memory - Existing managed / GC memory If we take the generated code for Car from: https://github.com/real-logic/simple-binary-encoding/blob/master/csharp/.nuget/examples/baseline/Car.cs Here's a short relevant excerpt: https://github.com/real-logic/simple-binary-encoding/blob/8ec67b1c28ce70b845ad43839f309dc7bf7b5473/csharp/.nuget/examples/baseline/Car.cs#L964-L986 We can clearly see that SBE currently generates Get/Set pair that can take as input/output a managed array of the underlying type (byte in case of a string). I would like to either replace or add additional Get/Set methods that accept a Span<byte> in this case, or a Span<T>. Adding such Getters/Setters would allow users to copy strings / arrays out of the message into stack allocated memory, for example, without requireing to have a managed buffer as it currently required, for example in the example usage for the same sample: https://github.com/real-logic/simple-binary-encoding/blob/8ec67b1c28ce70b845ad43839f309dc7bf7b5473/csharp/.nuget/examples/CarExample.cs#L184 https://github.com/real-logic/simple-binary-encoding/blob/8ec67b1c28ce70b845ad43839f309dc7bf7b5473/csharp/.nuget/examples/CarExample.cs#L259 I think this could be a nice first step in improving SBE's friendliness for high-perf / zero allocation users. As a second step I ALSO think it would be great if additional GetXXX() methods would be introduced returning a Span<T> pointing to the internal memory, but I would first want to get this done before opening a separate issue on that matter and implementing that. Any thoughts? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#587>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFI1pO7JXj9kpuz_V89_hbHa1VDEj3cVks5ufKZDgaJpZM4W8V7m> .

[damageboy]

OK, Sounds good.

I'll start working on this.

I might end up doing the "full" Span<T> support by adding Getters that return a Span<T> directly on top of the DirectBuffer owned memory.

One question though about that: When returning a Span<T> from *Encoder classes, the Span obviously needs to be R/W.

However, when returning one from the *Decoder classes, I could choose to return a ReadOnlySpan<T> and make sure the receiver of this Span can never modify the memory behind the "array" in the decoded message.

It's not clear to me that this is ever correct from the user's point of view (i.e. limiting the user to RO only).

Any thoughts about this?

[mjpt777]

In the Java codecs the Encoders only write, and the Decoders only read.

[damageboy]

@mjpt777 Understood, the question still stands: Should a "view" of the Decoder owned memory be limited in such a way the user cannot write to the decoded memory?

I can see this either way: On the one hand it is a good idea to never write into the decoded memory, therefore ReadOnlySpan<T> makes sense for that.

On the other hand, the user might want to mutate the buffer as part of decoding/parsing for whatever reasons I cannot imagine right now. If they retain the original pointer / managed array pointing to the data they could do so in theory through that pointer, therefore returning a ReadOnlySpan<T> would be limiting the user for no good reason.

Any more specific guidance?

[mjpt777]

For type safety it is best if the decoder is read only. This way it can be safely passed out without worry of modification.

[damageboy]

@mjpt777 Thanks for the comments. Will do.

[damageboy]

One question as I'm working on this...

The new generator now violates styling rules:

[ant:checkstyle] [ERROR] /home/damageboy/projects/public/sbe.git/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java:331:5: Method length is 102 lines (max allowed is 100). [MethodLength]
[ant:checkstyle] [ERROR] /home/damageboy/projects/public/sbe.git/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java:776:5: Method length is 137 lines (max allowed is 100). [MethodLength]

I assume these need to be taken seriously and broken into additional functions?

Just making sure...

[mjpt777]

For the generators we can relax a bit. If the methods can be decomposed then better.

[odeheurles]

This would be a nice addition but needs to be managed carefully. Span was recently introduced in .NET, even with new methods I think it will break compatibility for users who need/want to stick with an older runtime and/or compiler (C# compiler 7.2 required based on my quick search). If the changes only touch the generated code maybe a command line switch could be introduced to enable/disable generation of span related methods. If you need to change the runtime part of SBE (the SBE dll) then this will be a problem: everyone would have to upgrade to .NET Core 2.1, which wouldn’t be acceptable for many users.

…

On 27 Sep 2018, at 16:57, Martin Thompson ***@***.***> wrote: For the generators we can relax a bit. If the methods can be decomposed then better. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[damageboy]

@odeheurles Thanks for bringing this up: I've given it some thoughts, here they are, let me know what you think:

From the little that I see, it is enough to introduce a dependency on System.Memory if all I want it to accept / return Span<T>.

System.Memory has a .NET 4.5 compatible implementation which uses inline IL (I think) to implement Spans without ref T support that was indeed introduced as part of C# 7.2.

That is to say, while Span<T> is implemented "in C#" for later versions of the frameworks and compilers, there is still a version of Span<T> that was implemented by other means for older frameworks.

From my (admittedly short) research, There is no REQUIREMENT for C# 7.2 for usage of Span<T>, there are only additional features related to Span<T>, such as stackallocing into a Span<T> as part of C# 7.2.

There could have been other possible issue(s) with the usage of Span had the generated required BCL support of Span<T> (for example calling Stream.Write that accepts Span<T> that was introduced in later versions of .NET Core, but this is irrelevant for SBE currently.

As such, I thought it would be still OK to add a System.Memory dependency to sbe-dll, and also impose users to add another dependency on System.Memory if they actually want to create Span<T> in their own code.

Users would only have to use a newer compiler in the case they want to generate Spans in less orthodox ways, such as the stackalloc example I gave before.

Given this, I thought, to myself, that simply outputting the Span<T> accepting/returning code by default is an acceptable way to go forward even for older versions of the framework.

As for the dependency on System.Memory: generally speaking, SBE generated code does not generate the .csproj files, therefore I have no place to output the <PackageReference> or anything equivalent to it for older projects, so this would have to be exposed in the documentation of C# support in SBE and left for the user to do when setting up the project around the generated code...

Sounds good?

[billsegall]

I'll defer to Olivier on this. Bill.

…

On Fri, Sep 28, 2018 at 6:38 PM damageboy ***@***.***> wrote: I've given it some thought, Here they are, let me know if you know anything that contradicts this: From the little that I see, it is enough to introduce a dependency on System.Memory <https://www.nuget.org/packages/System.Memory/> if all I want it to accept / return Span<T> System.Memory has a .NET 4.5 compatible implementation which uses inline IL (I think) to implement Spans without ref T support that was indeed introduced as part of C# 7.2. That is to say, while Span<T> is implemented "in C#" for later versions of the frameworks and compilers, there is still a version of Span<T> that was implemented by other means for older frameworks. From my (admittedly short) research, There is no REQUIREMENT for C# 7.2 for usage of Span<T>, there are only additional features related to Span<T>, such as stackallocing into a Span<T> as part of C# 7.2. There is another possible issue with the usage of Span *had* the generated require BCL support of Span<T> (for example calling Stream.Write that was introduced in late versions of .NET Core, but this is irrelevant for this piece of generated code. As such, I thought it would be still OK to add a System.Memory dependency to sbe-dll, and also impose users to add another dependency on System.Memory if they actually want to create Span<T> in their own code. Users would only have to use a newer compiler in the case they want to generate Spans in less orthdox ways, such as the stackalloc example I gave before. Given this, I thought, to myself, that simply outputing the Span<T> accepting/returning code by default is an acceptable way to go forward even for older versions of the framework. Generally speaking, SBE generated code does not generate the .csproj files, therefore I have no place to output the <PackageReference> or anything equivalent to it for older projects, so this would have to be exposed in the documentation of C# support in SBE. Sounds good? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#587 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFI1pJ6hbk3fOZsDVsMmOede6quKG73Gks5ufeAUgaJpZM4W8V7m> .

[roji]

To reinforce (and possibly simplify) what @damageboy wrote above, introducing Span<T> into code doesn't, by itself, break compatibility or impose anything beyond the added dependency on the System.Memory nuget (assuming you're not looking to support .NET Framework versions older than 4.5). Various features around Span<T> may not be available, though - optimized JIT intrinsics support was introduced in .NET Core 2.1 (so Span<T> will run slower on other platforms), and stackalloc syntax support was introduced only in C# 7.2.

[damageboy]

Hi, I have a basic version working and I can submit a PR,
Do you want me to wait on this until we hear from @odeheurles or should I submit the PR and we will continue it over there?

One note, it appears most of the documentation is in from of GH wiki pages that I cannot edit as part of PR if I understand correctly, would you like me to first finish the PR and update the docs later on this project directly (I have little experience or practices of wiki editing on GH...)

[mjpt777]

@damageboy docs can be updated after the PR.

[odeheurles]

Sounds good to me, you can submit the PR Thanks Olivier

…

On 30 Sep 2018, at 16:31, Martin Thompson ***@***.***> wrote: @damageboy docs can be updated after the PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.