-
Notifications
You must be signed in to change notification settings - Fork 187
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
Comments
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 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 Everything I said about extensions also applies to Vulkan versions >1.0, which would get the same treatment per version. |
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.
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.
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
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. |
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 theppEnabledExtensionNames
array behind the scenes. I encourage poking around erupt to see what I mean by this:DeviceLoader::new
DeviceLoaderBuilder
implementationDeviceEnabled::new
implementationDeviceLoader::custom
implementationget_physical_device_present_rectangles_khr
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:
The text was updated successfully, but these errors were encountered: