jamescourtney / FlatSharp

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

Add support for Unity NativeArray as vector type #319

Closed joncham closed 1 year ago

joncham commented 2 years ago

Implementation for issue https://github.com/jamescourtney/FlatSharp/issues/303

Initial work to support Unity's NativeArray as a supported vector type.

Highlights:

Open Questions

  1. I initially had the Unity APIs withing the FlatSharp.Compiler assembly itself. This worked until I started writing tests, as they APIs would either need to be public or the internals would need exposed to tests. Is the current "stub" assembly approach acceptable?
  2. The NativeArray is constructed from the buffer/memory passed into ReadNativeArrayBlock. A copy is not performed. Is it safe to assume this memory is native or pinned managed memory that will not be moved?
  3. I added test for simple usage of NativeArray which just validates the compiler generates proper code. I can add tests for actual functionality by further stubbing out Unity APIs. Any input on this?

Thanks!

codecov[bot] commented 2 years ago

Codecov Report

Merging #319 (c2e8ca5) into main (359b753) will decrease coverage by 0.28%. The diff coverage is 81.96%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/FlatSharp.UnityPolyfills/NativeArray.cs 69.84% <69.84%> (ø)
...peModel/Vectors/UnityNativeArrayVectorTypeModel.cs 82.08% <82.08%> (ø)
src/FlatSharp/TypeModel/UnityTypeModelProvider.cs 83.33% <83.33%> (ø)
src/FlatSharp.Compiler/FlatSharpCompiler.cs 96.94% <100.00%> (+0.13%) :arrow_up:
...atSharp.Compiler/SchemaModel/PropertyFieldModel.cs 95.92% <100.00%> (+0.01%) :arrow_up:
src/FlatSharp.Runtime/IO/InputBufferExtensions.cs 96.29% <100.00%> (+0.55%) :arrow_up:
src/FlatSharp.Runtime/IO/SpanWriterExtensions.cs 100.00% <100.00%> (ø)
...latSharp/TypeModel/TypeModelContainerExtensions.cs 100.00% <100.00%> (ø)
src/FlatSharp.Runtime/IO/ArrayInputBuffer.cs 100.00% <0.00%> (+1.58%) :arrow_up:
src/FlatSharp.Runtime/IO/MemoryInputBuffer.cs 100.00% <0.00%> (+1.58%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 359b753...c2e8ca5. Read the comment docs.

joncham commented 2 years ago

I pushed some changes that address many of the issues raised and with some other differences. I can add more details later, but quickly:

  1. New command line option --unity-assembly-path which a) enables Unity support b) takes a path to an assembly providing Unity APIs
  2. Moved some types from compiler to runtime assembly. This allows Unity support to work in runtime compilation mode (not just AOT)
  3. Further stubbed out NativeArray. This implementation is quite different than Unity but functional.
  4. Implemented shared code path for NativeArray (de)serialization. This makes a copy of the underlying buffer data.
  5. Added a bunch of tests.

Next, I'll look into proper lazy deserialization/pinned support for NativeArray deserialization (avoiding the copy).

joncham commented 2 years ago

@jamescourtney is it worth splitting this PR up a bit? For example the IsPinned property on the buffer interface/implementations?

jamescourtney commented 2 years ago

@jamescourtney is it worth splitting this PR up a bit? For example the IsPinned property on the buffer interface/implementations?

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.

joncham commented 2 years ago

Okay, with 327fe1feda803e96904f29d5174cb79bb9282e74 I have:

  1. Added helper methods to InputBufferExtensions and SpanWriterExtensions to read/write from a Span
  2. Removed Unity src/FlatSharp.UnityPolyfills/UnityExtensions.cs
  3. Codegen'd the body of the NativeArray serialize/parse in UnityNativeArrayVectorTypeModel, using only Unity public APIs and the new helpers added in 1.

This means src/FlatSharp.UnityPolyfills is solely for testing purposes and should only contain stubs for existing Unity public APIs. It should not need included in a new or existing NuGet package. I'll rename it in a subsequent PR. Should it move until the Tests directory?

It also means that no extra source file is needed for "deployment" into Unity anymore implementing the read/write methods.

joncham commented 2 years ago

And thanks for the help and feedback @jamescourtney , I think we are iterating to a good solution.

jamescourtney commented 2 years ago

@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 net7.0 from the various .csproj elements to unblock yourself.

jamescourtney commented 2 years ago

@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!

joncham commented 2 years ago

@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.

jamescourtney commented 1 year ago

Hi @joncham -- I'm going to take your breaking changes for .IsPinned and the Memory<byte> constructor and go ahead and commit those on their own. I'm eager to get Flatsharp v7 released and taking just these changes will make the rest of this not a breaking change.

joncham commented 1 year ago

Hi @joncham -- I'm going to take your breaking changes for .IsPinned and the Memory<byte> constructor and go ahead and commit those on their own. I'm eager to get Flatsharp v7 released and taking just these changes will make the rest of this not a breaking change.

Thanks @jamescourtney , I appreciate you grabbing those changes!

joncham commented 1 year ago

Note, I pushed a rebased branch. I still didn't address all issues yet @jamescourtney but wanted to keep branch up to date.

jamescourtney commented 1 year ago

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 :)

jamescourtney commented 1 year ago

Hey @joncham -- thanks for the latest push. That looks reasonable to me. Do you feel that you are ready to merge?

joncham commented 1 year ago

@jamescourtney I am actively using this branch now in Unity, so I am comfortable enough merging if you are.

joncham commented 1 year ago

Hey @jamescourtney just pinging again to get your thoughts.

jamescourtney commented 1 year ago

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.