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

Consider a unified function pointer table #878

Open
Friz64 opened this issue Mar 16, 2024 · 2 comments
Open

Consider a unified function pointer table #878

Friz64 opened this issue Mar 16, 2024 · 2 comments

Comments

@Friz64
Copy link
Contributor

Friz64 commented Mar 16, 2024

Throwing this out there: Why not have a table for Entry, Instance, and Device each, that holds all function pointers? erupt did this. The loading API becomes really pretty, as we can figure out which extensions to load by reading the ppEnabledExtensionNames array behind the scenes. I encourage poking around erupt to see what I mean by this:

Users would also e.g. be able pass around one single object for all their device functions needs. And regarding the stack size of this, I think that we should just store it in an Arc by default (related discussion @ #731).

I understand that this would be a major breaking change, but I don't want to leave it unmentioned.


I originally mentioned this in this comment: #734 (comment), where Ralith replied:

This (...) has also been discussed at length in the past. We concluded that using extension tables to represent extension availability is a valuable pattern to nudge downstream code towards, as it's far less error prone than representing optional extension presence with a side channel and trying to remember to check if any time you use a function from it, if you can even remember which functions come from which extension.

Our current separate wrappers approach is imperfect (some functions may be dynamically absent even if an extension is loaded successfully, ash::Device is verison-independent, etc) and I've gone back and forth myself.

@Friz64
Copy link
Contributor Author

Friz64 commented Mar 16, 2024

Just playing around with ideas, throwing this one out there: Introduce a "hybrid" design, where we still have a central loader struct for for Entry, Instance, and Device each, but then store function pointers for extensions in their own struct. This way we can still determine which extensions to load based on the ppEnabledExtensionNames array, but functions are clearly associated with extensions.

You could then store each extension struct in an Option, which would also make it easy to check if an extension was loaded. But that would result in yet another check required when accessing function pointers (potentially on top of the layer introduced by #877).

Also, do you then place the function wrappers on the base struct, or the extension struct? The former would work well in most cases, but there a few functions which are "required" by multiple extensions, such as vkGetPhysicalDevicePresentRectanglesKHR (VK_KHR_swapchain and VK_KHR_device_group), and so it would make it unclear which one to choose. The latter makes optional extensions explicit, but is unergonomic to use.

Everything I said about extensions also applies to Vulkan versions >1.0, which would get the same treatment per version.

@Ralith
Copy link
Collaborator

Ralith commented Mar 16, 2024

The polar opposite of this proposal would be collecting every group of function pointers that share conditions for availability (e.g. "extensions X and Y enabled, core version > Z") into a separate struct which should only be constructed when those functions are genuinely available, making it easy to express guarantees of that availability at the type level.

The status quo is in an awkward in-between state: we have separate structs for each extension, but some extensions have conditionally-available functions, and we haven't separated core functions from different versions. Similarly, many extension features are exposed not through new function pointers, but extension structs passed indirectly to existing functions. Some functions might also depend on enabling fine-grained features, which may not always be supported. Separate function pointer tables cannot guard against incorrect use of these.

Our type-level reasoning about extension presence is therefore much weaker than it might seem, but in practice @MarijnS95 and I have still found it useful. In the end I remain ambivalent, except that I'd very much like to get a release out before pursuing far-reaching changes.

Users would also e.g. be able pass around one single object for all their device functions needs

I think this is a red herring for application code, because in real-world code the Vulkan state you pass around extends well beyond a single function pointer table, and the cost of an extra field in whatever larger struct you're using is insignificant.

Library code (when sharing Vulkan resources across independently maintained crates) is more interesting. Take #506 for a case study. In particular, the use of:

/// Functions required by [`Swapchain`] methods
pub struct Functions<'a> {
    pub device: &'a Device,
    pub swapchain: &'a khr::Swapchain,
    pub surface: &'a khr::Surface,
}

This is a microcosm of the tradeoffs: on the one hand, it makes the extensions required by the library impossible to miss. On the other, it's a bit less ergonomic, requiring callers to construct a temporary struct, and can't express common non-extension requirements like enabled features.

do you then place the function wrappers on the base struct, or the extension struct

Most users will only use the wrappers, so if they're on the base struct, the presence of optional extension structs is inconsequential. Ergonomics of using separate structs tend to be fine so long as you don't pass around required extensions behind a spurious Option.

And regarding the stack size of this, I think that we should just store it in an Arc by default (related discussion @ #731).

It'll still end up on the stack in debug builds, and even release builds if we don't take great care with initialization and/or run afoul of the optimizer. Users might also want to bundle it inside a larger struct which is itself on the heap, making the indirection a bit redundant.

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

No branches or pull requests

2 participants