jamescourtney / FlatSharp

Fast, idiomatic C# implementation of Flatbuffers
Apache License 2.0
510 stars 51 forks source link

Closer integration with Unity Native Collections types #303

Closed vvuk closed 1 year ago

vvuk commented 2 years ago

I'm trying to figure out (including implementing in FlatSharp) how to better interop efficiently with Unity's "Native Collections" types -- e.g. NativeArray<T> (there's also NativeList<T>, but for my purpose just straight NativeArray is probably sufficient; NativeList<T> might be a natural extension and useful for writing though).

NativeArray<T> is essentially just a T* + count (plus some memory safety/usage tracking stuff that we can largely ignore). The memory is always from an unmanaged heap.

I'd like to be able to:

  1. declare that arrays are to be generated as NativeArray<T>.
  2. when writing into an object, to be able to assign an existing NativeArray<T> to a field, and have serialization just read from that into the output buffer
  3. when reading from an object, to get a NativeArray<T> (well, the ReadOnly variant) that references the underlying buffer
  4. when serializing, serialize to a NativeArray.

For 1-3, I think implementing a UnityNativeArrayVectorTypeModel in FlatSharp, along with appropriate Read/Write NativeArray methods in InputBufferExtensions and SpanWriterExtensions should do it. One question for reading -- from IInputBuffer, I can get a Memory<byte>. Is this memory already pinned, and guaranteed to be pinned while the input buffer is still alive? I'm basically calling Pin() on that and then grabbing the pointer value and stuffing it into a NativeArray, but I have to Dispose of the Pin. Writing is straightforward. I'd like to support arrays of structs in this way as well. (Also another question -- why is Memory<> only supported for byte currently? Just because the underlying input buffer uses a Memory<byte> and so it's only possible to slice that with the same type?)

For 4, I can actually already access the NativeArray as a Span, so I can just use that existing write method.

Does the above look reasonable, or is there anything major I'm missing?

The only other issue would be how to actually compile while linking with the assemblies that define the Unity types; we never pulled those out into a separate assembly (we really, really should) so I'm thinking to just create a dummy assembly with the right types/methods/etc. to link with for building.

vvuk commented 2 years ago

First stab at this: https://github.com/jamescourtney/FlatSharp/compare/main...vvuk:FlatSharp:unity-collections

I've got the linkage mostly resolved, though I still need a UnityEngine.dll to link with. This should gain some defines for unity support and be built out-of-band from nuget-land, with some defines for unity support in place.

jamescourtney commented 2 years ago

This is another good idea :) Thanks for all of your suggestions!

I'm sure if you've searched through my posts, I have absolutely no knowledge about Unity (though I'd like to!). That change looks reasonable at first glance. Can you supply any documentation I can use to get a mini-unity environment set up so I can play with that code?

vvuk commented 2 years ago

Will do -- I still need to figure out how to do this in a reasonable way, as the types themselves are only defined in UnityEngine.dll which I can reference from inside a Unity install (and it's valid to do so), but can't distribute. This is partially why in the PR I add some .Unity assemblies to avoid having to add that reference to to the base FlatSharp assemblies. I think I can at least create an assembly with an implementation of NativeArray and friends that can be used for building/testing and for running the compiler, but building the runtime portion without a real UnityEngine is going to be tricky.

One option that would solve the runtime issue would be to create a Unity package with the FlatSharp runtime source, as opposed to precompiled DLLs. Would you be open to that? That would also be the best debugging experience. I can set up a script that will generate that package within the repo as part of a build process.

jamescourtney commented 2 years ago

So, it may not be necessary to carry a reference to Unity itself. FlatSharp doesn't carry any gRPC references today, yet it still emits code for that scenario. That said, if FlatSharp is going to have deeper integrations with Unity, those do need to be testable, so the minimum requirement would be a way to unit test the Unity integrations.

jamescourtney commented 2 years ago

@vvuk -- do you have any more thoughts here? I'm thinking that I'm going to rev FlatSharp to version 7 soon, which is an opportunity for breaking changes.

vvuk commented 2 years ago

I started following one of the existing vector type integrations, and then redid it a bit — they support both runtime and AOT, and I need to just support AOT. That said the problem I ran into was that the flatsharp compiler needs to be able to reference the Unity types in order to emit valid c# with Roslyn that use those types. Any suggestions on how to avoid that? Maybe I can just add the dummy library to only the compilation references?

If so that may be sufficient. I’ll give that a try, because it would also solve unit testing (if that library also had a proper impl of NativeArray)

jamescourtney commented 2 years ago

Yeah -- FlatSharp is super reflectiony internally, which incidentally is the obstacle to using source generators. One thing -- and I think what is you were suggesting -- is that we can try is adding polyfills for the Unity types, sort of like I've done for some of the nullable annotations here: Polyfills.cs.

Additionally, the FlatSharp type model is (mildly) extensible via the TypeModelContainer class. So, what we could do is something approaching the following:

joncham commented 2 years ago

I have a "working" implementation that took a different (likely less acceptable) approach. I'll PR that just for discussion. Then I'll look into the suggestions above. Thanks @jamescourtney !

jamescourtney commented 2 years ago

Thanks -- happy to take a look!

jamescourtney commented 1 year ago

@joncham -- version 7.1 is now published. I'm going to close this. Thanks again for your contribution!