-
Notifications
You must be signed in to change notification settings - Fork 798
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
Inline Option module #14927
Inline Option module #14927
Conversation
For what it's worth, the size of FSharp.Compiler.Service.dll decreased from 18,335,232 to 18,252,288 bytes. Can't speak to the performance of the compiler, but I doubt the impact will be measurable. |
Just thinking out loud here - option is much more widely used than many others, so I guess, one of the downsides of inlining things like Wondering if it might become a problem in big codebases with use a lot of options everywhere. @EgorBo how does JIT determine those thresholds for inlining/optimizing now? |
That is actually quite interesting |
That's what I mean in the final 2 paragraphs.
That's also why making it as fast as possible will be noticed the most.
I expect this might be an issue in very special scenarios when you absolutely require JIT inlining and specific optimizations. If that's the case, the solution is simply to move the guts of the lambda into a function. In the vast majority of cases (shameless guess :)) we'd just be leaving a bit of performance on the board without
I reckon it would be the closures and extra functions that are no longer needed as the bodies are inlined. |
Yeah, it seems I've failed to read them properly, sorry.
Yeah, just wondering how often actually this would be the case. |
So very large methods have several limitattions:
So overall it's hard to say/predict anything, but in general very large methods aren't good for JIT and its Register Allocator. |
Looks great - My intuition is it's fine to add both @vzarytovskii Code size shouldn't be a problem as the size of a closure will usually be bigger. My understanding is that adding |
The trimming test also reports a smaller code size with this PR, although nothing dramatical. Expected: |
The overall code size is not important in this case, but rather each individual method size matters. |
(CI will fail at first, then expected size of trimmed app has to be updated one more time) |
* Inline Option module * Do not inline Option.get * Update expected trimmed size * Update check.ps1 --------- Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Implements a part of https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1115-InlineIfLambda-in-FSharp-Core.md.
Benchmark
I've decided to benchmark
map
anddefaultWith
, as other functions are structurally equivalent for the most part (withmap2
andmap3
being ever so slightly more complex) and should have nearly identical performance profiles.Code
Decompiled
Analysis
inline
has the largest performance impact, and addingInlineIfLambda
improves on it only marginally. This stems from the fact that the compiler doesn't need the attribute to avoid both a virtual call and allocating a closure with captured variables, although this might not be the case in all circumstances.Compare these 3 methods:
Decompiling to:
Inlining
Option.defaultWith
adds around 6 IL instructions at the call site, with the number increasing a little bit for more complex functions likeOption.map3
.InlineIfLambda
will naturally increase the function size further by whatever amount of instruction the lambda contains. This might in theory cause a cascade of inlinability changes, where some functions might swell past the limit the F# compiler and JIT consider automatically inlinable, or the F# compiler might split the function in 2.Nonetheless, I believe adding
inline
to all of the function would be a clear net win.InlineIfLambda
will generally improve performance a little bit more still, but we could conceive of edge cases where this would cause regressions, specifically with respect to JIT and what functions it considers eligible for inlining and optimizing. I'm leaning towards adding the attribute too, but don't mind removing it.