Skip to content

Conversation

teo-tsirpanis
Copy link
Contributor

This PR updates the bindings to the hostpolicy library to use function pointers for the callback parameters. This enables us to use UnmanagedCallersOnly for the callback functions in AssemblyDependencyResolver.

Because the callbacks in the hostpolicy APIs do not take an opaque void* parameter, we use a thread-local variable to store the callbacks' state. This is safe to do because:

  1. The callback passed to corehost_set_error_writer is documented to be per-thread.
  2. The callback passed to corehost_resolve_component_dependencies gets called only once at the end.

If you prefer to add new hostpolicy APIs, let me know.

@Copilot Copilot AI review requested due to automatic review settings August 23, 2025 17:38
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 23, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 23, 2025
@teo-tsirpanis teo-tsirpanis added area-Host and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 23, 2025
Copilot

This comment was marked as outdated.

We now have function pointers and can write this with less magic.
@teo-tsirpanis teo-tsirpanis requested a review from Copilot August 23, 2025 20:36
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modernizes the bindings to the hostpolicy library by replacing delegate-based callbacks with function pointers and UnmanagedCallersOnly methods. The change improves performance by eliminating delegate allocations and marshalling overhead while maintaining thread safety through thread-local storage for callback state.

Key changes:

  • Replaced delegates with function pointers in hostpolicy interop bindings
  • Implemented UnmanagedCallersOnly callback methods in AssemblyDependencyResolver
  • Added thread-local state management to safely store callback data without opaque parameters

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/libraries/Common/src/Interop/Interop.HostPolicy.cs Updated interop bindings to use function pointers instead of delegates
src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyDependencyResolver.cs Replaced delegate callbacks with UnmanagedCallersOnly methods and added thread-local state management
src/tests/Common/CoreCLRTestLibrary/HostPolicyMock.cs Updated mock implementation to use function pointers matching the new interop API

@vitek-karas
Copy link
Member

Curious what's the main motivation for this change? The description provides the reasoning of how... but not why.

@jkotas
Copy link
Member

jkotas commented Aug 27, 2025

Curious what's the main motivation for this change?

Our interop best practices say to prefer function pointers over marshalled delegates. This is one of the last few remaining instances of marshalled delegates in dotnet/runtime libraries. We got rid of them from as many places as possible.

  • Marshalled delegates have worse startup perf compared to UCO (marshalling IL stub requires runtime IL generation and JITing).
  • Marshalled delegates have worse throughput compared to UCO.
  • Marshalled delegates are bug prone. The code that uses them has to be very careful to keep the delegate alive as long as the marshalled pointer is used.
  • Marshalled delegates are not supported in locked down environments (Wasm, locked down Windows, SELinux). This point is not applicable here since this API is unlikely to be used in those environments.

@vitek-karas
Copy link
Member

The lack of context pointer is not ideal in the native API (sorry, my bad). If we could find a way to fix that, I would prefer that over the reliance on thread locals (although I agree it will work). But if it becomes too complicated I think it's OK to keep it as is.

@teo-tsirpanis
Copy link
Contributor Author

Opened #119139; I can do it in a subsequent PR.

@elinor-fung
Copy link
Member

Thank,s @teo-tsirpanis.

Opened #119139; I can do it in a subsequent PR.

Are you still planning on doing the work for adding the callback context? (Would be much appreciated) If so, I'd prefer we actually get that in first and then update this PR to use them (instead of introducing the thread locals here first, then adding the new APIs and then switching AssemblyDependencyResolver to them) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Host community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants