Support linking Vulkan directly

[Ralith]

This is the preferred pattern in most environments when an application cannot function without Vulkan, as it saves the libloading dependency, eliminates an error case, and makes the Vulkan dependency visible to the OS.

Ideally I'd like to move towards this being the default, but that might be more disruptive than is justified.

Draft until I've tested this properly on Windows. The build script might need to explicitly add VULKAN_SDK/lib to the linker search path.

[MarijnS95]

Nice, I've been intending to propose the same after 0.33 is out assuming we can get permission to absorb detection and static linking of MoltenVK through the same mechanism, effectively making ash-molten obsolete.

I'm still conflicted on the linked name, but guess it makes sense if we were to support both dynamic and static linking at some point in time.

[Friz64]

Please be aware that the current implementation (indirect linking) is recommended in order to achieve optimal performance as per the Vulkan Loader documentation.

[MarijnS95]

@Friz64 You probably intended to link https://github.com/KhronosGroup/Vulkan-Loader/blob/master/loader/LoaderAndLayerInterface.md#indirectly-linking-to-the-loader as https://github.com/KhronosGroup/Vulkan-Loader/blob/master/loader/LoaderAndLayerInterface.md#best-application-performance-setup is specific to linking Vulkan functions after having acquired vkGetInstanceProcAddr/vkGetDeviceProcAddr. This PR only concerns itself with dynamic/runtime loading of the vkGetInstanceProcAddr entrypoint, which should theoretically be faster if performed by the library loader instead of having to jump through extra hoops after application startup to call dlopen + dlsym (though this is what ld.so performs under the hood...).

The section you linked seems to recommend using vkGetDeviceProcAddr to retrieve device/driver specific functions instead of vkGetInstanceProcAddr which would return a generic function with extra table/indirection to dispatch the call to the right driver based on the device being passed around. This is also why many device-extensions implemented in src/ash/extensions call get_device_proc_addr instead of get_instance_proc_addr, to make the resulting pointers ICD-specific and hence more performant.

[Friz64]

Yes, i intended to link that one instead. The documentation is somewhat confusing in this regard, but i understand now.

[Ralith]

I'm still conflicted on the linked name, but guess it makes sense if we were to support both dynamic and static linking at some point in time.

None of this code is sensitive to whether the linked library is static or dynamic; it's just a matter of what's in your search path.

[MarijnS95]

I'm still conflicted on the linked name, but guess it makes sense if we were to support both dynamic and static linking at some point in time.

None of this code is sensitive to whether the linked library is static or dynamic; it's just a matter of what's in your search path.

It feels like there should be something to distinguish it from runtime linking/loading, perhaps renaming libloading because that's rather vague if unfamiliar with the particular crate name (and anyway the only feature - and selected by default - so I don't think anyone has that explicitly listed in their Cargo.toml).

[Ralith]

dlopen isn't any kind of linking at all, so I think linked is a pretty apt distinction; renaming the other feature makes sense, and indeed shouldn't be very disruptive.

[Ralith]

I've added a commit which folds all the various entry types into a single monomorphic Entry struct that contains a lib guard iff runtime loading is enabled, just like I do in the OpenXR bindings. I think this is a big win for API simplicity.

[Ralith]

