jamescourtney / FlatSharp

Fast, idiomatic C# implementation of Flatbuffers
Apache License 2.0
497 stars 50 forks source link

Add Match method to Union types #416

Closed parched closed 6 months ago

parched commented 8 months ago

This is one of the changes we have in the fork we're using at our company.

It adds a Match method to unions similar to other unions in the C# ecosystem, e.g., OneOf, dunet.

It avoids the need to create a visitor class for one time use.

We use it like this:

public int StepCount => Match(
    single => 1,
    multistep => multistep.StepCount,
    modal => modal.Modes.Count,
    envelope => 2
);

If you're happy to accept a change like this. I'll tidy it up a bit and add tests where necessary.

jamescourtney commented 8 months ago

Thanks for the contribution! Give me a couple of days to mull this over; FlatSharp's unions used to support behavior similar to this. The reason that I moved away from it was for performance reasons. Basically, if you implement your visitor as a struct, the JIT can make things faster by removing virtual method calls and object allocations for the delegates.

The other is that newer syntax innovations have made an interface implementation lighter weight than it used to be. In terms of lines of code, this isn't that different than inline delegates:

class MyClass
{
     void DoSomething(SomeUnion union)
     {
           union.Accept(new SimpleVisitor());
     }

     private struct SimpleVisitor : SomeUnion.Visitor<int>
     {
           public int VisitDog(Dog d) => d.Age;
           public int VisitCat(Cat c) => c.LivesRemaining;
     }
}

The general rule I try to follow is that I assume people are using FlatBuffers because they care about performance, so I try to structure the APIs to encourage performance.

That said, I completely see your reasoning. My day job isn't terribly performance-sensitive, so I'd probably also be irritated that these unions don't support a .Match pattern :)

parched commented 8 months ago

Fair enough, I'll create a benchmark to see how much difference there is. As I understand, the object allocations for the delegate shouldn't be a problem as they'll be lifted to static members if they don't capture anything.

jamescourtney commented 7 months ago

Did you ever get results of the benchmark? Another way to do this would be to have a DelegateVisitor implementation that is easy to construct.

parched commented 7 months ago

No I haven't, but I'll have some time coming up next week to look at this upstreaming.

jamescourtney commented 7 months ago

I've looked a little bit at this using LinqPad, and the delegate implementation looks substantially worse in terms of number of bytes emitted by the JITer. You can find a gist here: https://gist.github.com/jamescourtney/1929e73d551bc1e4f73592cfde09d145. The JITer is also not creating static methods for the delegates -- new objects are allocated for each method call.

The short version is that the struct implementation of the visitor has the best characteristics and maximum inlining. I feel pretty strongly that the FlatSharp API should steer users to write things efficiently since that is the point of FlatBuffers :) However, I recognize that the delegate approach is useful for oneoffs or other cases where performance may not matter as much.

Perhaps the question is: "how can we structure the API so that it steers people to do the right thing while making it clear that the delegate approach is not as good?"

parched commented 7 months ago

We could just add a note in the doc comment for Match that Visit should be used for better performance.

For what it's worth, we chose FlatSharp because we wanted a language agnostic IDL with generators for multiple programming languages, but with an idiomatic C# implementation (which is our main language). I didn't find any other options. Performance was just a secondary benefit.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6260caf) 97.31% compared to head (5bf8b7a) 97.22%. Report is 6 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416/graphs/tree.svg?width=650&height=150&src=pr&token=6EUECHZGT4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney)](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney) ```diff @@ Coverage Diff @@ ## main #416 +/- ## ========================================== - Coverage 97.31% 97.22% -0.10% ========================================== Files 126 126 Lines 9698 9738 +40 Branches 786 787 +1 ========================================== + Hits 9438 9468 +30 - Misses 169 179 +10 Partials 91 91 ``` | [Files](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney) | Coverage Δ | | |---|---|---| | [....Compiler/SchemaModel/ReferenceUnionSchemaModel.cs](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney#diff-c3JjL0ZsYXRTaGFycC5Db21waWxlci9TY2hlbWFNb2RlbC9SZWZlcmVuY2VVbmlvblNjaGVtYU1vZGVsLmNz) | `96.49% <100.00%> (+0.46%)` | :arrow_up: | | [...harp.Compiler/SchemaModel/ValueUnionSchemaModel.cs](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney#diff-c3JjL0ZsYXRTaGFycC5Db21waWxlci9TY2hlbWFNb2RlbC9WYWx1ZVVuaW9uU2NoZW1hTW9kZWwuY3M=) | `97.20% <100.00%> (+0.24%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney). Last update [6260caf...5bf8b7a](https://app.codecov.io/gh/jamescourtney/FlatSharp/pull/416?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=James+Courtney).
jamescourtney commented 7 months ago

Agree -- You'll want to do a pull from my main branch since I've made some changes to exception handling. You should be able to emulate what I do in Accept for the new Match method.

Please also add a comment to both Accept and Match that indicates the performance characteristics of each. Use codeWriter.AppendSummaryComment for this.

Regarding testing, please add some tests in FlatSharpEndToEndTests. There should already be some union cases you can add to. Regarding mutation testing, the pipeline may fail for you. This is done with dotnet-stryker and is the most rigorous set of tests that I've written. It can be a little tricky to get working. Feel free to look at the .yml if you want to give it a shot, otherwise I'm happy to write the tests.

jamescourtney commented 6 months ago

I've implemented this and published in FlatSharp 7.6.0