Add support for Unity NativeArray as vector type #319
Conversation
Codecov Report
@@ Coverage Diff @@
## main #319 +/- ##
==========================================
- Coverage 96.41% 96.13% -0.29%
==========================================
Files 111 115 +4
Lines 8563 8742 +179
Branches 803 815 +12
==========================================
+ Hits 8256 8404 +148
- Misses 204 230 +26
- Partials 103 108 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Overall, this looks like you're on the right track. Thanks for making the code look like the rest of the code in this repo (writing code to generate code is often awkward -- let me know if you know of any better techniques!). I do have some questions about where the implementations of the other parts of the #ifdef declarations live, and I'd like to see more robust testing, if that's something that's practical.
Regarding your questions:
-
I think the current stub approach is acceptable.
-
You cannot make this assumption. FlatSharp doesn't pin or otherwise care about the
IInputBufferinstance. I think there are two components of the solution here:The first is to make a copy, which is what I thought the code you wrote did. Of course, that's not very FlatBuffers-y, but FlatSharp does offer
Greedymodes, so some support there would make things predictable, so we do need the ability to copy if the user has requested it. Basically, when you use Greedy mode, FlatSharp promises that the underlying buffer is no longer referenced at all and you're free to throw it away or overwrite it. Of course, you really shouldn't use Greedy mode, but it's a safe default option that might not perform great but definitely won't corrupt your data. If copies are not possible or otherwise difficult, I'd also be OK with throwing an exception if deserializing aNativeArrayin a Greedy context. Basically, my preference is always to fail loudly rather than lie.The other part of the solution is to create a new
IInputBufferalong the lines ofPinnedInputBuffer. Then, if you were asked to deserialize aNativeArray<T>with anIInputBufferof a different type in a non-greedy context, you could bail immediately and just throw aInvalidOperationException. This is what FlatSharp does today withReadOnlyMemoryInputBufferwhen trying to access a writableMemoryorSpan. For the structure, I recommend a struct for the implementation ofIInputBufferwith an inner class with the appropriate Finalizers (again, just likeReadOnlyMemoryInputBuffer) since this allows the JIT (not sure about IL2CPP) to devirtualize all the method calls. Another option would be to add a.IsPinnedproperty to theIInputBufferinterface so you could interrogate that property instead of the type of buffer, which might make things more extensible. Since FlatSharp 7 isn't out yet, this is a good time for breaking changes. You'll want to update the__AotHelpermethod to include your new InputBuffer type since (I think) Unity needs this to help with AOT and Generics. -
Always love more tests. I think a solution where we can interoperate between a
NativeArrayand anIListof valuetypes with different schemas would be ideal from my perspective (ideally the raw bytes would be the same). I know that these won't be "real" tests, but do at least build a little confidence I'm not unintentionally regressing Unity scenarios later. On a side note, do you have any suggestions how I might get some "real" Unity automation set up in this repo? Even a small separate test project that just runs as a Github action?
src/FlatSharp.Compiler.UnityStub/FlatSharp.Compiler.UnityStub.csproj
Outdated
Show resolved
Hide resolved
|
|
||
| public override CodeGeneratedMethod CreateCloneMethodBody(CloneCodeGenContext context) | ||
| { | ||
| string body = $"throw new NotSupportedException();"; |
There was a problem hiding this comment.
this will break copy constructors that the compiler generates. Since it fails predictably, that seems not-terrible, but it's going to be a non-obvious failure. Is there a case where NativeArrays should be cloneable? Since they seem immutable it might be enough to just do the equivalent of what we do for strings and primitives where we just return the original value and the whole thing becomes a no-op.
There was a problem hiding this comment.
They are mutable. I am looking into handling some sort of Clone now.
|
I pushed some changes that address many of the issues raised and with some other differences. I can add more details later, but quickly:
Next, I'll look into proper lazy deserialization/pinned support for NativeArray deserialization (avoiding the copy). |
There was a problem hiding this comment.
Thanks for the changes here! I have some more comments, but they are a bit lower level than before, which is usually a good sign.
Apologies for two different reviews. I did the first one and then realized I had more comments, which I started adding and then realized it might be easier for you if I had unified everything together.
I mentioned this in one comment, but a lot of times I just ask questions in PRs for the sake of facilitating discussion; if you feel strongly, please disagree with me and we can chat about it!
| @@ -32,6 +32,8 @@ public ArrayInputBuffer(byte[] buffer) | |||
|
|
|||
| public bool IsReadOnly => false; | |||
|
|
|||
| public bool IsPinned => false; | |||
There was a problem hiding this comment.
Can you add a few tests in InputBufferTests.cs to exercise this property?
FlatSharp\src\Tests\FlatSharpTests\ClassLib\InputBufferTests.cs
| internal Memory<T> m_Buffer; | ||
|
|
||
| internal void* m_Data; | ||
| internal int m_Length; |
There was a problem hiding this comment.
This goes along with my comment from the compiler, but I think if we just relax the : struct constraint and just update the internals of this to be a simple T[] or something, we'll end up with a simpler solution. Performance absolutely doesn't matter here, so there's no reason to do anything fancy.
I don't particularly care if the FlatSharp version of NativeArray works the same way the Unity version does under the hood; I mostly just want something that looks the same so I have confidence the code is working.
| Persistent = 4, | ||
| } | ||
|
|
||
| public unsafe struct NativeArray<T> : IEnumerable<T>, IEquatable<NativeArray<T>> where T : struct |
There was a problem hiding this comment.
Code coverage tools are mad at this file. Can you add [ExcludeFromCodeCoverage] here?
There was a problem hiding this comment.
Also, does this need to be behind a #ifdef?
| { | ||
| public static class UnityExtensions | ||
| { | ||
| public static NativeArray<TElement> ReadNativeArrayBlock<TElement, TBuffer>(this TBuffer buffer, int uoffset) |
There was a problem hiding this comment.
As I'm looking at this, I have a few questions...
- Will generated code need to depend on this Polyfill assembly like it does FlatSharp.Runtime?
- Would it perhaps be better to emit this from the Type Model codegen instead of a method here? That would remove such a requirement to have that reference for generated code while keeping the
NativeArrayimplementation for basically reflection and testing only purposes? This is similar to how the FlatSharp compiler does gRPC today -- it doesn't actually have an assembly reference to gRPC. Rather, it just spits out a bunch of code that happens to compile when gRPC is referenced. But in that case, we won't care which NativeArray is referenced, only that the type exists.
There was a problem hiding this comment.
I don't think publishing FlatSharp.Unity.Tools as a separate NuGet package is bad or anything. I'm just wondering if we can keep the dependencies simpler.
There was a problem hiding this comment.
And feel free to disagree with me here! Another Nuget package sounds pretty reasonable to me; I often talk out loud just to make sure we've considered all options.
There was a problem hiding this comment.
- Will generated code need to depend on this Polyfill assembly like it does FlatSharp.Runtime?
It will depend on UnityExtensions.cs only. The NativeArray.cs file will be fulfilled by assemblies present in the Unity environment.
But in that case, we won't care which NativeArray is referenced, only that the type exists.
This should be the case today. Tests run with the NativeArray stubbed here. Unity uses it's own implementation.
Would it perhaps be better to emit this from the Type Model codegen instead of a method here?
I have debated this, but I still need to resolve a NativeArray type. Since I already will require an external assembly reference for the NativeArray type, the hand written C# has not been too burdensome. Also, it's easier to reason about and iterate on as a static file ATM.
There was a problem hiding this comment.
Since UnityExtensions is in a different DLL, then we'll definitely need to push that as a separate package, correct?
|
@jamescourtney is it worth splitting this PR up a bit? For example the |
I'm comfortable with the size of it, but I'll be happy to go whatever way you want. I'm also happy to check in and iterate as long as we have items to track various discussion points. |
|
Okay, with 327fe1f I have:
This means It also means that no extra source file is needed for "deployment" into Unity anymore implementing the read/write methods. |
|
And thanks for the help and feedback @jamescourtney , I think we are iterating to a good solution. |
| if (!buffer.IsPinned) | ||
| throw new NotSupportedException(""Lazy parsing of a NativeArray requires a pinned buffer.""); | ||
| var bufferSpan = {context.InputBufferVariableName}.UnsafeReadSpan<{context.InputBufferTypeName}, {this.ItemTypeModel.GetGlobalCompilableTypeName()}>({context.OffsetVariableName}); | ||
| return Unity.Collections.LowLevel.Unsafe.NativeArrayUnsafeUtility.ConvertExistingDataToNativeArray<{this.ItemTypeModel.GetGlobalCompilableTypeName()}>(bufferSpan, Allocator.None); |
There was a problem hiding this comment.
Sadly, you'll have to also emit the ENABLE_COLLECTIONS_CHECKS #ifdef and the manual safety handle setting, despite Alloactor.None. This is a super annoying wart with this API. (This should crash if you try to read from one of these returned arrays in the editor or in development player builds currently.)
e.g.:
var foo = NativeArrayUnsafeUtility.ConvertExistingDataToNativeArray<>(...);
#if ENABLE_UNITY_COLLECTIONS_CHECKS
NativeArrayUnsafeUtility.SetAtomicSafetyHandle(ref foo, AtomicSafetyHandle.GetTempUnsafePtrSliceHandle());
#endif
return foo;
But I don't think TempUnsafePtrSliceHandle is correct. (It'll work fine for now.) If we want to be totally correct, we can allocate a safety handle that corresponds to the original buffer we're creating the array from, and use that. That way if the input buffer goes away for some reason and someone kept a NativeArray around, they'll get a proper error.
| @@ -30,17 +53,34 @@ public override CodeGeneratedMethod CreateParseMethodBody(ParserCodeGenContext c | |||
| context.AllFieldContexts, | |||
| context.Options); | |||
|
|
|||
| var lazySuffix = context.Options.Lazy ? "Lazy" : ""; | |||
| if (context.Options.Lazy) | |||
There was a problem hiding this comment.
I think this actually needs to be !context.Options.GreedyDeserialize. The distinction is a little subtle, but FlatSharp supports 4 deserialization options:
Lazy: What you'd expect.
Greedy / GreedyMutable: Also what you'd expect
Progressive: Lazy-with-caching. Basically, still references the underlying buffer but guarantees each thing is only loaded once. Faster than Lazy for repeated visits to the same field, only slightly slower than Lazy otherwise. It's really a pretty good one-size-fits-all solution.
So Progressive shouldn't make a copy.
| { | ||
| int numberOfItems = buffer.Length; | ||
| int vectorStartOffset = ctx.AllocateVector( | ||
| itemAlignment: Unsafe.SizeOf<TElement>(), |
There was a problem hiding this comment.
In FlatBuffers, structs are aligned to according to the max alignment of all their members. So a struct with an int64 member would have 8-byte alignment, etc. This is an artifact of the time when FlatBuffers was designed; some architectures at the time didn't support unaligned reads, so care was taken to align all the data. This turns out to be not a good choice for size on the wire, but the format is the format.
I think the case where a memory copy will be enough to properly serialize a struct is actually pretty narrow: There is no padding between members (that is -- StructSize % Alignment == 0). An example would be the typical Vector3 struct with 3 float32's inside.
However, there are other cases where a memory copy won't be enough:
-
Big endian machines. FlatBuffers encodes all data as little endian. FlatSharp at least has code sprinkled throughout trying to be thoughtful about BE architectures. Not that I have a BE machine to test it on, of course, But it's there. And it might even work :)
-
Cases where consecutive structs need padding between items (ie, not aligned according to the above).
The fallback for both of these cases would be to use a loop and serialize/parse "normally". You can take a look at what I do for value-type structs here. Basically, the logic is that if memory-marshalling is allowed by the user and it's a LE machine, then do it. Otherwise, go field-by-field.
That said, if Unity is LE only/mostly and you think it would be better to raise the error about poor alignment rather than have people get unexpectedly bad performance, then raising an error might be reasonable.
There was a problem hiding this comment.
For some examples, I have a few "nightmare struct" tests:
There was a problem hiding this comment.
Finally, I think I might also suggest experimenting to see if this should be inlined or not. Does IL2CPP actually honor that attribute?
| @@ -36,6 +38,27 @@ public static class SpanWriterExtensions | |||
|
|
|||
| memory.Span.CopyTo(span.Slice(vectorStartOffset + sizeof(uint))); | |||
| } | |||
|
|
|||
| public static void UnsafeWriteSpan<TSpanWriter, TElement>( | |||
There was a problem hiding this comment.
You actually don't need to pass all of these fields if you don't want. This is just the "default" serialization signature for FlatSharp's generated methods, but since this in effect a leaf node, feel free to throw other items in this signature as you need, such as item alignment, etc.
| // We need to construct a Span<TElement> from byte buffer that: | ||
| // 1. starts at correct offset for vector data | ||
| // 2. has a length based on *TElement* count not *byte* count | ||
| var byteSpanAtDataOffset = buffer.GetSpan().Slice(uoffset + sizeof(uint)); |
There was a problem hiding this comment.
One note on .GetSpan(), which is that ReadOnlyMemoryInputBuffer will throw for this case. Since NativeArray is mutable, this seems appropriate to me, but just wanted to call it out.
|
@joncham -- You may need to merge from main. One thing to keep in mind is that I've upgraded to target .net 7, which may cause Visual Studio to be unhappy. Unless you want to install the preview, just remove |
|
@joncham -- are you waiting on me to review here? I did take a quick look at your latest changes, but saw that the alignment issue I mentioned before wasn't fixed, so I didn't add any new comments because I assumed you were not done. Let me know if I've missed something or you are waiting on input from me. Thanks! |
@jamescourtney no, I am trying out the changes in some Unity scenarios to make sure things are working as I expect. I'll update PR to address your comments in coming days. |
|
Hi @joncham -- I'm going to take your breaking changes for |
Thanks @jamescourtney , I appreciate you grabbing those changes! |
joncham
force-pushed
the
unitySupport
branch
from
96feec3
to
4484b89
Compare
|
Note, I pushed a rebased branch. I still didn't address all issues yet @jamescourtney but wanted to keep branch up to date. |
That sounds good. Sorry for any merge conflicts. I hope that you didn't have too difficult of a time. The branch is going to be mostly quiescent for awhile now. I'm starting to work on moving some test from reflection based execution to precompiled, but I won't be touching the main code for awhile. Thanks for your work here! Looking forward to publishing this when it's done :) |
|
Hey @joncham -- thanks for the latest push. That looks reasonable to me. Do you feel that you are ready to merge? |
|
@jamescourtney I am actively using this branch now in Unity, so I am comfortable enough merging if you are. |
|
Hey @jamescourtney just pinging again to get your thoughts. |
Hey! Sorry about this. I intentionally tried to unplug a bit during the holidays, but I'm (reluctantly) back to work now so will take a look at this soon. |
Implementation for issue #303
Initial work to support Unity's NativeArray as a supported vector type.
Highlights:
FlatSharp.Compiler.UnityStub(naming?) added to provide stub/polyfill APIs needed for schema compilation as well as test compilation.src/FlatSharp.Compiler.UnityStub/UnityExtensions.csprovides the glue to Unity APIs.FLATSHARP_UNITY_POLYFILLSdefine) theNativeArraytype is stubbed and extension methods for reading/writing are empty, allowing the usage of types and API surfaces but no functionality.NativeArrayis provided by Unity assemblies and the reading/writing methods are implemented against further Unity APIs.FlatSharp.Compiler.UnityStubassembly will be deployed with the compiler, but should never be used at runtime in a Unity contextOpen Questions
NativeArrayis constructed from the buffer/memory passed intoReadNativeArrayBlock. A copy is not performed. Is it safe to assume this memory is native or pinned managed memory that will not be moved?Thanks!