Findings from CI: libvulkan.so.1 (which is what ends up in the binary's RUNPATH) is installed by default, but libvulkan.so (which is the symlink the linker finds at compiletime) comes from libvulkan-dev, which is not.

[Rua]

The Entry API currently assumes that you will be using the "fancy" wrappers, but Vulkano uses only the bare wrappers, EntryFnV1_0 and so on. Therefore, Vulkano has kept using its original loader for the moment. Would it be possible to change the loading mechanism in such a way that it can be used with the bare wrappers as well? Then Vulkano could use it. Thanks!

[Ralith]

What exactly about the current iteration poses a problem for you? You've always been able to access the function pointer table directly via e.g. Entry::fp_v1_0.

[Rua]

True, but it feels a bit weird to construct an Entry when I don't intend to use it at all. Separating the loading mechanism from the rest of the Vulkan API seems cleaner.

[Ralith]

How exactly do you not intend to use it? Entry loads function pointers, which is what you're asking for access to.

[MarijnS95]

dlopen isn't any kind of linking at all, so I think linked is a pretty apt distinction; renaming the other feature makes sense, and indeed shouldn't be very disruptive.

While I would technically agree, this isn't the first time I have seen it being called a dynamic linking loader.

I've added a commit which folds all the various entry types into a single monomorphic Entry struct that contains a lib guard iff runtime loading is enabled, just like I do in the OpenXR bindings. I think this is a big win for API simplicity.

Should be fine, it's extremely unlikely for the features to be used together. In the rare event that someone wishes to use multiple Vulkan libraries at the same time (building whatever arbitration and circumventing the ICD) they better use runtime loading to keep the libraries separated.

Findings from CI: libvulkan.so.1 (which is what ends up in the binary's RUNPATH) is installed by default, but libvulkan.so (which is the symlink the linker finds at compiletime) comes from libvulkan-dev, which is not.

Fortunately we're already using libvulkan.so.1 in entry_libloading.rs, not libvulkan.so, but that isn't easily selectable with println!("cargo:rustc-link-lib=vulkan"); I suppose? Pretty strange of Ubuntu to provide it that way...

The Entry API currently assumes that you will be using the "fancy" wrappers, but Vulkano uses only the bare wrappers, EntryFnV1_0 and so on. Therefore, Vulkano has kept using its original loader for the moment. Would it be possible to change the loading mechanism in such a way that it can be used with the bare wrappers as well? Then Vulkano could use it. Thanks!

@Ralith It appears @Rua is referring to this shorthand:

https://github.com/MaikKlein/ash/blob/17149bd791cd6e09b145063a238d3e27d855780c/ash/src/entry.rs#L159-L166

Which allows all structs in ash/src/extensions to accept an Entry to call get_instance_proc_addr() on, instead of the raw vk::StaticFn. I suppose Vulkano is keeping track of the various vk::*Fn* structs on its own?

But that has always been the case. Not sure if it should change towards StaticFn as part of this PR, the lines are already being touched though. In the end I'm in favour of this change as Ash should just accept the bare minimum (StaticFn in this case) in its API.

[MarijnS95]

This is looking really good so far. Should we let the CI run on it? I'm curious what others think on it, @MaikKlein? It's quite an impactful change, but I'd love to roll this out soon.

[MaikKlein]

Awesome, I don't have any concerns here.

If anyone has any ideas what, if any, special measures are needed for good behavior on macOS, let me know!

Not sure myself. I am also not sure how this will integrate with ash-molten.

I'd like to keep ash-molten as a separate crate because it is quite opinionated and builds or downloads moltenvk itself.

I think we can keep ash-molten as is and it can internally do Entry::from_static_fn. I think we can use no-default-features there as well and let ash-molten statically link with vulkan.

[MarijnS95]

Should we let the CI run on it?

CI seems to be running on it fine here? Maybe that's only visible to me. Will un-draft it once I've satisfied my concerns about the Windows build.

Yeah, that. Good to see it run now.

@Ralith It appears @Rua is referring to this shorthand:

I don't really follow. What capability is missing from the refactored Entry?

Nothing that this PR changes, afaik. They must not be using Entry at all for whatever reason. The only change I can see being useful is to require just StaticFn in extension constructors instead of the "full" Entry which is not technically required to call StaticFn::get_instance_proc_addr.

Pretty strange of Ubuntu to provide [libvulkan.so in the -dev package]

I think this is reasonable for them to do, as the unversioned .so symlink is only referenced at linktime. If we somehow hardcoded the .so.1 in the linker args that would just make us fragile and unidiomatic.

Derp. Yes, that is totally sensible. We're already using .so.1 for loading at least. Looking at ldd $(which vkcube) on my machine (Arch Linux) shows links against the versioned variant too. Seems like the most sane thing to do compatibility-wise (but apparently tricky to get right).

it's extremely unlikely for the features to be used together

To be clear, they can still be used together with the unified type; cargo features must be composable, after all.

Exactly. That's better than making them incompatible and breaking --all-features in strange ways, thanks!

While I would technically agree, this isn't the first time I have seen it being called a dynamic linking loader.

Hah, good cite. I'm open to clearer naming ideas.

With both features enabled by default I don't think it matters too much anymore. dynamic_linked and runtime_linked are a bit too verbose.

[MarijnS95]

Ready to get in. Not sure if we should bite the bullet and accept StaticFn instead of Entry in all the extension constructors though, this PR is already touching all those lines anyway.

[Ralith]

With both features enabled by default

This PR disables dynamic loading by default, since it's a niche case and brings in an extra dependency. I think the docs still do a reasonably good job of making things clear thanks to doc(cfg(_)) though.

Not sure if we should bite the bullet and accept StaticFn instead of Entry in all the extension constructors though, this PR is already touching all those lines anyway.

I'd prefer to leave that for future work, since it's logically independent and you can easily construct an Entry given a StaticFn already.

[MarijnS95]

Then we'll wait for @MaikKlein to give the final sign-off on this, and get it in! Let's immediately cut a new minor release as well with perhaps some other PRs?

[Ralith]

@MaikKlein ping?

[MaikKlein]

Sorry for the delay, looks good 🥳 and I'd like to get this in.

Two high level comments:

• This will default to dynamic/static loading which will most likely break all the CIs with a linker error. I assume most don't actually have a Vulkan environment. So I think I'd prefer either runtime loading by default or no default features at all. With "no default features " at least they would get a build error.

• Does this PR solve Support for a statically linked vulkan library #462? How would one build ash statically wit this PR? What happens if you have both the .so and .a in the path? IIRC you can specify the link type somewhere as a flag. Just asking, this PR doesn't have to solve this.

[MarijnS95]

• This will default to dynamic/static loading which will most likely break all the CIs with a linker error. I assume most don't actually have a Vulkan environment. So I think I'd prefer either runtime loading by default or no default features at all. With "no default features " at least they would get a build error.

According to #457 (comment) this needs sudo apt-get install libvulkan-dev as seen in ci.yaml.

• Does this PR solve Support for a statically linked vulkan library #462? How would one build ash statically wit this PR? What happens if you have both the .so and .a in the path?

According to https://github.com/MaikKlein/ash/pull/457/files#r689123563 the .so is preferred over the .a. I marked #462 as linked to this PR since the asker only seems to have a .a (if they're cross-compiling and it finds the hosts' .so that's a different issue), probably something to find out over time?

IIRC you can specify the link type somewhere as a flag. Just asking, this PR doesn't have to solve this.

There's the optional [kind=], we can add cargo:rustc-link-lib=static=vulkan (and dylib= just in case) with some feature flag if this turns out to be a problem.

[MaikKlein]

According to #457 (comment) this needs sudo apt-get install libvulkan-dev as seen in ci.yaml.

Yeah that is what I meant. It will break all the CIs without a vulkan environment and they have to add sudo apt-get install libvulkan-dev right?

[Ralith]

Yes, the build environment will need to have a libvulkan for the target available. IMO it's a reasonable default. For one thing, the linker error you get for a missing foreign library is highly recognizable and easy to google, with the same solution across all programming languages, whereas a build error due to not having default features would be more idiosyncratic.

Leaving the default unchanged wouldn't be a disaster, but there's something to be said for defaulting to the lighter-weight, more robust, and more conventional configuration.

How would one build ash statically wit this PR?

As @MarijnS95 says, it'll use whatever's available in the search path. If the target environment only has a static library, then that's what'll be used. I expect this will be sufficient in practice; especially when cross-compiling, the user has considerable control over the available libraries.

[Jasper-Bekkers]

Yes, the build environment will need to have a libvulkan for the target available. IMO it's a reasonable default. For one thing, the linker error you get for a missing foreign library is highly recognizable and easy to google, with the same solution across all programming languages, whereas a build error due to not having default features would be more idiosyncratic.

I'll resurrect this thread now that ash 0.34 is out and we're starting to see the first linker errors across our crates. I think this may have been a mistake. Rust users aren't used to fixing (or seeing) linker errors, and I honestly think this is a bad precedent to set from a usability point of view.

[Ralith]

FWIW, there is overwhelming precedent for this behavior for FFI crates (e.g. cpal, openssl, openexr, gtk, ...). While I'm sure ash 0.34 will be some peoples' first encounter with link-time dependencies, most ash users should find this familiar and easily diagnosed.

[aloucks]

I agree with @Jasper-Bekkers. This imposes a burden that is 100% avoidable. This is not the same as a sys crate where we are building the C source. We're linking to an external library. I think it's fine to support compile time linking but it should not be the default.

[Jasper-Bekkers]

I agree with @Jasper-Bekkers. This imposes a burden that is 100% avoidable. This is not the same as a sys crate where we are building the C source. We're linking to an external library. I think it's fine to support compile time linking but it should not be the default.

Along with this, it's quite unclear what the need for this change was to begin with, dropping a small and extremely common dependency is not really a gain. Neither in build times (only if you look at them in isolation, not when you look at them in the context of a much larger app) nor in runtime performance.

At best it's a dubious default to have, at worst it actively harms usability for this crate. I would suggest doing a fox release that reverts this behavior honestly.