Separate function-pointer structs between instance and device #734

MarijnS95 · 2023-04-03T21:41:06Z

~~Depends on #733~~
Fixes #727

Not only is it more efficient to load specialized device functions via get_device_proc_addr() instead of get_instance_proc_addr() (saves a device-based jump in the ICD), loading instance functions via get_device_proc_addr() (which was the case in VK_KHR_swapchain, VK_KHR_device_group and VK_EXT_full_screen_exclusive) always results in a NULL causing their instance functions panic no matter what.

Low-level *Fn structs are separated out between instance and device pointers for every extension that contains both, forcing users (typically high-level extension writers) to explicitly account for this.

ash/src/extensions/ext/calibrated_timestamps.rs

Ralith · 2023-04-05T01:24:42Z

Having large numbers of types with one of two suffices feels a bit like we're reimplementing the module system. A ZST purely for the sake of the name constant also feels a bit weird.

Possible alternative approaches:

Three types per extension, with FP tables canonically exposed via associated types on a single extension ZST, alongside an associated const NAME
The above, except the root extension type has fields instance: InstanceFn, device: Option<DeviceFn>
One module per extension, containing const NAME, struct Device, and/or struct Instance
Two structs per extension, each having the same name but inside ash::extensions::device::... and ash::extensions::instance::... respectively (and similarly for the raw layer)

Ralith · 2023-04-05T01:25:56Z

I also wonder if we should scrap the panicing stub functions and fail loaders if any function doesn't load, since that's already bitten us pretty badly by making the errors subtle (and confused users/contributors). I guess that's tricky for versioned stuff.

MarijnS95 · 2023-04-05T21:54:49Z

Having large numbers of types with one of two suffices feels a bit like we're reimplementing the module system. A ZST purely for the sake of the name constant also feels a bit weird.

We already have modules but only for the sake of ash readability it seems: all these structs are reexported from the module hierarchy. If we go ahead with a module system we'd have to drop the reexports and make the modules public (or only for these few specific extensions? Then I'm more in favour of associated types on a ZST to at least look similar to other extensions).

Possible alternative approaches:

Three types per extension, with FP tables canonically exposed via associated types on a single extension ZST, alongside an associated const NAME

I've been thinking the same thing for the suggested ZST above, but then would wonder if we could keep the struct name "private" in favour of always referring to it as ExtensionName::Device?

Unfortunately these so-called inherent associated types are still unstable:

pub struct CalibratedTimestamps;

impl CalibratedTimestamps {
    // inherent associated types are unstable
    // see issue #8995 <https://github.com/rust-lang/rust/issues/8995> for more information
    pub type Device = CalibratedTimestampsDevice;
    pub type Instance = Instance;
}

The above, except the root extension type has fields instance: InstanceFn, device: Option<DeviceFn>

The downside here is that it forces reloading instance functions, if you already have them or only care about device functions. OTOH we can find a middle-ground here a chain of constructors either loads instance, or takes/moves an existing value for it so that you can upgrade the struct with "device" support.

Additionally we could keep a mode or constructor where the device functions are loaded via get_instance_proc_addr (and make it all available via a single type) but nobody complained about the current setup requiring Device for device-specific extensions so this isn't useful and only adds lots complexity/code.

One module per extension, containing const NAME, struct Device, and/or struct Instance

That'd be nice and generic, if a bit harder to manage for the folks that import structs rather than modules, but nothing that can't be fixed. Perhaps worth investigating given that associated types on ZSTs are the "clearer" (?) variant of this but not yet stabilized.

In this case I'd name them DeviceFn and InstanceFn or something similar, to disambiguate between a real Vulkan device/instance? (Not all extensions hold the device/instance handle after all)

Two structs per extension, each having the same name but inside ash::extensions::device::... and ash::extensions::instance::... respectively (and similarly for the raw layer)

Doing the device/instance split on the module level rather than on the struct level? That may be interesting but really pulls the codebase apart and makes it again more prone to import the same item names in scope (solving it either with as DeviceXXXExtension or qualifying in scope with device::XXXExtension).

MarijnS95 · 2023-04-05T21:56:58Z

