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

Emit direct delegates when possible #1083

Open
5 tasks done
dsyme opened this issue Oct 4, 2021 · 12 comments
Open
5 tasks done

Emit direct delegates when possible #1083

dsyme opened this issue Oct 4, 2021 · 12 comments

Comments

@dsyme
Copy link
Collaborator

dsyme commented Oct 4, 2021

F# currently never emits "direct" delegates - it only ever emits delegates that go via a closure object.

As background, .NET IL permits the emit of direct delegates, e.g. for the code:

delegate D of someArg: int -> int

type C() = 
    static method M(arg:int) = 1

System.Func<int,int>(C.M)

F# currently always emits a closure. Instead it is possible to emit

ldnull // load null because method is static
ldftn void C::M(int)
newobj instance void class System.Func`2<int, int>::.ctor(object, native int)

That is, a direct delegate is not a closure but rather a delegate with delegate.TargetMethod known to be the exact MethodInfo for the target.

This needs careful specification and is best done as an RFC as the differences are, in some sense, observable to reflection.

The basic situation is

SomeDelegateType<...>(expr)

The cases we care about are where the target is a "known" function or method, for example:

System.Action<_,_>(handler) // non-eta-expanded known target 
System.Action<_,_>(fun a b -> handler a b) // eta-expanded known target 
System.Action<_,_>(fun a b -> handler (a,b)) // eta-expanded known target with different currying/tupling but same compiled representation
System.Action<_,_>(SomeType.StaticMethod) // same with a static method
System.Action<_,_>(fun a b -> SomeType.StaticMethod a b) // same with a static method
System.Action<_,_>(fun a b -> SomeType.StaticMethod(a,b)) // same with a static method
System.Action<_,_>(SomeType<...>.StaticMethod<...>) // same with a generic static method 
System.Action<_,_>(fun a b -> SomeType<...>.StaticMethod<...> a b) // same with a generic static method
System.Action<_,_>(fun a b -> SomeType<...>.StaticMethod<...>(a,b)) // same with a generic static method
System.Action<_,_>(someObject.InstanceMethod) // same with an instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod a b) // same with an instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod(a, b)) // same with an instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod<...> a b) // same with a generic instance method
System.Action<_,_>(fun a b -> someObject.InstanceMethod<...>(a, b)) // same with a generic instance method

Arguably we may also care about partial applications - here a closure is needed but the argument names generated for the closure could be those drawn from the elided arguments of the obvious target:

System.Action<_,_>(handler arg1 arg2) // non-eta-expanded known target via partial application
System.Action<_,_>(SomeType.StaticMethod arg1) // non-eta-expanded known target via partial application

The questions are

  1. When do we guarantee to create a direct delegate?
  2. If we don't, do we guarantee to at least create a closure with specific argument names

We have to consider these questions for both Debug and Release code.

The proposal for the cases above is:

  1. Direct delegate?

    • For non-eta-expanded - currently no - proposal is to change this to "yes" for both debug and release code
    • For eta-expanded - currently no - proposal is to change this to "yes" for release code. The user has made the lambdas explicit, however release code can be more efficient eliminating intermediate closures. For debug code we would still generate a closure with the user-given argument names.
  2. If no direct delegate, then what are the closure argument names?

    • For non-eta-expanded - currently you get a closure with funny argument names "delegateArg0" etc. The proposal is to always generate a direct delegate, so there is nothing more to do beyond that
    • For non-eta-expanded partial applications - currently you get a closure with funny argument names "delegateArg0" etc. The proposal is to use the argument names drawn from the obvious target
    • For eta-expanded - currently we use the argument names derived from the patterns in the source code. The proposal is to leave this unchanged for debug code. In release code we will generate a direct delegate.
  3. Are these changes visible in quotations?

    • The proposal is that quotations wouldn't change.
  4. Are these changes visible to FCS?

    • The proposal is that FCS reported expressions won't change.

This is in theory a potential breaking change because user code doing inspection on delegate value .TargetMethod may detect the difference. However within the context of the language spec it's a reasonable change as the TargetMethod is currently a closure and so inspection is not particularly useful, and the language spec gives no guarantees about this situation. For this reason it is reasonable to make this change via an RFC and /langversion

The transformation would be implemented in the F# optimizer and Ilxgen.

Pros and Cons

The advantages of making this adjustment to F# are efficiency and more direct translation of F# code

The disadvantages of making this adjustment to F# are it's work.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@dsyme
Copy link
Collaborator Author

dsyme commented Oct 4, 2021

One problem with the above is that in the case of a multi-argument delegate there's no simple way to always get a direct delegate

Consider this code:

type C() = 
    static method M(arg1: int, arg2: int) = 1

System.Func<int,int,int>(C.M)

This doesn't compile - the problem is that F# asks delegates to be implemented using curried functions, so an explicit lambda is needed:

type C() = 
    static method M(arg1: int, arg2: int) = 1

System.Func<int,int,int>(fun a b -> C.M(a,b))

Per the above spec, a direct delegate would only be generated for release code, but not debug code. We could in theory always generate a direct delegate here even for debug code though it would be a little unusual because you would then no longer be able to breakpoint inside the lambda on the call to C.M(a,b). There's no obvious solution to this dilemma - we have problems if we do, and problems if we don't.

Note there are situations where C# APIs actually do care about the names of arguments and characteristics of target methods - they use inspection to do various things and it can even effect data decoding and deserialization. As far as I understand things this includes the ASP.NET Core Minimal WebAPI implementation.

@dsyme
Copy link
Collaborator Author

dsyme commented Oct 4, 2021

I'll mark this as approved in principle, though there is one technical question remaining as mentioned above.

@dsyme
Copy link
Collaborator Author

dsyme commented Oct 4, 2021

As an aside, it should be noted that delegate construction always delays (and reevaluates) the relevant function on each call, so, for example

new System.Action<int>(failwith "")

is equivalent to

new System.Action<int>(fun arg -> (failwith "") arg)

and doesn't fail on delegate construction, but rather delegate invocation.

@dsyme
Copy link
Collaborator Author

dsyme commented Oct 4, 2021

Notes on the testing matrix for this

  • "unit returning" cases should be considered. This is the matrix of cases where the compiled representation of the target method returns void or unit or a generic type variable instantiated to unit, and the delegate likewise has signature with return unit or a generic type variable instantiated to unit.

  • targets which are structs v. ref types should be considered.

  • targets which are generic v. non-generic should be considered.

  • targets which are F# methods v. IL methods should be considered.

  • targets which are generic with witnesses v. not

  • targets which are extension methods or not

  • targets which are static or instace

  • targets which are virtual and non-virtual

@dsyme
Copy link
Collaborator Author

dsyme commented Oct 8, 2021

Note to self: when creating first-class lambdas for .NET methods we aren't giving good names to implied arguments for the lambda. This can be improved:

> System.IO.File.OpenText;;
val it : arg00:string -> System.IO.StreamReader

@Dolfik1
Copy link

Dolfik1 commented Nov 10, 2021

Emit direct delegates might be useful with mono p/invoke callbacks:
https://stackoverflow.com/a/28570186

We are interested in this code:

CaptureDelegate d = MyCapture;
block_value.SetupBlock(d, null);

...

[MonoPInvokeCallback(typeof(CaptureDelegate))]
static void MyCapture(IntPtr block, IntPtr self, IntPtr uiView)
{
...

This code can be written in F# as follows:

let d = CaptureDelegate(MyClass.MyCapture)
block_value.SetupBlock(d, null)

but this throws an exception at runtime:
The method <StartupCode$MyApp>.$MyClass+d@78.Invoke does not have a [MonoPInvokeCallback] attribute.

Perhaps this is incorrect behavior of the compiler and compiler should add attributes to the method of closure object. @dsyme what do you think?

I found a way to create "direct" delegate via reflection:

let d = Delegate.CreateDelegate(typeof<CaptureDelegate>, typeof<MyClass>.GetMethod("MyCapture"))
block_value.SetupBlock(d, null)

and this code works well. But this looks like a dirty hack.

@teo-tsirpanis
Copy link

I will try to fix it one day; don't know when I will have time.

@piaste
Copy link

piaste commented Jun 13, 2022

Considering the parameter name issue reported in #1145, does that not indicate that debug and release code should stay aligned?

The OP says:

This is in theory a potential breaking change because user code doing inspection on delegate value .TargetMethod may detect the difference. However within the context of the language spec it's a reasonable change as the TargetMethod is currently a closure and so inspection is not particularly useful, and the language spec gives no guarantees about this situation. For this reason it is reasonable to make this change via an RFC and /langversion

but if the user code relying on this inspection for API contracts is the ASP.NET Core framework, that's a whole other order of magnitude of breaking change.

@NinoFloris
Copy link

NinoFloris commented Jul 13, 2022

Relevant minimal api issue dotnet/aspnetcore#38906

Important to note, the one way to add attributes to parameters (for instance for FromHeader and friends) would be to pull out the lambda to a named function. This then breaks the parameter name reflection. So realistically you're stuck if you need to do this.

@cmeeren
Copy link

cmeeren commented Jan 12, 2023

Does this suggestion cover dotnet/fsharp#14582?

@vshapenko
Copy link

Is there any hope this behaviour will be implemented? Because right now, for example, openapi is completely broken in f# minimal api apps

@vzarytovskii
Copy link

Is there any hope this behaviour will be implemented? Because right now, for example, openapi is completely broken in f# minimal api apps

Sure, It's not planned as for now, but we'll accept RFC and implementation.

brianrourkeboll added a commit to brianrourkeboll/fsharp that referenced this issue May 9, 2024
* Override `ToString` on `BuildPhase`.

* Cache the delegate passed into `ConcurrentDictionary.GetOrAdd` where
  possible. See dotnet#14582, fsharp/fslang-suggestions#1083, etc.
psfinaki added a commit to dotnet/fsharp that referenced this issue May 13, 2024
* Minor compiler perf improvements

* Override `ToString` on `BuildPhase`.

* Cache the delegate passed into `ConcurrentDictionary.GetOrAdd` where
  possible. See #14582, fsharp/fslang-suggestions#1083, etc.

* Name

* Update release notes

* Fmt

* Remove unneeded `StringBuilder`

* Start count at 1

* Go back to `GetOrAdd`

* I don't think I fully understand why, but I did some rough
  microbenchmarking, and for some reason the `GetOrAdd` and
  `Interlocked.Increment` on a ref cell technique is actually something
  like twice as fast as `AddOrUpdate`.

---------

Co-authored-by: Petr <psfinaki@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants