dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

Stateful interop marshallers cannot be copy free on both managed and unmanaged side #97435

Open ThadHouse opened 8 months ago

ThadHouse commented 8 months ago

Description

When using a stateful interop marshaller, there is no way to make both the managed and unmanaged side a reference or a pointer. The only way to make the marshaller happy is to have either the managed input or unmanaged output be a byvalue copy.

Say I have the following interop signature (RefNetworkTableValue is a large ref struct, and is a const NativeNetworkTableValue* on the C side)

[LibraryImport("ntcore", EntryPoint = "NT_SetEntryValue")]
[UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])]
internal static unsafe partial void SetEntryValueIn(in RefNetworkTableValue value);

That is required to have the following marshaller shape for a stateful marshaller

[CustomMarshaller(typeof(RefNetworkTableValue), MarshalMode.ManagedToUnmanagedIn, typeof(RefNetworkTableValueMarshaller))]
public unsafe ref struct RefNetworkTableValueMarshaller
{
    public void FromManaged(in RefNetworkTableValue managed) {...}

    public NativeNetworkTableValue ToUnmanaged() {...}

    public void Free() {...}
}

Note that the ToUnmanaged call returns NativeNetworkTableValue by value. I can't make it return a ref, as the marshaller is not happy about that. Additionally, I can't make it return a NativeNetworkTableValue*, as that makes the signature on the generated end const NativeNetworkTableValue**, which is incorrect.

If I change the initial call to not be an in, as follows

[LibraryImport("ntcore", EntryPoint = "NT_SetEntryValue")]
[UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])]
internal static unsafe partial void SetEntryValueIn(RefNetworkTableValue value);

Then my generator is can return a pointer from ToUnmanaged, but there's now a copy of a large struct on the input side.

[CustomMarshaller(typeof(RefNetworkTableValue), MarshalMode.ManagedToUnmanagedIn, typeof(RefNetworkTableValueMarshaller))]
public unsafe ref struct RefNetworkTableValueMarshaller
{
    public void FromManaged(in RefNetworkTableValue managed) {...}

    public NativeNetworkTableValue* ToUnmanaged() {...}

    public void Free() {...}
}

Additionally, at least in my case, ToUnmanaged is likely too large of a function to be inlined, and I don't suspect the JIT would work around this issue in a different way.

ghost commented 8 months ago

Tagging subscribers to this area: @dotnet/interop-contrib See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When using a stateful interop marshaller, there is no way to make both the managed and unmanaged side a reference or a pointer. The only way to make the marshaller happy is to have either the managed input or unmanaged output be a byvalue copy. Say I have the following interop signature (RefNetworkTableValue is a large ref struct, and is a `const NativeNetworkTableValue*` on the C side) ``` [LibraryImport("ntcore", EntryPoint = "NT_SetEntryValue")] [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])] internal static unsafe partial void SetEntryValueIn(in RefNetworkTableValue value); ``` That is required to have the following marshaller shape for a stateful marshaller ``` [CustomMarshaller(typeof(RefNetworkTableValue), MarshalMode.ManagedToUnmanagedIn, typeof(RefNetworkTableValueMarshaller))] public unsafe ref struct RefNetworkTableValueMarshaller { public void FromManaged(in RefNetworkTableValue managed) {...} public NativeNetworkTableValue ToUnmanaged() {...} public void Free() {...} } ``` Note that the ToUnmanaged call returns `NativeNetworkTableValue` by value. I can't make it return a ref, as the marshaller is not happy about that. Additionally, I can't make it return a `NativeNetworkTableValue*`, as that makes the signature on the generated end `const NativeNetworkTableValue**`, which is incorrect. If I change the initial call to not be an `in`, as follows ``` [LibraryImport("ntcore", EntryPoint = "NT_SetEntryValue")] [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])] internal static unsafe partial void SetEntryValueIn(RefNetworkTableValue value); ``` Then my generator is can return a pointer from `ToUnmanaged`, but there's now a copy of a large struct on the input side. ``` [CustomMarshaller(typeof(RefNetworkTableValue), MarshalMode.ManagedToUnmanagedIn, typeof(RefNetworkTableValueMarshaller))] public unsafe ref struct RefNetworkTableValueMarshaller { public void FromManaged(in RefNetworkTableValue managed) {...} public NativeNetworkTableValue* ToUnmanaged() {...} public void Free() {...} } ``` Additionally, at least in my case, ToUnmanaged is likely too large of a function to be inlined, and I don't suspect the JIT would work around this issue in a different way.
Author: ThadHouse
Assignees: -
Labels: `area-System.Runtime.InteropServices`, `tenet-performance`, `untriaged`, `source-generator`
Milestone: -
AaronRobinsonMSFT commented 8 months ago

@ThadHouse I would avoid using in in most cases. It has very subtle semantics and in this case even if it is used I am guessing it will be spilled to the stack. I think ref readonly is more likely what you want - https://learn.microsoft.com/dotnet/csharp/language-reference/proposals/csharp-12.0/ref-readonly-parameters.

Can you share the RefNetworkTableValue definition?

ThadHouse commented 8 months ago

Switching to ref readonly breaks the source generator.

CSC : error CS8785: Generator 'LibraryImportGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NotImplementedException' with message 'Support for some RefKind'. [/Users/thadhouse/GitHub/wpilib/src/ntcore/ntcore.csproj]
/Users/thadhouse/GitHub/wpilib/src/ntcore/Natives/NtCore.ClientServer.cs(16,39): error CS8795: Partial method 'NtCore.GetNetworkMode(NtInst)' must have an implementation part because it has accessibility modifiers. [/Users/thadhouse/GitHub/wpilib/src/ntcore/ntcore.csproj]

https://github.com/robotdotnet/WPILib/blob/rebuild/src/ntcore/RefNetworkTableValue.cs https://github.com/robotdotnet/WPILib/blob/rebuild/src/ntcore/Natives/RefNetworkTableValueMarshaller.cs

At most of the callsites I am using an rvalue, so in does actually seem correct.

I also tried just a normal ref (not a ref readonly), by switching MarshalMode.ManagedToUnmanagedIn to MarshalMode.ManagedToUnmanagedRef, and that also broke the source generator. No matter what I do, and what shape I change the marshaller to, it tells me that type is not supported by the source generator. Although ref definitely doesn't work because I'm passing rvalues.

AaronRobinsonMSFT commented 8 months ago

@ThadHouse Appreciate the concern here. Is there any way you can confirm noticing a performance impact? The runtime has a type of RVO that should generally avoid duplication - see https://github.com/dotnet/runtime/pull/64130. If you can confirm there are multiple copies I think it might be more appropriate to look at what the JIT can do.

I'm going to move this into Future for now.