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

Replace builders with lifetimes/setters directly on Vulkan structs #602

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Mar 24, 2022

Based on prior discussion in #441 and #483.

Remarkably, this doesn't seem to significantly impact build times despite deleting on the order of 20k(!) lines of code. Arguably still an improvement for the sake of simplicity and safety, though. Could use confirmation on the build impact by someone with a better test environment.

TODO: Try making all the setters by &mut rather than by value, per discussion in #483.

@Ralith Ralith force-pushed the direct-setters branch 4 times, most recently from a9969c9 to 90b4cd0 Compare March 24, 2022 06:31
@MarijnS95
Copy link
Collaborator

MarijnS95 commented Mar 29, 2022

Remarkably, this doesn't seem to significantly impact build times

This may have been the #[inline] change pushed later, but this PR in its current form nets me ~1.3 seconds:

$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      7.113 s ±  0.062 s    [User: 7.927 s, System: 0.405 s]
  Range (min … max):    7.053 s …  7.244 s    10 runs
$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      5.812 s ±  0.036 s    [User: 6.644 s, System: 0.356 s]
  Range (min … max):    5.725 s …  5.862 s    10 runs

I recall discussing earlier that the builder structs themselves likely weren't the problem, but it was down to lifetime propagation on the setter methods. Can't find that discussion anymore, but I'd love to get this in and replace trivial setters with just member updates or struct initialization, as previously discussed and proposed in #441.

EDIT: Keeping in mind the twitter link at #441 (comment), would be nice to also drop setters for pointers (Rust borrows with a proper lifetime now) and "trivial slices" with a length (unique!) + pointer pair.

generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +1714 to +1720
let lifetime = has_lifetimes
.contains(&name_to_tokens(&field.basetype))
.then(|| quote!(<'a>));

Some(quote!{
pub fn #param_ident_short(mut self, #param_ident_short: #param_ty_tokens) -> Self {
self.inner.#param_ident = #param_ident_short;
#[inline]
pub fn #param_ident_short(mut self, #param_ident_short: #param_ty_tokens #lifetime) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we can do inside construction above:

let param_ty_tokens = field.safe_type_tokens(quote!('a));

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not trivially, I don't think, since it needs the has_lifetimes state.

@Ralith
Copy link
Collaborator Author

Ralith commented Mar 29, 2022

This may have been the #[inline] change pushed later, but this PR in its current form nets me ~1.3 seconds:

Nice! Can you verify whether it was the inlining or not? If so, we definitely can/should pursue it further. I don't expect that to have mattered, since I believe codegen still occurs for inline stuff (so it can only add work), but there's some conjecture there.

EDIT: Keeping in mind the twitter link at #441 (comment), would be nice to also drop setters for pointers (Rust borrows with a proper lifetime now) and "trivial slices" with a length (unique!) + pointer pair.

Per previous discussion I don't believe it's possible to avoid setters for any slice case due to ABI concerns. I'm also a bit leery of requiring a mixture of different approaches to initialize a struct; let's leave that for future work.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Mar 29, 2022

$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      6.251 s ±  0.032 s    [User: 7.082 s, System: 0.387 s]
  Range (min … max):    6.176 s …  6.295 s    10 runs

Seems to be both, with your initial approach netting about ~800ms and the #[inline] addition another ~400ms.

Per previous discussion I don't believe it's possible to avoid setters for any slice case due to ABI concerns. I'm also a bit leery of requiring a mixture of different approaches to initialize a struct; let's leave that for future work.

Whoops, I meant to write the VulkanSlice1 wrapper type. Still needs a setter and/or Into/new() construction pattern, but could also expose a safe iterator. Perhaps that type is a bit neater to initialize and use than having a pointer_to_first_element: &'a vk::Something (or doing away with lifetime safety and keeping it *const). Let's indeed leave both the discussion and implementation for a different place though ^^

@Ralith
Copy link
Collaborator Author

Ralith commented Mar 29, 2022

Seems to be both, with your initial approach netting about ~800ms and the #[inline] addition another ~400ms.

Interesting! Most ash functions should objectively be inlined, so this would be good to explore further.

@MarijnS95 MarijnS95 merged commit 71bb3d3 into ash-rs:master Mar 29, 2022
MarijnS95 added a commit that referenced this pull request Apr 13, 2023
 #602 introduced builder functions directly on the raw Vulkan struct
types by using lifetime borrows which are FFI compatible (ABI is
identical) wuth raw pointers, simplifying the whole system and
protecting the user against losing lifetimes upon calling `.build()`.
However, this change wasn't propagated through to the `README` so the
code snippets were still showcasing removed `::builder()` and `.build()`
functions and documenting "the `.build()` footgun" which doesn't even
exist anymore 🎉
MarijnS95 added a commit that referenced this pull request Apr 14, 2023
 #602 introduced builder functions directly on the raw Vulkan struct
types by using lifetime borrows which are FFI compatible (ABI is
identical) wuth raw pointers, simplifying the whole system and
protecting the user against losing lifetimes upon calling `.build()`.
However, this change wasn't propagated through to the `README` so the
code snippets were still showcasing removed `::builder()` and `.build()`
functions and documenting "the `.build()` footgun" which doesn't even
exist anymore 🎉
MarijnS95 added a commit that referenced this pull request Apr 15, 2023
 #602 introduced builder functions directly on the raw Vulkan struct
types by using lifetime borrows which are FFI compatible (ABI is
identical) wuth raw pointers, simplifying the whole system and
protecting the user against losing lifetimes upon calling `.build()`.
However, this change wasn't propagated through to the `README` so the
code snippets were still showcasing removed `::builder()` and `.build()`
functions and documenting "the `.build()` footgun" which doesn't even
exist anymore 🎉
MarijnS95 added a commit that referenced this pull request Apr 16, 2023
 #602 introduced builder functions directly on the raw Vulkan struct
types by using lifetime borrows which are FFI compatible (ABI is
identical) wuth raw pointers, simplifying the whole system and
protecting the user against losing lifetimes upon calling `.build()`.
However, this change wasn't propagated through to the `README` so the
code snippets were still showcasing removed `::builder()` and `.build()`
functions and documenting "the `.build()` footgun" which doesn't even
exist anymore 🎉
MarijnS95 added a commit that referenced this pull request Apr 16, 2023
* README: Autoformat

* README: Remove deprecated `builder()` snippets and guidelines

 #602 introduced builder functions directly on the raw Vulkan struct
types by using lifetime borrows which are FFI compatible (ABI is
identical) wuth raw pointers, simplifying the whole system and
protecting the user against losing lifetimes upon calling `.build()`.
However, this change wasn't propagated through to the `README` so the
code snippets were still showcasing removed `::builder()` and `.build()`
functions and documenting "the `.build()` footgun" which doesn't even
exist anymore 🎉
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants