feat: add Span<T> params to C# struct Create methods for zero-allocation stackalloc support#8970
feat: add Span<T> params to C# struct Create methods for zero-allocation stackalloc support#8970statxc wants to merge 6 commits intogoogle:masterfrom
Conversation
…ion stackalloc support
|
@jtdavis777 Could you please review this PR? I'd appreciate any feedbacks. Thanks! |
|
@statxc In practice, it is better to use |
Yes. Good point! 100% right. Our Create methods only read from the array parameters - they never write to them. So the parameter type should be ReadOnlySpan instead of Span. Thanks @ODtian 👍. I will update soon |
|
@ODtian I updated all! Could you review again? thanks |
|
@statxc The rest looks good to me. I'm not very familiar with FlatBuffers itself though, so it'd be best to have someone with more expertise take a look. |
|
@jtdavis777 could you please review this PR? Welcome to any feedbacks |
|
@jtdavis777 Sorry for ping again. Any updates for me, please |
|
@jtdavis777 I saw your post in the discussion (#8984). Thanks for your passion - I'm really glad to see this community. |
|
I will admit I know very little about C# (sorry to all the Microsoft Java fans out there) but know that I see you and will help as best I can! |
|
I hope someone expert can review this PR as well. |
|
I like this PR. I'm not sure I qualify as an expert, but I've spent a decent amount of time working with FlatBuffers and minimizing heap allocations in C#.
If there is an instance where a 2D array is needed, when array_cnt > 0, you could instead trigger the creation of an overloaded CreateXxx that accepts the array types. Then you're covered with backward compatibility and you can avoid changes to Object API Pack().
My take is that ENABLE_SPAN_T needs to be paired with UNSAFE_BYTEBUFFER given the following warning in the runtime code: #if ENABLE_SPAN_T && !UNSAFE_BYTEBUFFER Consider adding UNSAFE_BYTEBUFFE to your #ifs. Other thoughts: Now that you're using spans, FlatBufferBuilder Put can be used reduce for loops (flattened 2d operations could just 'Slice' each row and Put()). However, those Put functions don't do endian swapping per span element, chance of unexpected breaks for big endian users. |
…sharp/span-params-create-struct
…oads alongside them instead of replacing them
f673e3d to
fee49cc
Compare
|
@bigjt-dev |
|
Nice, LGTM. I did pull your changes to see how the param types would resolve. I saw this test was failing: FixedLengthArray_CreateWithStackalloc_ReturnTrue. The nestedStruct.__assign needs fixed in that test plus a few other call sites external to your changes. I'd change to: nestedStruct.__assign(buffer.GetInt(buffer.Position) + buffer.Position, buffer); to get to the right spot. |
…sharp/span-params-create-struct
…he root offset after Finish
|
@jtdavis777 @bigjt-dev ready to merge now. please review again. thanks |
Summary
Span<T>parameters to C# structCreateXXXmethods under#if ENABLE_SPAN_T, enablingstackallocfor zero-GC struct creationT[,]) to 1DSpan<T>with computed indices for nested struct arraysT[]still compiles since arrays implicitly convert toSpan<T>Changes
src/idl_gen_csharp.cpp: AddSpan<T>codegen path withStructHasArrayFields()helper and flat indexing supporttests/MyGame/Example/{NestedStruct,ArrayStruct,LargeArrayStruct}.cs: Regenerated viaflatctests/FlatBuffers.Test/FlatBuffersExampleTests.cs: Updated test for flat array params underENABLE_SPAN_Ttests/FlatBuffers.Test/FlatBuffersFixedLengthArrayTests.cs: Addedstackalloctest demonstrating zero-allocation usageHow to test
Test plan
UNSAFE_BYTEBUFFER=true: 156 tests passedENABLE_SPAN_T=true+UNSAFE_BYTEBUFFER=true: 156 tests passedflatcoutput (zero diff)Closes #8927