In/outgoing Span<byte> support, linux net45 build #589

damageboy · 2018-10-01T13:14:03Z

Closes [C#] Support Span<T> instead of / on top of T[] for array getters/setters #587
Add netfx.props and tweak sbxee-dll csproj to allow building sbe.dll
for net45 on Linux using dotnet sdk following:
https://andrewlock.net/building-net-framework-asp-net-core-apps-on-linux-using-mono-and-the-net-cli/
Remove packages.config files from projects already converted to SDK style project since
those files serve no purpose other than confusing potential contributors...
Intoroduce minimal Span GetBytes/SetBytes implementation for copying
to/from Span to sbe.dll itself.
Change code generation for array types to use Span internally keeping the
original functionality intact with no (hopefully!) visible user facing changes except
for the addtitional Get/Set methods accepting Span being added to the mix.

damageboy · 2018-10-01T13:29:34Z

It's worthwhile to mention it might appear that I've removed a couple of input validation checks in the generated code, but this is not really so. Those checks happen anyway inside the constructor of Span<T> and will throw essentially the same exception from inside the Span<T> ctor.

damageboy · 2018-10-07T07:28:31Z

Any comments about this?

billsegall · 2018-10-07T09:18:25Z

My only question would be whether there was test cases for those exceptions? Bill

…

On Sun, 7 Oct 2018, 5:28 PM damageboy ***@***.***> wrote: Any comments about this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFI1pEyacdIkHwWpnrdIBQEwGWwajJlCks5uia0jgaJpZM4XCCgM> .

damageboy · 2018-10-07T12:26:24Z

I'll try figuring that out, I admin I worked under the assumption that tests run as part of gradle, I'm suddenly not so sure

ZachBray · 2018-10-08T09:54:38Z

Thanks for the PR. Looks good to me, in general. :-)

My understanding is that only the code generation happens inside the Gradle script not the building/testing of the C# solution.

I have a few questions. I'm not sure who is best placed to answer them (maybe @billsegall ?).

Could the introduction of the System.Memory dependency affect anyone? I.e., could users have been using .NET Framework < 4.5 to build the SBE generated code prior to this change? I guess the lower limit is currently whatever the sbe-tool package targets. The NuGet site doesn't make this obvious for this particular package.
If users are affected, are we still happy to make the change? Or should we introduce a switch into the code generation that enables Span support? I think Olivier suggested this in [C#] Support Span<T> instead of / on top of T[] for array getters/setters #587.
Do we use any features in netstandard2.0 that aren't in netstandard1.1? I ask as it looks like netstandard1.1 is supported by the System.Memory package and according to the .NET Standard compatibility matrix this is supported by .NET Framework v4.5. Would this mean we could simplify the build somewhat?

ZachBray · 2018-10-08T09:55:01Z

Also...

Do we keep release notes somewhere?

billsegall · 2018-10-08T11:38:35Z

We're currently targeting the nuget packages at netstandard2.0 and have dropped 1.1. No-one complained but that doesn't mean they won't. In general I'm happy to maintain a reasonable amount of backward compatibility but MS made this very difficult pre 2.0. Similarly 4.5 would seem to me to be a reasonable minimum at this point but I'd be happy to address this if anyone really needed it. In the absence of a demonstrated need I'd leave things at that, Bill. Bill.

…

On Mon, Oct 8, 2018 at 7:54 PM Zach Bray ***@***.***> wrote: Thanks for the PR. Looks good to me, in general. :-) My understanding is that only the code generation happens inside the Gradle script not the building/testing of the C# solution. I have a few questions. I'm not sure who is best placed to answer them (maybe @billsegall <https://github.com/billsegall> ?). 1. Could the introduction of the System.Memory dependency affect anyone? I.e., could users have been using .NET Framework < 4.5 to build the SBE generated code prior to this change? I guess the lower limit is currently whatever the sbe-tool package <https://www.nuget.org/packages/sbe-tool/0.1.8.1-beta-1> targets. The NuGet site doesn't make this obvious for this particular package. 2. If users are affected, are we still happy to make the change? Or should we introduce a switch into the code generation that enables Span support? I think Olivier suggested this in #587 <#587>. 3. Do we use any features in netstandard2.0 that aren't in netstandard1.1? I ask as it looks like netstandard1.1 is supported by the System.Memory package <https://www.nuget.org/packages/System.Memory/> and according to the .NET Standard compatibility matrix <https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support> this is supported by .NET Framework v4.5. Would this mean we could simplify the build somewhat? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFI1pL_l0ybxFqKV_0vR3czwF-Ee9fmvks5uiyDogaJpZM4XCCgM> .

