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

Add custom dll resolver event #156

Merged
merged 1 commit into from
Dec 28, 2020

Conversation

Sergio0694
Copy link
Contributor

This PR includes a public event to allow consumers to inject their own DLL resolvers.

Diff for the API surface:

namespace TerraFX.Interop
{
    public static class Windows
    {
+        public static event DllImportResolver? ResolveLibrary;
    }
}

Just like in the TerraFX.Interop.Vulkan version, this one first tries to resolve libraries using the event, and then just does a normal NativeLibrary.TryLoad call as a fallback, if there are no external resolvers or if they all fail. Consumers not using this feature should observe no change in behavior when using TerraFX.Interop.Windows.

Just adding these details here for future reference, I know you know all this already 😄

/// <summary>Raised whenever a native library is loaded by TerraFX.Interop.Windows. Handlers can be added to this event to customize how libraries are loaded, and they will be used first whenever a new native library is being resolved.</summary>
public static event DllImportResolver? ResolveLibrary;

static Windows()
Copy link
Contributor

Choose a reason for hiding this comment

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

Introducing a static ctor has potential perf impacts. Consider using a method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought, though TerraFX.Interop.Vulkan does this too 🤔

I fixed this in dce29aa with a trick that is... A bit ugly, but should do the job 😄

Copy link
Member

Choose a reason for hiding this comment

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

You could use a module initializer instead and that's basically going to be the same thing.

I'd also rather just have the static constructor than the hack. It's a one time cost and is going to result in better codegen for AOT targets.
The import resolver is also going to, realistically, be run regardless of what you are doing as it is needed for any method invocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow, would you rather I did the module initializer, or just reverted the last commit?
I thought having a static constructor could potentially hurt the codegen when accessing all the other static fields in the class, which are a ton? Like, all the various GUIDs and whatnot. Also I'm confused as to why having a static constructor is better for AOT scenarios, could you elaborate on that? I'd love to learn more 😄

It was my understanding that beforefieldinit was strictly a superset of just having a static constructor, as in, with fields being allowed to be initialized up to when they're used, or before, rather than always just as soon as any member of the class is accessed at all. So doesn't that just give more flexibility to compilers, which could technically still just behave the same as when using a static constructor, without going against the spec? I don't understand how having more flexibility there could actually make things not just the same, but worse 🤔

Copy link
Member

Choose a reason for hiding this comment

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

  • If marked BeforeFieldInit then the type’s initializer method is executed at, or sometime before, first access to any static field defined for that type.
  • If not marked BeforeFieldInit then that type’s initializer method is executed at (i.e., is triggered by):
    • first access to any static field of that type, or
    • first invocation of any static method of that type, or
    • first invocation of any instance or virtual method of that type if it is a value type or
    • first invocation of any constructor for that type

In practice (on RyuJIT), this means that BeforeFieldInit inserts a check that is basically:

if (!type.HasStaticConstructorRun)
{
    type.cctor();
    type.HasStaticConstructorRun = true;
}
// access field

Given that this is normally in a JIT environment and the methods are effectively jitted on first call, this means that the JIT can opt to not emit this logic if it knows the static constructor has already run. Likewise, with rejit, it can remove said code when the method is rejitted if it had to emit it previously. In an AOT environment, all methods are compiled up front and there is no rejit, so every static field access for a BeforeFieldInit type has this check and it won't go away.

  • While the spec allows it to run earlier, this was made deterministic in .NET Core to simplify code and make things more reliable

Now, due to the DllImportResolver impacting resolution of static methods, it actually has to run not just before any static field access but before accessing any static method which is only possible via an explicit static constructor. However, C# 9 introduced module initializers which basically allows a set of functions to run on "module load" (which is effectively assembly load for most libraries) which can avoid some of the cost associated with having an explicit static constructor that @john-h-k referred to.

So, the best approach is likely to switch this to be a module initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thank you for the detailed answer! 😄

Fixed in 15732ee using [ModuleInitializer] 🚀

@Sergio0694 Sergio0694 force-pushed the feature/custom-dll-resolver branch from 15732ee to 3e01b15 Compare December 28, 2020 20:57
@tannergooding tannergooding merged commit 5e78805 into terrafx:main Dec 28, 2020
@Sergio0694 Sergio0694 deleted the feature/custom-dll-resolver branch December 28, 2020 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants