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

Attribute to prevent fsc inlining but allow JIT inlining #838

Closed
5 tasks done
NinoFloris opened this issue Feb 12, 2020 · 22 comments
Closed
5 tasks done

Attribute to prevent fsc inlining but allow JIT inlining #838

NinoFloris opened this issue Feb 12, 2020 · 22 comments

Comments

@NinoFloris
Copy link

NinoFloris commented Feb 12, 2020

I don't think this ever became a suggestion
dotnet/fsharp#5178

@dsyme called it a reasonable request
dotnet/fsharp#5178 (comment)

Pros and Cons

The advantages of making this adjustment to F# are:
Separate JIT and fsc inlining directives better

The disadvantages of making this adjustment to F# are:
Nothing substantial

Extra information

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

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
@zpodlovics
Copy link

It seems that I am no longer alone with the F# performance feature requests! While I have limited time, I am happy to help.

@abelbraaksma
Copy link
Member

When you say "prevent fsc inlining", do you mean explicit inline through inline, or implicit inline, like in let add x y = x + y?

But in either case, these are source inlinings, and preventing these could lead to different signatures, and automatic overloads not overloading anymore (ie, the functions would be fixed).

Assuming it's something else, what kind of inlinings are you talking about here? Not JIT inlining, so there must be something else. Do you have an example?

@buybackoff
Copy link

@abelbraaksma
Copy link
Member

abelbraaksma commented Sep 26, 2020

@buybackoff, I don't follow, as the function here is marked inline. Can you elaborate how this would help? Btw, we already have a NoInlining attribute value.

    let inline private asNode(value:MapTree<'Key,'Value>) : MapTreeNode<'Key,'Value> =
        // F# is "too smart" and eliminates the inlined IL call, but that should be left to JIT, otherwise stuff breaks
        // (# "ret" value: MapTreeNode<'Key,'Value> #)
        // Ideally we need ldarg.0;ret without S.R.CS.U dependency
        // :?> also works, but it's not free, while the usage guarantees correct unsafe casts
        // when this is implemented, S.R.CS.U could be removed for inline IL: https://github.com/fsharp/fslang-suggestions/issues/838 
        // Unsafe.As<MapTreeNode<'Key,'Value>>(value)
        value :?> MapTreeNode<'Key,'Value> // this is not visible for performance

@buybackoff
Copy link

@abelbraaksma it's market as inline for the implementation as is.