I also wonder if we should scrap the panicing stub functions and fail loaders if any function doesn't load, since that's already bitten us pretty badly by making the errors subtle (and confused users/contributors). I guess that's tricky for versioned stuff.

We should definitely have a mode where this is caught up-front, yes. Just thinking about how we should represent ash::Device containing four function pointer groups now, or start with the extensions first (which are typically an all-or-nothing, except the broken get_physical_device_present_rectangles() in this PR which requires Vulkan 1.1 in addition to the Swapchain extension. And there are probably more such combinations out in the wild now).

…ctions This is a minimal, semver-compatible backport of #734 to the `0.37-stable` branch, warning Ash users of the problem outlined below while the issue is properly being solved in the next breaking Ash release (by separating `Instance` and `Device` functions in the generator to avert this problem entirely while also always providing optimal `Device`-specific functions for extension wrappers that are currently already loading _everything_ via `Instance` to forgo the problem). As discovered and detailed in #727 a few extension wrappers were loading and calling `Instance` functions via `Device` and `get_device_proc_addr()` which is [defined] to only return non-`NULL` function pointers for `Device` functions. Those wrapper functions will always call into Ash's panicking NULL-stub functions as the desired `Instance` function could not be loaded. Deprecate the `new()` functions for extension wrappers that were doing this, while pointing the reader to `new_from_instance()` and explaining in the docs what function will always `panic!()` when the struct was loaded using `new()` instead. This function always takes a raw `vk::Device` directly to fill `handle` (rather than `ash::Device` to retrieve `handle()` from), allowing users to pass `vk::Device::null()` when they do intend to load this extension wrapper just for calling the `Instance` function. [defined]: https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetDeviceProcAddr.html#_description

…`Instance` functions (#754) extensions: Provide `new_from_instance()` fallback for `Instance` functions This is a minimal, semver-compatible backport of #734 to the `0.37-stable` branch, warning Ash users of the problem outlined below while the issue is properly being solved in the next breaking Ash release (by separating `Instance` and `Device` functions in the generator to avert this problem entirely while also always providing optimal `Device`-specific functions for extension wrappers that are currently already loading _everything_ via `Instance` to forgo the problem). As discovered and detailed in #727 a few extension wrappers were loading and calling `Instance` functions via `Device` and `get_device_proc_addr()` which is [defined] to only return non-`NULL` function pointers for `Device` functions. Those wrapper functions will always call into Ash's panicking NULL-stub functions as the desired `Instance` function could not be loaded. Deprecate the `new()` functions for extension wrappers that were doing this, while pointing the reader to `new_from_instance()` and explaining in the docs what function will always `panic!()` when the struct was loaded using `new()` instead. This function always takes a raw `vk::Device` directly to fill `handle` (rather than `ash::Device` to retrieve `handle()` from), allowing users to pass `vk::Device::null()` when they do intend to load this extension wrapper just for calling the `Instance` function. [defined]: https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetDeviceProcAddr.html#_description

MarijnS95 · 2023-05-06T19:12:24Z

@Ralith WDYT? This is one of the few PRs remaining before cutting 0.38, I think :)

Ralith · 2023-05-06T19:28:23Z

This needs a proper think, will try to get back to you on it later today. Thanks for driving everything forwards!

MarijnS95 · 2023-05-06T19:56:53Z

@Ralith lovely, let's surely think this through properly before merging and releasing any hasty implementation (though we can neatly use this PR to experiment and reflect, it was opened specifically to spark such a discussion). We can also defer to #727 to discuss this.

Ralith · 2023-05-07T05:15:46Z

Unfortunately these so-called inherent associated types are still unstable:

Aw.

we can find a middle-ground here a chain of constructors either loads instance, or takes/moves an existing value for it so that you can upgrade the struct with "device" support.

That's what I was thinking. Something like:

struct SomeExt {
    instance_handle: vk::Instance,
    instance_fn: InstanceFn,
    device: Option<(vk::Device, DeviceFn)>,
}

impl SomeExt {
    fn load(instance: &Instance, device: &Device) -> Self { ... }
    fn load_instance(instance: &Instance) -> Self { ... }
    fn load_device(&mut self, device: &Device) { ... }
}

This has the drawback of forcing dynamic checks on every device fn, of course, so I'm not sold on it. But maybe if the DeviceFn is also public it'd be an opportunity to let the user choose? Fair bit of duplicated code to offer high level wrappers on both, though.

If we go ahead with a module system we'd have to drop the reexports and make the modules public

That was my thinking. We'd set a convention of always importing at most the modules, never their contents, similar to idiomatic use of e.g. std::{io, mpsc, thread, ...}, and do so consistently for all extensions. Having some extensions be FooFn and some be foo::{InstanceFn, DeviceFn} would be too confusing, and being clear about the stage an extension is usable at seems nice even if it happens to only have one.

[module per extension]'d be nice and generic if a bit harder to manage for the folks that import structs rather than modules

What's the difference? An item's an item, as far as scoping is concerned.

In this case I'd name them DeviceFn and InstanceFn or something similar, to disambiguate between a real Vulkan device/instance? (Not all extensions hold the device/instance handle after all)

Makes sense to me.

given that associated types on ZSTs are the "clearer" (?) variant

It's not obvious to me that ZSTs are clearer in this application. Generally we use types to describe values, and in that case there's no values, so why have a type at all?

[splitting on device/instance first] may be interesting but really pulls the codebase apart

Yeah, I think that's probably the wrong way to go. It makes it difficult to survey the contents of an extension, and there's no single graceful place to hang the NAME.

MarijnS95 · 2023-05-07T07:05:02Z

[module per extension]'d be nice and generic if a bit harder to manage for the folks that import structs rather than modules

What's the difference? An item's an item, as far as scoping is concerned.

It seems to be more common to import items in scope rather than strategically importing modules (and this may also be driven by how rust-analyzer helps you do that, though it might also help you import modules?). So for example, a user may start out with:

use ash::extensions::khr::swapchain::{NAME, DeviceFn};

And now have a meaningless DeviceFn item in scope throughout their source. This gets worse when using a second or more extensions (But is equivalently easy to solve by importing up until swapchain;.

given that associated types on ZSTs are the "clearer" (?) variant

It's not obvious to me that ZSTs are clearer in this application. Generally we use types to describe values, and in that case there's no values, so why have a type at all?

Given the above, inherent associated types cannot be imported in scope yet (e.g. use ...::Swapchain::DeviceFn), so the above problem is forcefully solved.
(However, one can import/reexport enum variants so this might be enabled before the feature is stabilized?)

MarijnS95 · 2023-05-07T07:06:53Z

Re. "how std does it", the new model feels like we're having the following layout:

use std::collections::hash_map::Collection;
use std::collections::hash_set::Collection;
use std::collections::vec::Collection;

(Disregard the double collections module)

because all the modules have identically-named items.

Ralith · 2023-05-07T22:42:46Z

Contrast std::{format, io, thread}::Result, the many instances of ...::{Iter, IterMut}, std::{io, iter}::Take, etc. It's a well established pattern. Even rust-analyzer will only import DeviceFn if the user starts out by writing unqualified DeviceFn; starting with the extension name, as both exploring users and those referring to examples will tend to, will behave sensibly.

use std::collections::hash_map::Collection;

This doesn't exist because there's necessarily a sensible "main" type for any collection.

(However, one can import/reexport enum variants so this might be enabled before the feature is stabilized?)

Yeah, there's already some pressure for this today for associated constants, which would help bitfields and similar patterns feel first class.

MarijnS95 · 2023-05-08T07:46:35Z

Contrast std::{format, io, thread}::Result, the many instances of ...::{Iter, IterMut}, std::{io, iter}::Take, etc. It's a well established pattern. ...

use std::collections::hash_map::Collection;

This doesn't exist because there's necessarily a sensible "main" type for any collection.

Besides Result those types are typically never imported in scope in user code; they're the implicit result of a .take() / .iter() that the user uses without constructing or storing themselves. This is different for our DeviceFn/InstanceFn types which do count as a "main" type similar to the collections.

(Side note: I did find the various Results annoying to work with at times. Some codebases and even files in my own project import it from random modules, some with sensible default type Result<T, E = MyError> = Result<T, E>;, others with a forced error-type type Result<T> = Result<T, MyError>)

Just playing devil's advocate here 😉, making sure we properly think through the opportunities and drawbacks this approach has - even if it likely is the only way to implement the split right now given a lack of stabilized inherent associated types.

How should we do this for the generated code, by the way? I was thinking to just have a bunch of in-line:

mod swapchain {
    use super::*; // Import global scope, or reference everything with `super::`.
    const NAME = ...;
    struct DeviceFn {
        ...
    }
    struct InstanceFn {
        ...
    }
}

but haven't yet thought about how to structure the PFN types here; I may want to move them to a module since they're currently split across definitions.rs, features.rs and extensions.rs, with extra logic to either reference it from local scope or from crate::vk::PFN_.. if it was already defined elsewhere.

Even rust-analyzer will only import DeviceFn if the user starts out by writing unqualified DeviceFn; starting with the extension name, as both exploring users and those referring to examples will tend to, will behave sensibly.

Agreed, if the users writes swapchain::... it should prompt to import the module in scope.

Ralith · 2023-05-08T22:28:19Z

Besides Result those types are typically never imported in scope in user code

I've named them explicitly a number of times (e.g. slice::Iter comes in handy in APIs fairly regularly), but I'll allow that it's not common. mpsc::Receiver and thread::spawn are additional good examples of something that, while not heavily overloaded if we consider std in isolation, would be awfully vague unqualified. Free functions in std::{mem, iter} are similarly too terse to be comfortable to use unqualified.

This is different for our DeviceFn/InstanceFn types which do count as a "main" type similar to the collections.

The point was that since there's two of them, neither has inherent primacy.

How should we do this for the generated code, by the way? I was thinking to just have a bunch of in-line:

It's appealing to group everything defined by an extension inside the same module. This minimizes reference-chasing and makes it easy to inventory, and maps closely to idiomatic Rust. Might be nice to isolate the function pointer definitions a little since usually folks aren't looking for those, but e.g. having miscellaneous struct definitions right there alongside the new methods sounds nice. Perhaps ash::ext::pfn::*? ...::signatures::*?

Unfortunate that this proposal transposes the current organization of the generator output, though... seems like that might be churny to implement.

MarijnS95 · 2023-05-15T12:16:49Z

mpsc::Receiver and thread::spawn are additional good examples of something that, while not heavily overloaded if we consider std in isolation, would be awfully vague unqualified. Free functions in std::{mem, iter} are similarly too terse to be comfortable to use unqualified.

Agreed.

The point was that since there's two of them, neither has inherent primacy.

Indeed, either has it, but it's not about the primacy between these two elements within the module.

It's appealing to group everything defined by an extension inside the same module. This minimizes reference-chasing and makes it easy to inventory, and maps closely to idiomatic Rust. Might be nice to isolate the function pointer definitions a little since usually folks aren't looking for those, but e.g. having miscellaneous struct definitions right there alongside the new methods sounds nice. Perhaps ash::ext::pfn::*? ...::signatures::*?

Unfortunate that this proposal transposes the current organization of the generator output, though... seems like that might be churny to implement.

Main problem here is that PFNs are used outside of and across extensions as well; that idea might fly but we have to be more careful where the first definition of the PFN is going to be (currently it is all over the place) and adjust the generator to import it from the right place.

We'll need to do mod scoping around DeviceFn, InstanceFn, NAME and SPEC_VERSION, let's start with that.

Good point on the churny-ness: I've already received comments about ash changing too much for "being a simple sys wrapper" before even releasing these changes. And while I agree that the builder changes and now the Fn separation is very churny, it's all for the better (and we're much more than just a sys crate with bindgen-like content).

Ralith · 2023-08-27T18:05:45Z

Do we have any open questions here?

MarijnS95 · 2023-08-27T18:11:46Z

@Ralith I think I was going to mod-ify the low-level generated bindings, and then we see how to do tackle the hand-written bindings from there?

Still unsure what the best approach / naming would be.

Ralith · 2023-08-27T19:15:36Z

I stand by my preference for module-per-extension as discussed, but I don't feel strongly enough to block things.

MarijnS95 · 2023-08-27T19:17:14Z

Yeah I think that is totally reasonable, just hope the rename from CamelCase types to snake_case modules for every extension aren't going to be too much of a hassle for folks!

Still okay to keep InstanceFn and DeviceFn purely separate, or do you rather keep the combined structure proposed in #734 (comment)?

Ralith · 2023-08-27T19:36:05Z

just hope the rename from CamelCase types to snake_case modules for every extension aren't going to be too much of a hassle for folks!

I think it's mitigated by a few things: most people are probably mostly using core functions, and extension struct types don't need to be named very often. Hard to see how we'd get out of this hole without renaming them somehow, and with a small number of references, the cost of having to make a change at all dominates the specifics of the change.

Still okay to keep InstanceFn and DeviceFn purely separate, or do you rather keep the combined structure proposed in #734 (comment)?

I don't really like the Option there. I'm more tempted by the idea of a single struct that populates device fns with panicing stubs (or even unchecked Options), but if we were to do that, we might as well go all the way with a single monolithic table containing all extensions. Separate instance/device structs is most consistent with our existing, less error-prone paradigm of independently loaded tables.

MarijnS95 · 2023-08-27T21:50:21Z

My plan was still to have the separate InstanceFn and DeviceFn structs, and obviously separated per-extension as we already do (which is still complicated when multiple extensions are required, or when multiple extensions individually enable the same function, but those cases are few and far between).

…g` redirect

Ralith · 2024-03-17T02:07:17Z

Fixed up the CI failures, and implemented the "create modules for vendor prefixes" task. This required a hack for extensions whose names begin with a number (e.g. VK_KHR_16bit_storage); I resolved this by prefixing a _ (ash::vk::khr::_16bit_storage) but I'm open to alternatives.

generator/src/lib.rs

…evice-commands

…ing `vk::Device` argument When introducing this new `VK_KHR_calibrated_timestamps` extension based on `VK_EXT_calibrated_timestamps` in #890 in parallel to finalizing and merging the `Instance`/`Device` separation in #734, I seem to have missed an opportunity to use the newly available `self.handle` for this extension from `struct Device`, instead leaving an unnecessary `device: vk::Device` argument in place.

…ing `vk::Device` argument (#898) When introducing this new `VK_KHR_calibrated_timestamps` extension based on `VK_EXT_calibrated_timestamps` in #890 in parallel to finalizing and merging the `Instance`/`Device` separation in #734, I seem to have missed an opportunity to use the newly available `self.handle` for this extension from `struct Device`, instead leaving an unnecessary `device: vk::Device` argument in place.

MarijnS95 commented Apr 3, 2023

View reviewed changes

ash/src/extensions/ext/calibrated_timestamps.rs Outdated Show resolved Hide resolved

MarijnS95 force-pushed the separate-instance-device-commands branch from 97e731c to 01eada4 Compare April 3, 2023 22:24

MarijnS95 force-pushed the separate-instance-device-commands branch from 01eada4 to 7b1292b Compare April 5, 2023 21:57

MarijnS95 added the Breaking change label May 6, 2023

MarijnS95 mentioned this pull request May 6, 2023

[0.37-stable] extensions: Provide new_from_instance() fallback for Instance functions #754

Merged

MarijnS95 mentioned this pull request Jun 5, 2023

Some _device_ extensions are loading (and failing) _instance_ functions via get_device_proc_addr() #727

Closed

MarijnS95 force-pushed the separate-instance-device-commands branch from 6b07904 to 60b5179 Compare March 16, 2024 21:07

This comment was marked as off-topic.

Sign in to view

MarijnS95 added 6 commits March 16, 2024 22:48

Manual fixups

fce90c6

fixup! generator: Separate Fn struct

dc0d257

extension: Rename wrapper structs to Device or Instance

e407517

Remove unnecessary www.khronos.org/registry -> registry.khronos.or…

e8330f7

…g` redirect

Extension module documentation

ea58561

Rename modules

a994e4e

MarijnS95 force-pushed the separate-instance-device-commands branch from 60b5179 to a994e4e Compare March 16, 2024 22:12

Fix lint

05d0cf0

Ralith force-pushed the separate-instance-device-commands branch from 7252c03 to e0c1c76 Compare March 17, 2024 02:02

Ralith added 2 commits March 16, 2024 19:05

fixup! generator: Separate Fn struct

f06c0c7

Split extension modules into vendor/extension hierarchy

03ad1f4

Ralith force-pushed the separate-instance-device-commands branch from e0c1c76 to 03ad1f4 Compare March 17, 2024 02:05

fixup! fixup! generator: Separate Fn struct

0781216

MarijnS95 commented Mar 24, 2024

View reviewed changes

generator/src/lib.rs Outdated Show resolved Hide resolved

generator/src/lib.rs Outdated Show resolved Hide resolved

MarijnS95 force-pushed the separate-instance-device-commands branch from b06c835 to e3f64d8 Compare March 24, 2024 15:33

Minor nits on latest push

1ac974d

MarijnS95 force-pushed the separate-instance-device-commands branch from e3f64d8 to 1ac974d Compare March 24, 2024 15:34

Merge remote-tracking branch 'origin/master' into separate-instance-d…

6bacced

…evice-commands

MarijnS95 merged commit bd63892 into master Mar 24, 2024
20 checks passed

MarijnS95 deleted the separate-instance-device-commands branch March 24, 2024 17:03

MarijnS95 mentioned this pull request Mar 30, 2024

Unify extension module hierarchies #894

Merged

MarijnS95 mentioned this pull request Mar 31, 2024

extensions/khr/calibrated_device: Use self.handle instead of requiring vk::Device argument #898

Merged

MarijnS95 mentioned this pull request Mar 31, 2024

Release ash 0.38.0 and ash-window 0.13.0 #897

Merged

Neo-Zhixing mentioned this pull request Apr 24, 2024

Add InstanceExtension and DeviceExtension traits #912

Open

MarijnS95 added this to the Ash 0.38 milestone Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate function-pointer structs between instance and device #734

Separate function-pointer structs between instance and device #734

MarijnS95 commented Apr 3, 2023 •

edited

Loading

Ralith commented Apr 5, 2023

Ralith commented Apr 5, 2023 •

edited

Loading

MarijnS95 commented Apr 5, 2023

MarijnS95 commented Apr 5, 2023

MarijnS95 commented May 6, 2023

Ralith commented May 6, 2023

MarijnS95 commented May 6, 2023

Ralith commented May 7, 2023 •

edited

Loading

MarijnS95 commented May 7, 2023

MarijnS95 commented May 7, 2023

Ralith commented May 7, 2023

MarijnS95 commented May 8, 2023

Ralith commented May 8, 2023

MarijnS95 commented May 15, 2023

Ralith commented Aug 27, 2023

MarijnS95 commented Aug 27, 2023

Ralith commented Aug 27, 2023

MarijnS95 commented Aug 27, 2023

Ralith commented Aug 27, 2023

MarijnS95 commented Aug 27, 2023

This comment was marked as off-topic.

Ralith commented Mar 17, 2024 •

edited

Loading

Separate function-pointer structs between instance and device #734

Separate function-pointer structs between instance and device #734

Conversation

MarijnS95 commented Apr 3, 2023 • edited Loading

Ralith commented Apr 5, 2023

Ralith commented Apr 5, 2023 • edited Loading

MarijnS95 commented Apr 5, 2023

MarijnS95 commented Apr 5, 2023

MarijnS95 commented May 6, 2023

Ralith commented May 6, 2023

MarijnS95 commented May 6, 2023

Ralith commented May 7, 2023 • edited Loading

MarijnS95 commented May 7, 2023

MarijnS95 commented May 7, 2023

Ralith commented May 7, 2023

MarijnS95 commented May 8, 2023

Ralith commented May 8, 2023

MarijnS95 commented May 15, 2023

Ralith commented Aug 27, 2023

MarijnS95 commented Aug 27, 2023

Ralith commented Aug 27, 2023

MarijnS95 commented Aug 27, 2023

Ralith commented Aug 27, 2023

MarijnS95 commented Aug 27, 2023

This comment was marked as off-topic.

Ralith commented Mar 17, 2024 • edited Loading

MarijnS95 commented Apr 3, 2023 •

edited

Loading

Ralith commented Apr 5, 2023 •

edited

Loading

Ralith commented May 7, 2023 •

edited

Loading

Ralith commented Mar 17, 2024 •

edited

Loading