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

Expose an Unsafe.CallIndirect method. #23062

Closed
tannergooding opened this issue Aug 4, 2017 · 16 comments
Closed

Expose an Unsafe.CallIndirect method. #23062

tannergooding opened this issue Aug 4, 2017 · 16 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime.CompilerServices
Milestone

Comments

@tannergooding
Copy link
Member

Rationale

Today, in C#, you can only invoke an unmanaged function pointer by first calling Marshal.GetDelegateForFunctionPointer and then invoking the resulting delegate that is created.

This not only comes with the increased overhead of the call (Marshal.GetDelegateForFunctionPointer) itself, but it also incurs a heap allocation and a 4-6x memory increase, not including internal runtime allocations/overhead -- the MulticastDelegate type has an object and an IntPtr field and the base Delegate type itself has two object and two IntPtr fields -- Invoking the method also has some increased overhead as it goes through the Delegate.Invoke call rather than as a direct invocation.

The C# language has planned support for static delegates in the future (https://github.com/dotnet/csharplang/blob/c5de26f52019ca0e1e79d88584f75bfe530ee986/proposals/static-delegates.md), but those may still be a way off and could potentially not come at all.

Other languages may also face similar issues if they do not provide equivalent support.

Proposal

We should expose a new method on the System.Runtime.CompilerServices.Unsafe type that functions as an equivalent to the calli instruction.

namespace System.Runtime.CompilerServices
{
    public static class Unsafe
    {
        // Potential other names include: Call, Invoke, InvokeIndirect, Calli, etc...
        public static TResult CallIndirect<TDelegate, TResult>(IntPtr ptr, params object[] args);
    }
}

The runtime should treat this as an equivalent to the calli instruction with a signature matching TDelegate.

The runtime should validate that TResult/args matches the return/input type of TDelegate.

Other Thoughts

Using generics means that pointers are harder to use, there would have to be support to interpret IntPtr and UIntPtr args (those passed to CallIndirect) as equivalent to a pointer (as declared for the corresponding field in TDelegate).

The current proposal (using params object[]) means boxing for arguments, maybe having explicit <TDelegate, TArg, TResult>, <TDelegate, TArg1, TArg2, TResult> overloads would be better?

  • I can provide some details for the min/max/avg number of args the interop delegates take (at least for DirectX).

I think, for either case, out and ref would not be readily supported unless Unsafe.AsRef were used internally (also having special handling to interpret IntPtr and UIntPtr as ref equivalents).

Example Usage

A user might have a manually authored interop type such as:

[Guid("7B7166EC-21C7-44AE-B21A-C9AE321AE369")]
unsafe public /* blittable */ struct IDXGIFactory
{
    #region Fields
    public readonly Vtbl* lpVtbl;
    #endregion

    #region Delegates
    [SuppressUnmanagedCodeSecurity]
    [UnmanagedFunctionPointer(CallingConvention.ThisCall, BestFitMapping = false, CharSet = CharSet.Unicode, SetLastError = false, ThrowOnUnmappableChar = false)]
    [return: ComAliasName("HRESULT")]
    public /* static */ delegate int EnumAdapters(
        [In] IDXGIFactory* This,
        [In, ComAliasName("UINT")] uint Adapter,
        [Out] IDXGIAdapter** ppAdapter
    );

    [SuppressUnmanagedCodeSecurity]
    [UnmanagedFunctionPointer(CallingConvention.ThisCall, BestFitMapping = false, CharSet = CharSet.Unicode, SetLastError = false, ThrowOnUnmappableChar = false)]
    [return: ComAliasName("HRESULT")]
    public /* static */ delegate int MakeWindowAssociation(
        [In] IDXGIFactory* This,
        [In, ComAliasName("HWND")] void* WindowHandle,
        [In, ComAliasName("UINT")] uint Flags
    );

    [SuppressUnmanagedCodeSecurity]
    [UnmanagedFunctionPointer(CallingConvention.ThisCall, BestFitMapping = false, CharSet = CharSet.Unicode, SetLastError = false, ThrowOnUnmappableChar = false)]
    [return: ComAliasName("HRESULT")]
    public /* static */ delegate int GetWindowAssociation(
        [In] IDXGIFactory* This,
        [Out, ComAliasName("HWND")] void** pWindowHandle
    );

    [SuppressUnmanagedCodeSecurity]
    [UnmanagedFunctionPointer(CallingConvention.ThisCall, BestFitMapping = false, CharSet = CharSet.Unicode, SetLastError = false, ThrowOnUnmappableChar = false)]
    [return: ComAliasName("HRESULT")]
    public /* static */ delegate int CreateSwapChain(
        [In] IDXGIFactory* This,
        [In] IUnknown* pDevice,
        [In] DXGI_SWAP_CHAIN_DESC* pDesc,
        [Out] IDXGISwapChain** ppSwapChain
    );

    [SuppressUnmanagedCodeSecurity]
    [UnmanagedFunctionPointer(CallingConvention.ThisCall, BestFitMapping = false, CharSet = CharSet.Unicode, SetLastError = false, ThrowOnUnmappableChar = false)]
    [return: ComAliasName("HRESULT")]
    public /* static */ delegate int CreateSoftwareAdapter(
        [In] IDXGIFactory* This,
        [In, ComAliasName("HMODULE")] void* Module,
        [Out] IDXGIAdapter** ppAdapter
    );
    #endregion

    #region Structs
    public /* blittable */ struct Vtbl
    {
        #region Fields
        public IDXGIObject.Vtbl BaseVtbl;

        public IntPtr EnumAdapters;

        public IntPtr MakeWindowAssociation;

        public IntPtr GetWindowAssociation;

        public IntPtr CreateSwapChain;

        public IntPtr CreateSoftwareAdapter;
        #endregion
    }
    #endregion
}

