Use function pointers for native TaskDialog and Enum*Windows callbacks

[kpreisser]

Issue: #4135

Proposed changes

• Use function pointers instead of delegates in the TaskDialog and EnumWindows/EnumThreadWindows/EnumChildWindows callbacks, to reduce interop overhead and simplify the code.

For native callbacks that allow to pass user-defined reference data, we can use function pointers of static methods in order to simplify the code and improve performance, as described in Improvements in native code interop in .NET 5.0:

The most obvious change is that the allocation of a delegate is no longer needed. By requiring that the function only have blittable arguments, the runtime does not need to do any marshalling, so the only requirement for entering the function is a GC transition to cooperative mode. The restriction of not allowing the function to be called from managed code means that the JIT-ed function itself can do the GC transition. The function pointer for Callback above actually points directly to the JIT-ed function. The extra error-prone code for keeping the delegate alive is no longer needed either.

The callbacks for Enum*Windows() were already changed to static methods with #2639 (using a GCHandle to pass the actual instance as IntPtr), so we can also use a function pointer for them.

However, in my testing with EnumWindows (that calls the callback about 200 times) I did not observe any noticable difference in performance before/after switching to function pointers.

Customer Impact

• None

Regression?

• No

Risk

Test methodology

• Calling the native functions where a callback is passsed to (TaskDialogIndirect, EnumWindows, EnumThreadWindows, EnumChildWindows) using x86 and x64 architectures and verified that the callbacks are called correctly.

Test environment(s)

.NET SDK (reflecting any global.json):
Version: 6.0.100-alpha.1.20472.11
Commit: e55929c5a5

Runtime Environment:
OS Name: Windows
OS Version: 10.0.18363
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\6.0.100-alpha.1.20472.11\

Host (useful for support):
Version: 6.0.0-alpha.1.20468.7
Commit: a820ca1c4f

.NET SDKs installed:
5.0.100-rc.1.20452.10 [C:\Program Files\dotnet\sdk]
6.0.100-alpha.1.20472.11 [C:\Program Files\dotnet\sdk]

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

Merging #4003 (875f1ba) into master (36f3feb) will decrease coverage by 0.00773%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##              master       #4003         +/-   ##
===================================================
- Coverage   97.96958%   97.96185%   -0.00773%     
===================================================
  Files            499         499                 
  Lines         259010      259010                 
  Branches        4569        4569                 
===================================================
- Hits          253751      253731         -20     
- Misses          4457        4473         +16     
- Partials         802         806          +4     

Flag	Coverage Δ
Debug	97.96185% <ø> (-0.00773%)	⬇️
production	100.00000% <ø> (ø)
test	97.96185% <ø> (-0.00773%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[RussKie]

@AaronRobinsonMSFT do you think you could review this change? Thanks

[RussKie]

👍

[weltkante]

Maybe wait until this is fixed in the compiler before merging this.

I agree, this may be littering the code a bit with assumptions that are not guaranteed to be implemented in the way they are expected to. Unless there's significant gain (which it doesn't seem to have) I'd wait until the feature is completely supported by Roslyn before making use of it.

Generally speaking, once working properly, function pointers are certainly something that can and should be made use of on a wider scale in the WinForms codebase, probably something for .NET 6.

[AaronRobinsonMSFT]

@RussKie Thanks for adding me here. Support for function pointers and UnmanagedCallersOnly has a lot of value in some scenarios but I am not convinced this is one of them.

My main concern would be the unfortunate need to cast and then recast - it is ugly and is simply debt to fix post-GA. The WinForms code base has already paid the cost of handling delegate lifetimes so there isn't much win there. Another optimization win for the function pointers is reducing delegate allocation - which is hard to observe in practice and isn't a concern in this code path. A note for future is that this approach will at present only be beneficial in non-x86 scenarios (e.g. x86_64) - see dotnet/runtime#33582.

In my opinion I would defer this work until GA is out with official support for C# function pointers and UnmanagedCallersOnly.

[kpreisser]

Thank you for the feedback! I agree we should wait until GA is out and the UnmanagedCallersOnly attribute is fully supported (so the casts should no longer be necessary). I'm therefore marking the PR as draft in the meanwhile.

[kpreisser]

This seems to be fixed with SDK 5.0.100-rc.2.20479.15, the compiler now correctly recognizes the attribute and the casts are no longer needed.

[RussKie]

Thank you and apologies for the delays.