Implement direct delegates#19993
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
|
|
||
| module DirectDelegates = | ||
|
|
||
| let private coreOptions compilation = |
There was a problem hiding this comment.
Could you pls also add a call to the runtime's IL verifier, to make sure we produce valid IL ?
Example here, there are more helpers for it.
| @@ -0,0 +1,417 @@ | |||
|
|
|||
There was a problem hiding this comment.
Unless there are valid , by-design (or by test-case) reasons for deviations across optimize/realsig switches, it is better to have a single .bsl file covering more runs.
That way the tests hold equal codegen for both versions together, and guarantee it cannot deviate.
(the test framework allows for that - if e.g. .RealInternalSignature{b}. part is missing, it takes the common name.
There was a problem hiding this comment.
There are deviations depending on language version and optimizations. Therefore we have 4 snapshots of everything and can prove the feature is on under specific circumstances only. I suppose I can remove the signature switching though.
There was a problem hiding this comment.
But honestly, the language feature clearly gates this, so if you think there is no point asserting the non-preview behavior, I'll happily remove half of the baselines.
| not cenv.options.localOptimizationsEnabled | ||
| && tmvs |> List.exists (fun (v: Val) -> not v.IsCompilerGenerated) | ||
| then | ||
| // Eta-expanded delegate in an unoptimized build: the Invoke parameters are the user's own lambda | ||
| // variables, so keep the closure to preserve their names for debugging. Non-eta delegates (whose | ||
| // parameters are synthesized by BuildNewDelegateExpr) are always made direct; eta-expanded ones | ||
| // are made direct only in optimized builds, where debuggability is not a concern and the | ||
| // forwarding call survives to here. | ||
| None | ||
| else |
There was a problem hiding this comment.
If we are sure we are not losing any sequence points for any forms of lambdas, maybe the new stepping test helpers from @auduchinok could be used to guard that behavior.
There was a problem hiding this comment.
Yes, None here causes a fallback to existing behavior, completely unchanged.
| hasWitnesses | ||
| || newobj | ||
| || isSuperInit | ||
| || isSelfInit | ||
| || isBaseCall | ||
| || not (receiverShapeOk takesInstanceArg) | ||
| || receiverNotBindable () |
There was a problem hiding this comment.
I would like if this block of code has better explanation and maybe even encapsulation if possible (similar one is also for ILMethod ). To make sure future extensions of runtime/language know when to extend this block.
|
|
||
| open System | ||
|
|
||
| // Section D, bullet 1: the optimizer inlines small function bodies before IlxGen runs. If a delegate |
There was a problem hiding this comment.
Another dimension: Inlining across F# assembly boundaries
| // 3. eta-expanded known target, tupled application, same compiled representation | ||
| let case3_etaTupled () = Action<int, int>(fun a b -> handlerTupled (a, b)) | ||
|
|
||
| // 15. non-eta-expanded known target via partial application |
There was a problem hiding this comment.
I'll make sure the numbering is unified with the RFC cases
|
|
||
| open System | ||
|
|
||
| // Section D, bullet 1: the optimizer inlines small function bodies before IlxGen runs. If a delegate |
|
|
||
| open System | ||
|
|
||
| // Section D, bullet 2: cases 15/16 (in DelegateKnownFunction.fs / DelegateStaticMethod.fs) capture a |
There was a problem hiding this comment.
Clean the development-time pointers/indices please.
| |> coreOptions | ||
| |> withLangVersionPreview | ||
| |> withPreviewBaseline | ||
| |> compile |
There was a problem hiding this comment.
At least a representative for each positive case should be also running, not just compile.
| |> coreOptions | ||
| |> compile | ||
| |> shouldSucceed | ||
| |> verifyILBaseline |
There was a problem hiding this comment.
One more thought - for the positive scenarios, can we be more specific and instead of .bsl do a pair of "contains" (function pointer load) and "not contains" (no closure allocation).
There was a problem hiding this comment.
ldftn/ldvirtftn are present in both cases, along with putting arguments on the stack. The latter also requires a dup instruction. If your goal is to minimize potential future baseline churn, I'm not sure this would help.
There was a problem hiding this comment.
Yep, the goal would to minimize the chance of an unrelated change affecting the .bsl.
But the code samples are well isolated, so its not a big deal.
|
Ready for another pass, think I addressed everything. (But alternatives and unresolved questions in the RFC need answers still:)) |
I think this is not needed anymore, Tomas has added a msbuild way as well as compiler flag to disable specific language features:
or <ItemGroup>
<DisabledLanguageFeatures Include="DirectDelegateConstruction" />
</ItemGroup>You may want to check it picks up as is to allow selectively disabling the feature. |
| <Compile Include="EmittedIL\CompiledNameAttribute\CompiledNameAttribute.fs" /> | ||
| <Compile Include="EmittedIL\ComputationExpressions\ComputationExpressions.fs" /> | ||
| <Compile Include="EmittedIL\ComputedCollections\ComputedCollections.fs" /> | ||
| <Compile Include="EmittedIL\DirectDelegates\DirectDelegates.fs" /> |
There was a problem hiding this comment.
Since the use cases are known in the RFC and often framework-integrated, it would be great to have some smoke tests for this integration.
(ASP.NET routing/minimal APIs, dependency-injection containers, mocking libraries, serializers, and logging/diagnostics that print the bound method name. With F# they see Invoke on a closure rather than the intended method, which breaks or degrades these scenarios..)
Either going via "Regression tests" suite and referencing a separate repo and building it with the fresh compiler.
Or picking a few cases and doing an .fsx test with #load of a few chosen libraries?
| && staticLeadingArgIsRefType g takesInstanceArg leadingArgs | ||
| && receiverNotByref g leadingArgs | ||
|
|
||
| /// Confirm the target's IL signature is compatible with the delegate's Invoke. The type checker has already |
There was a problem hiding this comment.
Can you pls globally invoke the https://github.com/dotnet/fsharp/blob/main/.github/instructions/NoBloat.instructions.md instruction to shrink the number and verbosity of code comments added?
| None | ||
| else | ||
| match classifyForwardingTarget (Optimizer.ExprHasEffect Optimizer.EffectContext.Emit) g tmvs body with | ||
| | DirectDelegateForwardingTargetCandidate.FSharpVal(vref, valUseFlags, tyargs, leadingArgs) -> |
There was a problem hiding this comment.
So we are now replicating the direct delegate in IlxGen as well as Optimizer, and that prevents the inline race? Or am I reading it wrong?
There was a problem hiding this comment.
The recognizer (the piece of code that decides whether direct delegates can be used) runs in both places, but in the optimizer the only thing it does is suppress inlining.
Description
Implements fsharp/fslang-suggestions#1083, fixes #11898. RFC.
Test cases should cover everything mentioned in the language suggestion.
Checklist