In order to invoke EnumAdapters today, the user would code something similar to:

var EnumAdapters = Marshal.GetDelegateForFunctionPointer<IDXGIFactory.EnumAdapters>(pFactory->lpVtbl->EnumAdapters);

IDXGIAdapter* pAdapter;
for (var adapterIndex = 0u; DXGI_ERROR_NOT_FOUND != EnumAdapters(pFactory, adapterIndex, &pAdapter); adapterIndex++)
{
    // Check if the adapter supports D3D12
}

Under the proposal, someone would now declare:

IDXGIAdapter* pAdapter;
for (var adapterIndex = 0u; DXGI_ERROR_NOT_FOUND != Unsafe.CallIndirect<IDXGIFactory.EnumAdapters, int>(pFactory->lpVtbl->EnumAdapters, pFactory, adapterIndex, pAdapter); adapterIndex++)
{
    // Check if the adapter supports D3D12
}
@MichalStrehovsky
Copy link
Member

I would kind of prefer the C# compiler team adds dotnet/roslyn#11475 to their schedule instead. We have had to work around that in CoreRT for years.

If someone isn't satisfied with the Marshal.GetDelegateForFunctionPointer overhead, I doubt they would be happy with the array allocation and boxing required to do each call. (It would definitely not help us in CoreRT land.)

@tannergooding
Copy link
Member Author

I doubt they would be happy with the array allocation and boxing required to do each call

That is why I suggested "maybe having explicit <TDelegate, TArg, TResult>, <TDelegate, TArg1, TArg2, TResult> overloads would be better" in the other thoughts section.

I would kind of prefer the C# compiler team adds dotnet/roslyn#11475 to their schedule instead

