Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C# passing Refs to Slices #17

Closed
dbenson24 opened this issue Aug 15, 2021 · 10 comments
Closed

C# passing Refs to Slices #17

dbenson24 opened this issue Aug 15, 2021 · 10 comments
Labels
c-backend-c# C# Backend enhancement Make existing things better.

Comments

@dbenson24
Copy link

In order to use extern functions in burst, structs have to be passed by reference. If I try changing all my slices to be &mut slices, the extern functions are generated nicely with out SliceMut but I lose the other functions that are normally generated with them. (The ones with the pinning of arrays and conversion to SliceMut. If I want those helper functions for both Slice and &mut Slice does this mean that I need to add a pattern for handling the &mut version of a Slice the same way that the Slice is handled to the C# generator?
As an aside I would also like the ability to generate slices from native arrays. I see that you've already got some Unity specific flagging, would you be interested in PRs to add some more Unity specific interoperability to the generated c# behind those flags?

@ralfbiedert ralfbiedert added c-backend-c# C# Backend c-proc Proc Macros enhancement Make existing things better. labels Aug 15, 2021
@ralfbiedert
Copy link
Owner

In order to use extern functions in burst, structs have to be passed by reference. If I try changing all my slices to be &mut slices, the extern functions are generated nicely with out SliceMut but I lose the other functions that are normally generated with them. (The ones with the pinning of arrays and conversion to SliceMut.

I don't quite understand. Would you have a code sample, both Rust and C#, and what you'd like to happen instead?

As an aside I would also like the ability to generate slices from native arrays. I see that you've already got some Unity specific flagging, would you be interested in PRs to add some more Unity specific interoperability to the generated c# behind those flags?

Yes. I haven't fully made up my mind if #if's are the way to go, but feel free to start a PR and we can discuss. If most conditionals stay localized (e.g., within a single method or ctor invisible from the outside) I think I'm happy with it.

@ralfbiedert ralfbiedert removed the c-proc Proc Macros label Aug 15, 2021
@dbenson24
Copy link
Author

dbenson24 commented Aug 15, 2021

 [DllImport(NativeLib, CallingConvention = CallingConvention.Cdecl, EntryPoint = "query_ray")]
        public static extern int query_ray(ref BVHRef bvh_ref, ref Double3 origin_vec, ref Double3 dir_vec, out SliceMutBVHBounds shapes, out SliceMutBVHBounds buffer);
#[ffi_function]
#[no_mangle]
pub extern "C" fn query_ray(bvh_ref: &BVHRef, origin_vec: &Double3, dir_vec: &Double3, shapes: &mut FFISliceMut<BVHBounds>, buffer: &mut FFISliceMut<BVHBounds>) -> i32
{
}

Is what I need to use so that the burst compiler is happy because it won't pass structs to extern by value.
But when I declare it like that I lose this function in the generated C# (the helper that accepts an array)

public static int query_ray(ref BVHRef bvh_ref, ref Double3 origin_vec, ref Double3 dir_vec, BVHBounds[] shapes, BVHBounds[] buffer) {
            unsafe
            {
                fixed (void* ptr_shapes = shapes)
                {
                    var shapes_slice = new SliceMutBVHBounds(new IntPtr(ptr_shapes), (ulong) shapes.Length);
                    fixed (void* ptr_buffer = buffer)
                    {
                        var buffer_slice = new SliceMutBVHBounds(new IntPtr(ptr_buffer), (ulong) buffer.Length);
                        return query_ray(ref bvh_ref, ref origin_vec, ref dir_vec, shapes_slice, buffer_slice);
                    }
                }
            }
        }

This only gets generated when I declare the function like this
(the only difference being mut slice instead of slice: &mut)

#[ffi_function]
#[no_mangle]
pub extern "C" fn query_ray(bvh_ref: &BVHRef, origin_vec: &Double3, dir_vec: &Double3, mut shapes: FFISliceMut<BVHBounds>, mut buffer: FFISliceMut<BVHBounds>) -> i32
{
  }
        [DllImport(NativeLib, CallingConvention = CallingConvention.Cdecl, EntryPoint = "query_ray")]
        public static extern int query_ray(ref BVHRef bvh_ref, ref Double3 origin_vec, ref Double3 dir_vec, SliceMutBVHBounds shapes, SliceMutBVHBounds buffer);

Notice that now the Slice isn't passed by ref which will upset the Burst Compiler

ralfbiedert added a commit that referenced this issue Aug 15, 2021
@ralfbiedert
Copy link
Owner

ralfbiedert commented Aug 15, 2021

I need to read this again tomorrow, but I've started on a pattern_ffi_slice_5 which should reflect that use case.

#[ffi_function]
#[no_mangle]
pub extern "C" fn pattern_ffi_slice_5(slice: &FFISlice<u8>, slice2: &mut FFISliceMut<u8>) {
    let _ = slice.as_slice().len();
    let _ = slice2.as_slice().len();
}

One immediate issue was that &mut SliceMut reference always got transformed out Slice, which IIRC has wrong CLR semantics (as it won't necessarily copy the struct towards Rust). That should be fixed in the latest commit.

Getting the overloads to work can be a bit more tricky, but hopefully doable. As a stopgap you should be able to implement these overloads now manually with correct [In] / [Out] semantics in a companion Interop.handcrafted.cs.

@dbenson24
Copy link
Author

ah I was going to ask about the out modifier, it didn't quite seem like the right one (I was expecting ref) but I wasn't sure if you had made that decision knowing something I did not.
There's no real time crunch on my end, I have fully functional c# bindings for my entire API that I wrote by hand. But maintaining/extending them is starting to become a pain and this crate looked like a perfect fit. And so far the switch has been easy and the generated bindings look great. Thanks for all your hard work

@ralfbiedert
Copy link
Owner

I just realized Burst neither supports this:

var x = new uint[] { 1, 23 };
Interop.pattern_ffi_slice_1(x);

nor this:

Interop.pattern_ffi_slice_1(Test.MY_STATIC_ARRAY);

Is there any (Burst) use case for an interop method more complex than a simple ref SliceXXX, as seemingly you can't even invoke any useful overload?

public static extern void pattern_ffi_slice_1(ref Sliceu32 slice);

@dbenson24
Copy link
Author

When in burst you have data in the NativeArray class. There are some other types but they generally provide "as_array" like methods to read them as NativeArrays. NativeArrays themselves are allocated in unmanaged memory by Unity and are basically already Slices, they don't need to be pinned. For max burst compatibility I would want to generate an API that has overloads to allow either an [] or a NativeArray whenever slices are being passed

@dbenson24
Copy link
Author

I'll give this a test when you push the next version that has it

@ralfbiedert
Copy link
Owner

Pushed 0.9.4.

Constructor overloads accepting NativeArray exist for slices now, but you (still) need to create them manually before invoking a native method.

I'm exploring if better method overloads are possible, but I want to avoid combinatorial explosion accounting for all possible parameter types (IntPtr, NativeArray, int[] ... ) and patterns (e.g., automatic try{} checking on FFIErrors); and it's not entirely clear if a good automatic solution exists.

Callback delegates are another can of worms as Burst doesn't like them either.

Long story short, I hope 0.9.4 helps you make some progress.

@ralfbiedert
Copy link
Owner

Version 0.10 should have better support for Burst (and Unity in general). All of these should work (assuming FUNCTION_POINTER was set):

    [BurstCompile(CompileSynchronously = true)]
    private struct MyJob : IJob
    {
        [ReadOnly]
        public NativeArray<byte> bytes1;

        [ReadOnly]
        public NativeArray<byte> bytes2;
        
        public FunctionPointer<CallbackU8> FUNCTION_POINTER;
        
        [BurstCompile]
        public static byte CallbackU8(byte x0)
        {
            return x0;
        }
        
        public void Execute()
        {
            Interop.ref_mut_simple(out long xx);
            Interop.pattern_ffi_slice_5(bytes1, bytes2);
            Interop.pattern_ffi_slice_6(bytes1, FUNCTION_POINTER.Value);
        }
    }

Can be enabled like so; mind the new .add_overload_writer():

use interoptopus::util::NamespaceMappings;
use interoptopus::{Error, Interop};

#[test]
fn bindings_csharp() -> Result<(), Error> {
    use interoptopus_backend_csharp::{Config, Generator, Unsafe};
    use interoptopus_backend_csharp::overloads::{DotNet, Unity};

    let config = Config {
        dll_name: "example_library".to_string(),
        use_unsafe: Unsafe::UnsafePlatformMemCpy,
        namespace_mappings: NamespaceMappings::new("My.Company"),
        ..Config::default()
    };

    Generator::new(config, example_library_ffi::my_inventory())
        .add_overload_writer(DotNet::new())
        .add_overload_writer(Unity::new())
        .write_file("bindings/csharp/Interop.cs")?;

    Ok(())
}

Unfortunately I don't have a Burst project where I regularly test these things. However, in the new structure there now is a very tentative Unity test against which people can file PRs.

@dbenson24
Copy link
Author

I can make a new issue if you'd prefer, but just a headsup. Any IntPtrs on structs generated by this need to be marked with [NativeDisableUnsafePtrRestriction] in order to be used with burst stuff (it's a runtime enforced exception)

[Serializable]
[StructLayout(LayoutKind.Sequential)]
public partial struct SliceMutBvhNode
{
[NativeDisableUnsafePtrRestriction]
IntPtr data;
ulong len;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-backend-c# C# Backend enhancement Make existing things better.
Projects
None yet
Development

No branches or pull requests

2 participants