jamescourtney / FlatSharp

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

FlatSharp 5.6.0 net47 compiler generates code using ReadOnlySpan for a fixed length array #190

Closed tdmowrer closed 3 years ago

tdmowrer commented 3 years ago

I am using FlatSharp in the context of Unity, which limits me to .NET 4.7 or .NET Standard 2.0.

This schema:

struct Foo {
  v:[float:2];
}

Compiled with the net47 compiler generates C# code containing a ReadOnlySpan:

/// <summary>
/// Deep copies the first 2 items from the source into this struct vector.
/// </summary>
public void CopyFrom(ReadOnlySpan<System.Single> source)
{
    checked
    {
        var thisItem = this.item;
        var s = source;
        thisItem.__flatsharp__v_1 = FlatSharp.Compiler.Generated.CloneHelpers_2ac5afb7655d491a9c3b32ec33db4646.Clone(s[1]);
        thisItem.__flatsharp__v_0 = FlatSharp.Compiler.Generated.CloneHelpers_2ac5afb7655d491a9c3b32ec33db4646.Clone(s[0]);
    }
}

Which is incompatible with the .NET 4.7 framework. Is this expected?

Note this only happens when the fixed length array is of a primitive type (e.g., float). Fixed length arrays of user-defined types do not have this problem.

Possibly related thread: https://github.com/jamescourtney/FlatSharp/issues/125

jamescourtney commented 3 years ago

Thanks for reaching out. Happy to help investigate here.

For context -- everything FlatSharp does is based on Span<T>. I've had reports of other Unity users using FlatSharp successfully in #125 as you mentioned.

Is the problem specifically with ReadOnlySpan<T>, or does it affect both Span and ReadOnlySpan? This code definitely works in .NET Framework 4.7 (assuming you add a reference to System.Memory).

tdmowrer commented 3 years ago

everything FlatSharp does is based on Span<T>.

Thanks for the context -- good to know.

Is the problem specifically with ReadOnlySpan<T>, or does it affect both Span and ReadOnlySpan? This code definitely works in .NET Framework 4.7 (assuming you add a reference to System.Memory).

The only problematic generated code that I saw involved a ReadOnlySpan<T>. I would expect Span and ReadOnlySpan to have the same problem, but the compiler did not produce them from my schema. I agree it should be possible to add System.Memory for use in Unity.

Thanks for the quick response!

jamescourtney commented 3 years ago

Just to clarify -- are you unblocked? If adding System.Memory fixed your issue, I can add that to the docs.

tdmowrer commented 3 years ago

I decided not to go with FlatSharp and just use Google's FlatBuffers C# implementation, so I never got further than this.

One reason for this that may be of interest is that Google's implementation uses C# structs for the types generated from the schema, while FlatSharp seems to prefer use of classes. I (and Unity users in general) try to avoid per frame GC allocations. Looks like this was considered in https://github.com/jamescourtney/FlatSharp/issues/164.

jamescourtney commented 3 years ago

Sounds good -- thanks for letting me know. Flatsharp does everything by inheritance, which isn't a thing with structs. I'm not sure how Unity's GC works, but I at one point had a branch of FlatSharp that did offer "struct structs", and it performed worse in my testing than classes. However, I'm not a Unity expert by any stretch, so YMMV. Cheers!