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

Lift byref generics restrictions for inline functions #688

Closed
5 tasks done
NinoFloris opened this issue Aug 30, 2018 · 16 comments
Closed
5 tasks done

Lift byref generics restrictions for inline functions #688

NinoFloris opened this issue Aug 30, 2018 · 16 comments

Comments

@NinoFloris
Copy link

NinoFloris commented Aug 30, 2018

I propose to lift byref generics restrictions for inline functions.

Since all the new byref things have landed in 4.5 I've been playing quite a bit with Span. It's awesome it all works so well already and correctly prevents you from going off the rails.
It is definitely restrictive and it complicates generic programming a lot. It might especially shock people where they normally don't realize this applies, like piping |> or functions like id. The reason for this is purely a limitation of the CLR and its implementation of generics, it cannot accept a byref type argument, however F# could.

F# already has ways to deal with other generics limitations by marking functions as inline. This effectively removes this function from the runtime's view which allows the language to relax restrictions around generic constraints or limitations.

For instance lifting restrictions on byref for generic functions should allow the following contrived example to pass compilation:

Span<int>.Empty |> id

Both |> and id are generic, but both are also already marked inline today.

Making this change is neatly in line ;) with previous choices for inline to lift generics limitations and it greatly increases ergonomics working with byreflike types and byref arguments.

@TIHan has also quickly checked the feasibility of this and found a place to implement this inside PostInferenceChecks.fs where it can be, in theory, implemented in a way to guarantee correctness.

Pros and Cons

The advantages of making this adjustment to F# are increased language ergonomics and consistency as not being able to use byref in generics is purely a CLR restriction. This is opens up a lot of possibilities to add some sugar on interactions with Span and friends. And it might also help in performance scenarios where passing structs byref is preferred but where generic code is important too or used a lot. In the face of F# dissuading people from doing method overloading it is a good option to have.

The disadvantages of making this adjustment to F# are extra complexity in the compiler. A function marked as inline should, for byref call sites, not contain any calls to other generic functions that aren't marked inline as well.

Extra information

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

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
@cartermp
Copy link
Member

The advantages of making this adjustment to F# are increased language ergonomics and consistency as not being able to use byref in generics is purely a CLR restriction.

Agreed on this. It's so commonplace to pipe data into another function that it's a guaranteed surprise to first-time users of Span and other byref-like types that they cannot use |>.

Another thing worth calling out is that an error message when applying a byref-like type to a generic function that isn't inline needs to have a descriptive error message (tracked by dotnet/fsharp#5351). Otherwise, people will definitely be confused about why some functions "work" and others don't.

@dsyme
Copy link
Collaborator

dsyme commented Oct 9, 2018

I think this is basically reasonable though it's very tricky to implement the appropriate checks.

  • Currently all byref checks are done in PostInferenceChecks, that last phase of errors reported in the IDE.

  • The F# compiler does not run inlining before implementing PostInferenceChecks - inlining is implemented in the later Optimizer phase.

  • Running the Optimizer in the IDE is not feasible for performance reasons

  • Implementing inlining and reduction in the PostInferenceChecks is not feasible for complexity reasons

  • It's very hard to come up with a spec of allowable functions that doesn't really amount to implementing both inlining and reduction.

@abelbraaksma
Copy link
Member

@dsyme, wouldn't most of the tricky bits of implementing this be solved if we were to introduce a new (internal to the compiler only) feature to rewrite the expression tree in the parsing phase? For instance, only for productions that have been tagged (with an attribute perhaps) to be rewritable?

In the case of |>, this would mean that if, and only if, it is used as an operator (as opposed to usage as a function with (|>)), and the production rules match the signature, then the production is rewritten as a function call on the rh operand instead and the pipe operator disappears.

This would change the resulting opcodes, and obviously the intermediate AST, but since currently that already happens at the end during the inlining phase, I doubt this causes many issues in practice.

This will also fix the issue that piping breaks tail recursion.

If this is feasible, we might find other operators that can be optimized this way (like ||> or >>).

It might change the debugging experience, but since inlining already makes it nearly impossible to debug |>, we might as well live with it.

Rewriting the AST would not take place when the operator is redefined by the user.

@dsyme
Copy link
Collaborator

dsyme commented Nov 7, 2019

@abelbraaksma That kind of rewrite is more or less how the current F# compiler optimization phases work.

You can definitely make another dedicated lowering phase work, but it becomes very hard to control/test/document/explain/answer-questions -on-stackoverflow about the exact subset of constructs supported, people end up relying on corner cases that were never intended to work, and keep asking for more things to be supported ("can we inline Array.map into flattened code" etc.)

@7sharp9
Copy link
Member

7sharp9 commented Nov 7, 2019 via email

@TIHan
Copy link

TIHan commented Nov 7, 2019

I've thought about this for a while. I think we should refrain from allowing byref type arguments for inline functions because it introduces more complexity to the language that a user has to learn about.

When calling functions today, we don't really care if it is inline or not; we just call it. If we allowed this feature, you now have special semantics at the call site for the function in regards to it being marked as inline. Now, we do have SRTP which require inline functions, but the user calling the function is expected to understand the constraints rather than understanding that the function is inline.

I do agree that using |> would be useful for Span. However, there are only a limited amount of use cases for Span to the point where you should not be constantly using it.

@GratianPlume
Copy link

I get an idea: F# can use proxy types to pass IL checks, reorganize at target locations (non-inline functions), and erase intermediate proxy types during F# optimization. I did something similar last week.

   type DataCell<'T1, 'T2> = 
      val mutable Item1: 'T1
      val mutable Item2: 'T2
      new (value1, value2) = 
          { Item1 = value1
            Item2 = value2 }


      type Cite<'T, 'R> =
         | Cite of Content:'T * Read: ('T -> 'R) * Write: ('T * 'R -> unit)
         
         member inline __.Value with get() = match __ with | Cite(x, f, _) -> f x
                                 and set v = match __ with | Cite(x, _, f) -> f(x, v)

      let inline (|Cell|) (x: DataCell<'T1, 'T2>) = 
               let x1 = Cite(x, (fun x -> x.Item1), (fun(x, v) -> x.Item1 <- v))
               let x2 = Cite(x, (fun x -> x.Item2), (fun(x, v) -> x.Item2 <- v))
               (x1, x2)


      let foo = function
         | Cell(a, b) -> 
            a.Value <-   b.Value + 1

F# can optimize it to let foo (x: DataCell<'T1, 'T2>) = x.Item1 <- x.Item1 + 1
This is done to pass byref<'T>. For Span<'T>, there are many limitations to doing it at the user level. It should be much more convenient to do it by the compiler, and it can be done more perfect, such as ensuring that the intermediate type is erased, and the unerased intermediate type will cause Warning, and use smooth conversion, etc.

@roboz0r
Copy link

roboz0r commented May 7, 2021

Would it be possible to create another type of error e.g. OptimisableError, which signifies a piece of code has produced an error at a certain phase of compilation which may be eliminated by subsequent compilation steps?

The idea would be that only the call tree around the OptimisableError would be run through the full compiler rather than the entire project to attempt to resolve the error before reporting to the IDE.

@matthewcrews
Copy link

I don't know if this will ever become feasible but I have run into this limitation quite a bit in my most recent work. This would be incredibly valuable in high-performance scenarios.

@abelbraaksma
Copy link
Member

@dsyme, I just noticed you removed approved-in-principle on June 16, was that by accident? Just today I had a long discussion with @matthewcrews on Slack about the hoops he needs to jump through to use byrefs in certain high performance scenarios. He seems willing to spend time on this, but it’d be a much larger incentive if the tag was still there :).

@JustinWickMS
Copy link

@dsyme:

Seconded on the need for this, I'm trying to cleanup some concurrent data structure commit semantics and allowing spans to be stored in a struct that represents a transaction would be a big improvement for situations where one cares about GC pauses. Maybe there's a restricted-but-useful subset of scenarios where it wouldn't be hard to add this?

@Happypig375
Copy link
Contributor

Happypig375 commented Aug 1, 2022

image

image

@matthewcrews
Copy link

matthewcrews commented Aug 1, 2022

To clarify, I am not trying to push F# in a direction it is not intended to go. If this runs contrary to the design goals of F#, please disregard it. These are messages that were shared on Discord discussing a different .NET language.

I sense I'm in a unique position where my company wants F# but wants it to run as fast as possible. These are the items that are causing me the most grief. Not saying F# needs to change for me, obviously 😊.

It's fine if this will never happen. Having that information will inform my design decisions down the road.

@dsyme
Copy link
Collaborator

dsyme commented Aug 1, 2022

This isn't necessarily a totally bad idea, but it brings "inline" to be closer to macro expansion, which means, for example, that these specific inline functions must actually be applied to arguments.

The current design goals for "inline" are

  1. support code that generalizes over trait constraints
  2. allow users to write more performant code that allows more optimizations to kick in, without any deep guarantees about what flattening does and doesn't occur.
  3. allow dynamic invocations to inline functions, e.g. when evaluating quotations or other interpretation of F# code
  4. do not require the actual expansion or flattening of code during the checking phase (the code is expanded during optimization).

Since F# 6 there is also this goal:

  1. Allow expansion of resumable code under slightly different rules.

This would add a 6th goal:

  1. Allow code where the checking phase checks that it is sufficiently flattened to allow the use of "stack restricted" constructs such as byrefs, reraise, Span, CallerName, stackalloc and so on.

Note that nothing in 1-4 is about stack-bound constructs. Resumable code is related to stack-bound constructs but also has other considerations.

Anyway, goal (6) would certainly conflict with goals (3) and (4) above. This is not necessarily a bad thing, but yes it does need to be very carefully thought through. For example it might need an attribute to declare if an inlined function uses stack-bound constructs - this would then require that the inlined function is actually applied to arguments, and would mean no dynamic implementation would be available.

@matthewcrews
Copy link

As much as it pains me, if the compiler cannot provide guarantees around inlining behavior, I don't think this would be a good idea. If this is not the design intention of inline, then it's probably not a good idea from a developer's experience.

If inlining is a "maybe," then it's possible that a developer could write code that, in theory, can be compiled but then later fails. This is incredibly confusing. I've already run into this. F# compiled and thought everything was good to only have the IL fail when the CLR processed it. The only reason I figured it out was with the help of someone who took the IL and re-arranged it to work.

I've also run into issues with inline when it needs to call an internal or private member. All the IDEs report the code as good, but when it goes to compile, the F# compiler reports an error saying the inline function is using a method without sufficient visibility.

I realize those errors are coming from different places. My point is that when a developer is working in VS Code/VS/Rider and none of them are reporting an error, you expect your program to be able to be compiled and run. Adding this increases the probability of a "maybe an error? maybe not? 🤷"

Don't get me wrong. I still want to be able to use ByRefLike types in lambdas, but this is starting to sound like feature abuse where I'm asking to take inline in a direction it wasn't intended, and that makes me a little nervous.

I'm not sure what the synthesis of these concepts would be, but if the solution makes it possible to have these really obtuse errors, I would suggest against it for the sake of the new developer.

I would rather increase the guarantees around inlining behavior first so we know what will and will not be inlined.

@dsyme
Copy link
Collaborator

dsyme commented Apr 12, 2023

I'm closing this based on the discussion above

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