I fully expect we will get proper static delegate support (which compiles down to calli) before we get support for compiler intrinsics (at least based on dotnet/csharplang#789, where the static delegates proposal is on the roadmap, but Compiler Intrinsics is not).

The point of this proposal is to provide a stop-gap between now and C# 8 (or later, if it gets pushed out) and to provide a stop-gap mechanism for other languages missing such a feature.

Providing generic overloads for the arguments (<TDelegate, TArg, TResult>) avoids the boxing problem and will be able to cover most methods. A general params object[] overload will cover the remaining cases and can at least avoid some of the remaining overhead (but not all of it).

@mellinoe
Copy link
Contributor

mellinoe commented Aug 4, 2017

If someone isn't satisfied with the Marshal.GetDelegateForFunctionPointer overhead, I doubt they would be happy with the array allocation and boxing required to do each call. (It would definitely not help us in CoreRT land.)

I certainly wouldn't switch any of my calli usage (done through a rewriter step similar to CoreRT) to this if it involved an array allocation and boxing per-parameter. I think that is clearly a non-starter. Providing a bunch of fixed-arity generic overloads would be closer, but the problem of pointers is a bad one, IMO. Most of your calli targets will involve pointer parameters, realistically speaking, and now they will all need to be represented as IntPtr. If you want strongly-typed pointer overloads, you'll have to write another wrapper. Modifying the runtime in support of Unsafe is not really reasonable -- none of the other methods we've added have been treated specially at all by the runtime(s), they all map to simple IL instructions which are well-supported by all of our existing runtimes.

The point of this proposal is to provide a stop-gap between now and C# 8 (or later, if it gets pushed out) and to provide a stop-gap mechanism for other languages missing such a feature.

I get what you're saying, but there's not really such a thing as a "stop-gap" for the BCL. This would have to be supported forever, on all of our runtimes, even when the proper solutions (static delegates or compiler intrinsics) come into existence.

Is there a way we can "bump up the priority" of the language features (either/or/both)? It feels to me that they would be very, very helpful at addressing a clear gap in the current system.

@MichalStrehovsky
Copy link
Member

dotnet/roslyn#11475 is not a language feature - it's just a matter of someone sitting down and doing it. I'll buy beers (or other beverage of their choosing) to the person who makes it happen because I really want to use it in CoreRT. The feature was discussed in LDM in January. I would hope we can find someone to do it in the 7.x timeframe.

@mellinoe
Copy link
Contributor

mellinoe commented Aug 4, 2017

@MichalStrehovsky Good point, it's just a "compiler feature". I also really want to use it, and I've discussed it with a few other projects who would use it.

@tannergooding
Copy link
Member Author

I get what you're saying, but there's not really such a thing as a "stop-gap" for the BCL. This would have to be supported forever, on all of our runtimes, even when the proper solutions (static delegates or compiler intrinsics) come into existence.

I think there are always going to be languages that don't support this (possibly just because it is out of scope for the language).

I also think, that while compiler intrinsics are nice, it is probably more worthwhile to invest in actual language features that expose things in desired ways (for the compiler team, what is the benefit of providing a compiler intrinsic for calli when they are already looking at exposing static delegates which does the same thing in a much better way).

I think the Unsafe class should be the thing that fills in these gaps instead, as it makes it available to all languages, both existing and future, for when they don't expose the functionality themselves (most of these 'missing' features seem to be fairly consistent and are already exposed in Unsafe).

Is there a way we can "bump up the priority" of the language features (either/or/both)? It feels to me that they would be very, very helpful at addressing a clear gap in the current system.

Sadly, I don't think so. Like most of the teams, there is plenty of backlog for requested features, planned features, etc and only so many devs that can work on them. So it all comes down to priorit, need, etc

@MichalStrehovsky
Copy link
Member

Sadly, I don't think so. Like most of the teams, there is plenty of backlog for requested features

Yet you're proposing a feature that puts it on the backlog of CoreCLR, CoreRT/.NET Native, and Mono teams :). System.Runtime.CompilerServices.Unsafe targets NetStandard 1.0 - requiring runtime features will require bumping it way up.

when they are already looking at exposing static delegates which does the same thing in a much better way

The proposed intrinsics have a broader applicability because they're not limited to static methods. Static delegates would only cover maybe a half of my use cases and I'm only half as excited about them.

@tannergooding
Copy link
Member Author

Yet you're proposing a feature that puts it on the backlog of CoreCLR, CoreRT/.NET Native, and Mono teams :).

I'm slightly more confident in being able to implement one of those myself :)

@mjsabby
Copy link
Contributor

mjsabby commented Mar 25, 2018

The right thing to do is for the C# Compiler to just implement the ability to emit calli, it's been a while since the intrinsic proposal was first initially floated.

I even have a working implementation I've been using for a year or so that we use internally at Bing.

mjsabby/roslyn@e6123e0 (or as a nuget package like Microsoft.Net.Compilers package, https://www.nuget.org/packages/CSCWithCompilerIntrinsics)

The only part I don't like about my implementation is that it generates the calli in a method body. The JIT team fixed inlining calli methods with default calling convention which partially mitigates this for my scenario. (https://github.com/dotnet/coreclr/issues/12713)

Prior to this I had my own tool like @mellinoe, https://github.com/mjsabby/PInvokeCompiler and I'm sure many others have reinvented their own.

So I don't think the proposal here makes sense, but we should ping the Compiler Intrinsics proposal again.

@stephentoub
Copy link
Member

@tannergooding, can this be closed in favor of the function pointer support coming in C#?
https://github.com/dotnet/csharplang/blob/56d8447281d054d4acf20e501f41a40c2db009cd/proposals/function-pointers.md

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@nietras
Copy link
Contributor

nietras commented May 1, 2020

<TDelegate, TArg, TResult>, <TDelegate, TArg1, TArg2, TResult> overloads would be better

@tannergooding I have a need for this and thought perhaps you might detail how you would write the IL for this e.g. how would you implement the below in IL:

public static TResult CallIndirect<TDelegate, TArg1, TArg2, TResult>(
    IntPtr ptr, TArg1 arg1, TArg2 arg2);

and what other alternatives are there to using TDelegate to define the "call-site description"? And would delegates with similar signature be interchangeable?

I understand you might use SignatureHelper to define the call-site description, but wondering the pros and cons to this.

UPDATE: The question of static vs instance methods also pops up. How is/could that be handled?

@tannergooding
Copy link
Member Author

I would likely wait for C# 9 and function pointers at this point. They will allow you to properly use calli without needing any hacky workarounds or IL rewriting.

There is a lot of complexity around calli, as detailed above, and you would need different helpers for all the various argument overloads and possible calling conventions.
The idea around this proposal was to have the JIT dynamically construct the calli based on the given TDelegate signature and arguments/operands, but it had its own limitations as well.

@nietras
Copy link
Contributor

nietras commented May 1, 2020

would likely wait for C# 9 and function pointers at this point

I can't wait 😉

idea around this proposal was to have the JIT dynamically construct the calli based on the given TDelegate signature and arguments/operands, but it had its own limitations as well

Right, that makes more sense to me then. Anyway, I really want to try to do this, since I have an immediate need, but also just as a learning exercise :) Would you mind if I ask some questions around this? I could give a concrete example for some specific types and take it from there. The idea was to add this to my own copy of Unsafe e.g. DotNetCross.Memory.Unsafe with specific limitations around use cases.

@tannergooding
Copy link
Member Author

tannergooding commented May 1, 2020

I'm happy to answer questions but I don't believe you will be able to implement this as proposed in any form of library without JIT support.

calli takes a callsitedescr, which is a metadata token for a standalone signature. The standalone signature needs to specify the calling convention, parameter/return types, and iirc cannot use generics.
And so the entire basis behind the proposal was that the JIT could dynamically construct the relevant information based on the given generics and workaround the limitations of IL.

The next best thing, if waiting for function pointers to get merged to master and be available in nightlies is too long, is likely using something like Fody to do IL rewriting to use calli instead.

@nietras
Copy link
Contributor

nietras commented May 1, 2020

if waiting for function pointers to get merged to master and be available in nightlies is too long

Sounds like you expect this sooner than I would have thought? :)

UPDATE: As an outsider viewing at the below two issues would indicate there is still some ways to go :)

dotnet/roslyn#39865

dotnet/roslyn#43321

@nietras
Copy link
Contributor

nietras commented May 1, 2020

@tannergooding anyway, to ask a specific question, lets look at the following type (just an example):

    public class ComparableClassInt32 
        : IComparable<ComparableClassInt32>
    {
        public readonly int Value;

        public ComparableClassInt32(int value) =>
            Value = value;

        public int CompareTo(ComparableClassInt32 other) => 
            Value.CompareTo(other.Value);
    }

Now I would like to call CompareTo but as an open method that is basically have the equivalent CallIndirect to:

public static int CallIndirect(IntPtr methodPtr, ?? callSiteDescriptor, object x, object y)

That is x is equivalent to this. With the call site descriptor then referring to the CompareTo method for the above type. Call site descriptor can be cached, but the call has to be as fast as possible for this type, that is as fast as a "normal" call in a generic setting.

That is in my case, this is just for calling managed code, not interop. Hence, calling conventions is not an issue I guess.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests

7 participants