If we were to use (# "ret" value: MapTreeNode<'Key,'Value> #) this method should not be inlined by F# and left for JIT, ideally with AggressiveInlining attribute. So that in the end we get exactly the same method as S.R.CS.Unsafe.

Currently, F# inlines a method with only (# "ret" value: MapTreeNode<'Key,'Value> #) and then in the Map case things break.

For JIT such a method with ldarg.0;ret will be noop, while normal cast has small cost.

@abelbraaksma
Copy link
Member

@buybackoff Sounds to me that instead the inline algorithm needs fixing, if code can break. It's probably due to the inline IL syntax, which iirc, gets treated differently in the inline functions compared to normal source code.

Interesting that both versions get inlined, but one causes failed execution. Perhaps it's not the inlining that goes bersirk, but the optimization algorithm, which eliminates redundant IL.

@abelbraaksma
Copy link
Member

Btw, do I understand you correctly that if you remove inline it still fails, or the function cannot be marked without inline and still function (in which case I understand the request that you want to not inline when inline is present).

@buybackoff
Copy link

@abelbraaksma

Perhaps it's not the inlining that goes bersirk, but the optimization algorithm, which eliminates redundant IL.

I think this is the case, but in the context of this issue is an example when a new F# attribute is useful.

If I add NoInline to an implementation with (# "ret" ... #) it works fine

@buybackoff
Copy link

Btw, do I understand you correctly that if you remove inline it still fails,

In that case F# still inlines it, because it's small.

A workaround like this would prevent inlining when it's actually useful.

@abelbraaksma
Copy link
Member

So, I gather that [<MethodImpl(MethodImplOptions.NoInlining)>] is not an option either, because you do want the JIT to inline. Just no source inlining.

@buybackoff
Copy link

So, I gather that [<MethodImpl(MethodImplOptions.NoInlining)>] is not an option either, because you do want the JIT to inline. Just no source inlining.

Exactly

@zpodlovics
Copy link

Please note, the new CoreCLR have profile guided optimization as of dotnet/runtime#42277. The JIT can make decisions based on the actual execution profile. For example a small tight code that called in a loop - especially when the generated code fits in the instruction decoding cache - could be really fast, could be lot faster than than the flattened code without loop (dotnet/fsharp#5178 (comment)) .

@buybackoff
Copy link

This maybe better placed there, but logically this is grouped here for high performance IL/JIT tweaks.

Currently, F# adds .tail IL instruction before every function call. I'm not totally sure that is the case, but it looks like a method that calls another method with a tail prefix cannot be inlined according to this.

E.g. in some pseudo-language

[AggressiveInlining] ** [NoTailCallsInside] - a new attribute **
int Compare<T>(x:T,y:T)
{
     if(typeof(T)==typeof(int))
           return ((int)(obj)(x)).CompareTo((int)(obj)(y))
     if ... other types ...
           ....
     else
         // .tail here is added by F#, which prevents inlining of Compare above
         return FallbackCompareNotInlined(x,y)
}

A new attribute such as NoTailCallsInside above would be helpful.

You could see what performance gains are possible for primitives in this Map and Set PRs. But even for them, I had to use struct ctor instead of a method, because F# does not add .tail to struct ctors.

In that case, performance for reference types regressed probably due to additional non-inlined wrapper call. If this theory is right then we could get huge performance increase for known types and keep the performance for other types exactly the same.

Together with the attribute from this issue it will let to control IL and inlining behavior for hot paths much better. I could open a separate suggestion, but IMO they should go together. (Hopefully as a part of .NET 6 🙏)

@AndyAyersMS Could you, please, confirm that .tail before Fallback... prevents inlining of Compare in the sample above? And for non-special-case types it becomes a wrapper method that just calls another method, never inlined.

@AndyAyersMS
Copy link

Could you, please, confirm that .tail ... prevents inlining ...

Yes. There are two aspects of this:

  1. the jit will not inline at any call site with a .tail prefix,
  2. the jit will not inline any method containing a call with an explicit .tail prefix.

The reason for the restrictions above is that proper handling of .tail sometimes requires using a "slow" helper, and the jit is currently not able to reason about whether inlining in either of the cases above would convert "fast" tail calls into "slow" ones.

We've recently made the helpers faster and more uniform across all ABIs and ISAs, and we may be able to revisit this policy in the future. See for example dotnet/runtime#12304

@buybackoff
Copy link

Thank you for insight, @AndyAyersMS!

I did not notice the issue dotnet/fsharp#6329 before, my comment is a dupe of it.

@dsyme dsyme changed the title Attribute to prevent fsc inlining Attribute to prevent fsc inlining but allow JIT inlining Jun 15, 2022
@dsyme
Copy link
Collaborator

dsyme commented Jun 15, 2022

Marking as approved-in-principle. It would be great to have this as a community-led contribution

@kerams
Copy link

kerams commented Nov 7, 2022

Completed.

@vzarytovskii
Copy link

This will need a small RFC describing the theory how it works. For any future references.

@abelbraaksma
Copy link
Member

abelbraaksma commented Nov 7, 2022

@kerams can we also document here and in the rfc with a small example what the attribute is and how it works?

@kerams
Copy link

kerams commented Nov 7, 2022

Here, hope it's sufficient.

@abelbraaksma, it's really simple. Slap the attribute on any value, function or method and it won't be inlined in IL.

@abelbraaksma
Copy link
Member

abelbraaksma commented Nov 7, 2022

Perfect! Thanks. Awesome work on the PR, btw! I was curious about this in particular, which you answered there:

Using both inline and NoCompilerInliningAttribute should result in a compilation error.

@abelbraaksma
Copy link
Member

abelbraaksma commented May 25, 2024

Not sure why this was never closed. This is now part of F# 7.0 and up.

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