Make builders optional

[Ralith]

Reduces build time by ~22% in my environment.

I don't know if we should actually merge this as-is, since builders are such an important correctness helper. However, it's strong evidence that reducing the amount of builder code, e.g. as discussed in #441, could have a large benefit.

[MarijnS95]

Yeah I think I'd rather go for seeing if we can unify the builders and native structs instead.

Have you also checked the build time reduction when merely omitting trivial setters (removing everything except those touching lifetimes/arrays)? That might give a better indication what we could achieve in #441, bar having two types instead of one.

In particular I'm referring to the:

let foo = Foo { f, .. Default::default() }.with_bar_slice(&bar)

Where f is something trivial like an integer.

[aloucks]

I'd like to see an option that has the builder methods fused onto the native structs. The methods are helpful but the builder structs themselves are not. I did an experiment in #286 to try out two scenarios: (1) remove the builders entirely and (2) remove only the lifetime from the builders. The lifetimes in the builders are what causes them to compile so slowly, and add little value since build() discards the lifetime anyway. The builder methods only add about 0.5 seconds to the build. This resulted in a great debate around the safety of build() and I ultimately dropped the issue and moved on.

However, if we're now entertaining the idea of making an option to remove the builders, we should move the builder methods to the native structs. The compile time is only increased slightly.

[Ralith]

The builder methods only add about 0.5 seconds to the build

What is that in relative terms, for a clean build? Hard to generalize an absolute change between environments.

[lifetimes] little value since build() discards the lifetime anyway.

To the contrary, the lifetimes are the most valuable part. The trick is that you should almost never have to call build, due to the Deref impls, and the rare exceptions can be kept lexically inside a Vulkan call.

The plan we discussed in #441 was to move the lifetimes, along with the setter methods, into the Vulkan structs/calls themselves. If that still leaves a lot of buildtime on the table, that's unfortunate... dropping lifetimes entirely will certainly lead to more people shooting themselves in the foot, but perhaps there's a finite amount of buildtime that's worth trading off against.

[aloucks]

To the contrary, the lifetimes are the most valuable part. The trick is that you should almost never have to call build

"Almost never" is not the same as "never". The biggest quality-of-life improvement of the builders is when you need pass in a slice of something and this is the use case that requires build(). I really don't want to re-hash that whole discussion again, but the reality is that build() appears to be safe but it is not. It enables you to capture pointers to the stack and reference freed memory. You must heed caution when using it and it should be marked unsafe accordingly. It's not safe if the compiler can't enforce it. There's no precedent for this in the standard library and there have been multiple complaints and discussions both here and on reddit over the years. If I recall correctly, your argument was that calling build() itself cannot directly cause a segfault or UB. However it can absolutely cause that behavior indirectly, as we are essentially performing a transmute.

#441 doesn't work for slices, which makes it a non-starter, IMO.

The following addresses both the compilation slowness and footgun issues, while still keeping the quality-of-life improvements:

1. Remove the builder structs.
2. Move the builder methods to the native structs.
3. Mark any method that erases a lifetime unsafe.

I apologize if my tone comes across as anything but neutral, but I'm frustrated by the fact that this continues to pop up every so often and there has been no willingness to accept the fact that erasing a lifetime is unsafe even if the UB isn't fully expressed until the pointer in question has been dereferenced.

[Ralith]

I really don't want to re-hash that whole discussion again

Then I will disregard the following paragraph in which you do so :P

#441 doesn't work for slices, which makes it a non-starter, IMO.

The conclusion of the discussion in #441 included:

moving all setters to the actual Vulkan structs rather than builders

If we then lifetime-qualify the Vulkan structs themselves then there's no need to erase lifetimes at all. If we don't, then that's a big step backwards in static safety checking compared to the currently recommended pattern, which is a shame. I think we should prototype this and get hard numbers for the impact. We've been waiting for @MaikKlein's generator rewrite before diving into that, though that seems to be in limbo at the moment.

[aloucks]

moving all setters to the actual Vulkan structs rather than builders

That's what I'm suggesting. But, I don't think it's a good idea to treat the arrays and non-array pointers differently though, and you can't embed a slice into the native struct for the reasons you stated.

However, maybe we could add PhantomData to the end of the struct to hold the lifetime, and keep the pointers as-is for maximal flexibility, transparency, and consistency:

#[repr(C)]
pub struct DeviceCreateInfo<'a> {
    pub s_type: StructureType,
    pub p_next: *const c_void,
    pub flags: DeviceCreateFlags,
    pub queue_create_info_count: u32,
    pub p_queue_create_infos: *const DeviceQueueCreateInfo,
    pub enabled_layer_count: u32,
    pub pp_enabled_layer_names: *const *const c_char,
    pub enabled_extension_count: u32,
    pub pp_enabled_extension_names: *const *const c_char,
    pub p_enabled_features: *const PhysicalDeviceFeatures,
    pub _phantom: PhantomData<&'a ()>,
}

impl<'a> DeviceCreateInfo<'a> {
    pub fn queue_create_infos(mut self, queue_create_infos: &'a [DeviceQueueCreateInfo]) -> Self {
        self.inner.queue_create_info_count = queue_create_infos.len() as _;
        self.inner.p_queue_create_infos = queue_create_infos.as_ptr();
        self
    }
    pub fn enabled_features(mut self, enabled_features: &'a PhysicalDeviceFeatures) -> Self {
        self.inner.p_enabled_features = enabled_features;
        self
    }
}

[Ralith]

I like the consistency of that! Probably saves some effort rewriting the types, too.

[aloucks]

The other thing to consider is whether or not these should be "pure" builders (e.g. take self) or if we want to make them more like setters (that take &mut self). I think I prefer the "setter" pattern with the descriptors. I'm not sure how much I care about the set_ prefix, but it's something else to consider as well.

let mut create_info = DeviceCreateInfo::default();
create_info.some_hypothetical_field = 42;
create_info.set_queue_create_infos(infos);

[Ralith]

My habit is to omit the set_ prefix iff it would be present on ~all methods, since it's not adding information at that point.

I think taking and returning &mut self is a sweet spot:

• Repeated calls are concise and convenient
• Conditional updates aren't awkward
• Vulkan structs are usually passed by reference, so inline use like foo(vk::Bar::default().baz(quux)) should Just Work
• Construction for inline use by-value only requires prefixing a single *.

By comparison, taking self by value actually requires slightly more syntax for inline use, and only slightly less for out-of-line definitions.

[MarijnS95]

I tried the same thing as #286 to not generate the builders at all (and replace the few uses in extensions with a p_next: &mut foo as *mut _ as _) on the current master at 675f91e with Rust 1.56.1, and it only brings compile time for b -p ash down from 10s to exactly 7.5s (roughly the same ~25% improvement as this PR with the cfg). That's less than the gain I was hoping for, especially with the twitter post from #441 (comment) seemingly punting our compile-time deficit on the builders.

I guess we'll pick up most of that time again when moving the lifetimes and useful builder functions directly into the native structs (in whatever way discussed above), making me curious if we're leaving build-time performance on the table elsewhere.

Fwiw removing all manual modules (mod device, entry, instance, extensions) only brings us down to ~7s, but removing all the generated function bodies that call into the function pointers from the table brings it down again to 6.2-6.4s. Bit inconsistent but that's at least a small win if we can change every manual call from device.foo(x, y, z) to (device.foo)(x, y, z)? The only remaining code at this point is the derives on most types, maybe our macros around handles, or the function loaders that are rather significant in size?

[MarijnS95]

Perhaps llvm-lines can help us target what's next. Here's the top output from it:

  Lines          Copies        Function name
  -----          ------        -------------
  264103 (100%)  14260 (100%)  (TOTAL)
   12749 (4.8%)    170 (1.2%)  <*const T as core::fmt::Pointer>::fmt
    5681 (2.2%)    563 (3.9%)  <&T as core::fmt::Debug>::fmt
    4225 (1.6%)      1 (0.0%)  ash::vk::features::DeviceFnV1_0::load
    3745 (1.4%)      1 (0.0%)  ash::vk::const_debugs::<impl core::fmt::Debug for ash::vk::enums::StructureType>::fmt
    3675 (1.4%)     25 (0.2%)  alloc::raw_vec::RawVec<T,A>::allocate_in
    2078 (0.8%)     63 (0.4%)  ash::prelude::<impl ash::vk::enums::Result>::result_with_success
    2045 (0.8%)     17 (0.1%)  ash::prelude::read_into_uninitialized_vector
    1750 (0.7%)     25 (0.2%)  core::alloc::layout::Layout::array
    1705 (0.6%)      1 (0.0%)  ash::vk::const_debugs::<impl core::fmt::Debug for ash::vk::enums::Format>::fmt
    1650 (0.6%)     25 (0.2%)  alloc::raw_vec::RawVec<T,A>::current_memory
    1341 (0.5%)    149 (1.0%)  <*const T as core::fmt::Debug>::fmt
    1256 (0.5%)      8 (0.1%)  alloc::raw_vec::RawVec<T,A>::grow_amortized
    1237 (0.5%)     15 (0.1%)  core::fmt::builders::DebugList::entries
    1224 (0.5%)    153 (1.1%)  core::slice::<impl [T]>::as_ptr
    1035 (0.4%)     74 (0.5%)  core::intrinsics::write_bytes

Dropping all the debug implementations nets us another ~20-25% - together with the full builder removal it's down from 10s to just about 5s on my machine.

Perhaps a good idea to move that all behind a #[cfg_attr(debug, derive(Debug))] etc for the cases where it is useful?

[Ralith]

i.e. #482?

[MarijnS95]

Gah, I should learn to remember all PRs floating around. Neat!

[Ralith]

#602 drafts folding things together. Currently setters are all by-value just because that was the easiest place to start.

[MarijnS95]

@Ralith Do you want to keep this open or shall we aim for landing #602 followed by prototyping the removal of trivial setters as proposed in #441?

[Ralith]

Closing in favor of #602.