billsegall · 2018-10-08T11:40:10Z

Release notes are a good idea. How do people feel about an additional section in ~csharp/README.md? Bill.

…

On Mon, Oct 8, 2018 at 7:55 PM Zach Bray ***@***.***> wrote: Also... 1. Do we keep release notes somewhere? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFI1pBTjbjtzQbYLbvG3tkN9eE-t4GhOks5uiyEBgaJpZM4XCCgM> .

damageboy · 2018-10-08T15:50:09Z

@billsegall @ZachBray
I was thinking about (per @mjpt777) to update the relevant wiki pages re. the new dependency post merge.

I didn't think about the README.md inside the csharp folder, and see no reason not to amend this commit to reflect that dependency as well..

As for .NET 4.5 as minimum I think its a fair limitation given that official support is practically non existent for anything below 4.5.2 on the one hand, and .NET Framework 4.5 was released on 15 August 2012 on the other hand.

This is especially true for a performance / latency oriented project like this where you practically get free perf boost just for moving to .NET Core.

In other words, to me, it seems reasonable to say optimal perf is on .NET Core, compat for whatever reason you might need it goes back to 2012.

mjpt777 · 2018-10-08T17:27:52Z

The bulk of the notes can be in the csharp/README.md which can be referenced from the wiki.

ZachBray · 2018-10-08T22:48:17Z

@billsegall

I'm happy to maintain a reasonable amount of backward compatibility but MS
made this very difficult pre 2.0

I didn't realise that. Fair enough! :-)

We're currently targeting the nuget packages at netstandard2.0

Assuming the consumers are not building from source (is this a fair assumption?), this means we support .NET Framework >= v4.6.1 according to the .NET Standard compatibility table. In which case, please can we drop the multiple targets from here?

https://github.com/real-logic/simple-binary-encoding/blob/master/csharp/sbe-dll/sbe-dll.csproj#L4

I.e., only target netstandard2.0 for the class library. I believe this will make it easier to build on unix without mono if we switch the tests to use dotnet core. We could then get the CSharp tests running as part of the Travis build matrix.

ZachBray · 2018-10-08T22:48:51Z

@damageboy apologies for polluting your PR with these comments! They probably belong somewhere else.

damageboy · 2018-10-09T09:56:43Z

@ZachBray As I'm now knee deep in figuring out how the unit tests work, it's actually good timing I think.

To be quite honest, I think I should simply run the tests as is manually on windows and give a scout's honor promise the tests are green.

Other than that, I think the tests require their own major overhaul in a separate PR,
I've opened a different issue for this: #592

- Closes aeron-io#587 - Add netfx.props and tweak sbxee-dll csproj to allow building sbe.dll for net45 on Linux using dotnet sdk following: https://andrewlock.net/building-net-framework-asp-net-core-apps-on-linux-using-mono-and-the-net-cli/ - Remove packages.config files from projects already converted to SDK style project since those files serve no purpose other than confusing potential contributors... - Intoroduce minimal Span<byte> GetBytes/SetBytes implementation for copying to/from Span<byte> to sbe.dll itself. - Change code generation for array types to use Span<byte> internally keeping the original functionality intact with no (hopefully!) visible user facing changes except for the addtitional Get/Set methods accepting Span<byte> being added to the mix. - Add release notes section to README.md and some more touch ups - Add sbe-tests.csproj netcoreapp2.1 test target

mjpt777 · 2018-11-09T21:10:20Z

Can @JPWatson, @odeheurles, or @billsegall let me know if this should be merged or not?

billsegall · 2018-11-11T04:51:55Z

I've completed a manual build/test with the patch and it's working for me on Windows building with VS2017. I'll probably freshen the nuget package following the release of 1.9.1. I didn't add any validation tests using the the new code. Bill.

…

On Sat, Nov 10, 2018 at 7:10 AM Martin Thompson ***@***.***> wrote: Can @JPWatson <https://github.com/JPWatson>, @odeheurles <https://github.com/odeheurles>, or @billsegall <https://github.com/billsegall> let me know if this should be merged or not? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFI1pCUyKAObXW_lnaL-RrelMZtIn3Qwks5ute8_gaJpZM4XCCgM> .

mjpt777 · 2018-11-17T16:19:48Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java


        sb.append(String.format("\n" +
            indent + "public %1$s Get%2$s(int index)\n" +
            indent + "{\n" +
-            indent + INDENT + "if (index < 0 || index >= %3$d)\n" +
-            indent + INDENT + "{\n" +
+            indent + INDENT + "if (index < 0 || index >= %3$d) {\n" +


Why change the generated format here? Will this not now be inconsistent?

damageboy force-pushed the csharp/span branch from 2d6ab4d to 1c25ed3 Compare October 1, 2018 17:20

damageboy mentioned this pull request Oct 9, 2018

[C#] Overhal unit testing state #592

Open

damageboy force-pushed the csharp/span branch from 1c25ed3 to dd54c66 Compare October 13, 2018 14:56

damageboy force-pushed the csharp/span branch from dd54c66 to 5223d74 Compare October 25, 2018 14:13

mjpt777 merged commit 1793c8a into aeron-io:master Nov 17, 2018

mjpt777 reviewed Nov 17, 2018

View reviewed changes

mjpt777 added a commit that referenced this pull request Dec 3, 2018

[Java] Fix formatting after merge of PR #589.

f6a7e26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In/outgoing Span<byte> support, linux net45 build #589

In/outgoing Span<byte> support, linux net45 build #589

damageboy commented Oct 1, 2018

damageboy commented Oct 1, 2018

damageboy commented Oct 7, 2018

billsegall commented Oct 7, 2018 via email

damageboy commented Oct 7, 2018

ZachBray commented Oct 8, 2018

ZachBray commented Oct 8, 2018

billsegall commented Oct 8, 2018 via email

billsegall commented Oct 8, 2018 via email

damageboy commented Oct 8, 2018 •

edited

Loading

mjpt777 commented Oct 8, 2018

ZachBray commented Oct 8, 2018

ZachBray commented Oct 8, 2018

damageboy commented Oct 9, 2018

mjpt777 commented Nov 9, 2018

billsegall commented Nov 11, 2018 via email

mjpt777 Nov 17, 2018

In/outgoing Span<byte> support, linux net45 build #589

In/outgoing Span<byte> support, linux net45 build #589

Conversation

damageboy commented Oct 1, 2018

damageboy commented Oct 1, 2018

damageboy commented Oct 7, 2018

billsegall commented Oct 7, 2018 via email

damageboy commented Oct 7, 2018

ZachBray commented Oct 8, 2018

ZachBray commented Oct 8, 2018

billsegall commented Oct 8, 2018 via email

billsegall commented Oct 8, 2018 via email

damageboy commented Oct 8, 2018 • edited Loading

mjpt777 commented Oct 8, 2018

ZachBray commented Oct 8, 2018

ZachBray commented Oct 8, 2018

damageboy commented Oct 9, 2018

mjpt777 commented Nov 9, 2018

billsegall commented Nov 11, 2018 via email

mjpt777 Nov 17, 2018

Choose a reason for hiding this comment

damageboy commented Oct 8, 2018 •

edited

Loading