Split vk.rs into multiple files #286

aloucks · 2020-04-11T01:10:10Z

This PR updates the generator to produce multiple output files instead one large vk.rs. In addition, the generator can now be run from the project root directory.

The initial motivation for this was that after the latest update to ash, IntelliJ couldn't handle the parsing vk.rs file due to its size exceeding the default threshold. I've verified that both IntelliJ and VSCode + rust-analyzer work as expected with these changes.

Related:
intellij-rust/intellij-rust#5192

A few other benefits to this change include:

It should make it easier to see how changes vk.xml or the generator affect the output, now that its not one huge file.
Ash compiles slightly faster.

Before

$ cargo build --lib
   Compiling cc v1.0.50
   Compiling winapi v0.3.6
   Compiling libloading v0.5.2
   Compiling ash v0.30.0 (D:\projects\ash\ash)
    Finished dev [unoptimized + debuginfo] target(s) in 16.31s

After

$ cargo build --lib
   Compiling cc v1.0.50
   Compiling winapi v0.3.6
   Compiling libloading v0.5.2
   Compiling ash v0.30.0 (D:\projects\ash\ash)
    Finished dev [unoptimized + debuginfo] target(s) in 14.79s

Ralith

Surprising that this is a buildtime win! We could definitely bear to invest more effort there in general; 15 seconds for a bindings crate isn't great.

This'll make it easier to poke around in the generated code in any editor, and I don't see any hazards, so it seems like a clear win to me!

Ralith · 2020-04-11T18:37:27Z

generator/src/lib.rs

+        // #(#extension_code)*
+        // #feature_extensions_code
+        // #const_debugs
+        // #(#aliases)*


I initially left it there for reference and then forgot about it. I've removed it and then also moved all of the file writes down below into a single section to make it more clear what files are being written.

I also rebased in the const enums fix that was merged earlier. Oddly enough I ran into that today too and was going to put in another PR!

MaikKlein · 2020-04-12T10:23:42Z

Thank you for the PR, I'll have a closer look at it later today. I think I am okay with all of this, as I have also been thinking about splitting up vk.rs for a while. Might be a could time to move all of this into a new crate ash-sys.

aloucks · 2020-04-12T17:30:14Z

Might be a could time to move all of this into a new crate ash-sys.

Although I used to be in favor of this, I now think that splitting it out into a separate crate will actually be more of a burden. The generator, bindings, and wrapper are all closely coupled to vk.xml and the wrapper would have to expose the sys crate in it's public API anyway.

Ralith · 2020-04-13T23:15:40Z

Non-demanding users might appreciate skipping potentially a large chunk of ash's considerable build time by omitting the higher level complexities like builders.

aloucks · 2020-04-14T01:39:55Z

I got curious and took out all of the builder code. That shaved off another 3-4 seconds.

Note that I also had to disable the ray tracing and amd extensions (with rather broad strokes) because they depended on the builders.

    Finished dev [unoptimized + debuginfo] target(s) in 11.45s

I have mixed feelings about the builders. They should really be marked as unsafe because they erase lifetimes (I've shot myself in the foot with this before), but they offer a lot of convenience with both array/slices and the structure types.

This got me thinking... The builders are really just a bunch of setters. They shouldn't create that much extra build time. They do, however, all have lifetimes, where are known to balloon build times. The lifetimes are rather useless though because they just convert the slices to raw pointers and then discard them with build. I added the builders back, but removed the lifetimes:

    Finished dev [unoptimized + debuginfo] target(s) in 11.98s

So.. the builders themselves are < 0.5 seconds of compile time. The lifetime in the builders is what what slows things down by another 4 seconds. Considering that the lifetimes don't add any additional safety, I think we should just remove them, but keep the builders overall.

I can submit that change in another PR though if there is interest. I'd prefer not to add it to this one in order to narrow the scope of change.

Ralith · 2020-04-14T01:59:49Z

[Builders] should really be marked as unsafe

No, they should not. They do not perform any unsafe operations whatsoever; constructing a raw pointer from a reference is specifically safe. Vulkan functions that might dereference them are unsafe already.

[builders] erase lifetimes

No, they do not. Preserving lifetime information is a large part of the point of builders, and removing it would make them of questionable value. What erases lifetimes is calling build, which is almost never necessary due to Deref. In the rare cases where it is necessary, such as when you need to construct an array of Vulkan structures, you can reliably avoid problems by remembering one simple rule: defer calling build until you are syntatically inside the function call that consumes the result.

aloucks · 2020-04-14T02:05:21Z

you can reliably avoid problems by remembering one simple rule: defer calling build until you are syntatically inside the function call that consumes the result

I think the point of the borrow system is so you don't have to remember. It's statically enforced by the compiler unless you opt out with unsafe.

What erases lifetimes is calling build, which is almost never necessary due to Deref.

Based on this, I think build should be marked unsafe in its current form. In any case, the usage of deref for the builders goes against the documented intended use:

https://doc.rust-lang.org/std/ops/trait.Deref.html

Implementing Deref for smart pointers makes accessing the data behind them convenient, which is why they implement Deref. On the other hand, the rules regarding Deref and DerefMut were designed specifically to accommodate smart pointers. Because of this, Deref should only be implemented for smart pointers to avoid confusion.

Removing the lifetime from the builders reduces the build time by a non-trivial amount, but like I said, I'm not going to add it to this PR.

Ralith · 2020-04-14T07:25:23Z

I think the point of the borrow system is so you don't have to remember. It's statically enforced by the compiler unless you opt out with unsafe.

In this case, it is merely statically enforced in the overwhelming majority of cases. This is better than statically enforced in zero cases. Still, people who aren't interested in static error checking at that level, it might be useful to disable them entirely.

Based on this, I think build should be marked unsafe in its current form.

That would be an incorrect use of unsafe. There is no way to use build in and of itself, or in combination with any other safe interface, to produce UB. This is exactly analogous to the construction of raw pointers from references in general.

In any case, the usage of deref for the builders goes against the documented intended use:

Nonetheless, it's what we've got. Better solutions welcome.

MaikKlein

Thanks for the PR! I left a few comments.

MaikKlein · 2020-04-15T20:13:59Z

generator/src/bin/generator.rs

@@ -2,5 +2,17 @@ use generator::write_source_code;
 use std::path::Path;

 fn main() {
-    write_source_code(Path::new("Vulkan-Headers/registry/vk.xml"));
+    let cwd = std::env::current_dir().unwrap();
+    let cwd_str = cwd.as_os_str().to_str().unwrap();


Why are you converting it to a str instead of using https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.ends_with ?

MaikKlein · 2020-04-15T20:19:06Z

generator/src/lib.rs

+        #ptr_chain_code
+    };
+
+    let dir = vk_rs.as_ref().parent().unwrap().to_path_buf().join("vk");


It would be simpler to just pass the root dir ash/src instead of ash/src/vk.rs. Then you don't need to call parent.

MaikKlein · 2020-04-15T20:28:35Z

ash/src/vk/prelude.rs

+pub const fn version_patch(version: u32) -> u32 {
+    version & 0xfff
+}
+pub type RROutput = c_ulong;


Those could go into another file platform_definitions.rs or something like that.

MaikKlein · 2020-04-15T20:29:19Z

ash/src/vk/prelude.rs

+#[doc = r" This definition is experimental and won't adhere to semver rules."]
+pub type GgpFrameToken = u32;
+pub type CAMetalLayer = c_void;
+#[macro_export]


The macros you could also put into macros.rs

MaikKlein · 2020-04-15T20:30:30Z

ash/src/vk/prelude.rs

@@ -0,0 +1,254 @@
+use crate::vk::definitions::BaseOutStructure;


I am not super liking the name prelude.rs here. But I also have a hard time coming up with something better. Maybe the rest should move into vk.rs.

MaikKlein · 2020-04-15T20:31:12Z

generator/src/lib.rs

        #(#aliases)*
    };
-    write!(&mut file, "{}", source_code).expect("Unable to write to file");
+
+    let vk_rs_code1 = r#"


Can you give those vars descriptive names like clippy_lints and exports or similar instead of vr_rs_code12?

Generator changes: - No longer convert current dir to a string when checking if the path ends with 'generator' - Pass the 'ash/src' dir instead of the 'vk.rs' path Generated and generated output changes: - The majority of prelude.rs has been moved to macros.rs - The pointer chain and Handle are now included in vk.rs - Platform types has been moved to its own file.

aloucks · 2020-04-15T22:18:28Z

@MaikKlein I didn't like vk/prelude.rs either. It's now gone :) I've addressed all the other comments too.

Ralith reviewed Apr 11, 2020

View reviewed changes

Allow the generator to be run from project root dir

41cff01

aloucks force-pushed the split_vk_rs branch from c32e28c to 68ae05c Compare April 11, 2020 21:43

Split vk.rs into multiple files

fa74369

aloucks force-pushed the split_vk_rs branch from 68ae05c to fa74369 Compare April 11, 2020 21:51

MaikKlein requested changes Apr 15, 2020

View reviewed changes

MaikKlein approved these changes Apr 19, 2020

View reviewed changes

MaikKlein merged commit bd6515c into ash-rs:master Apr 19, 2020

aloucks mentioned this pull request Nov 14, 2021

Make builders optional #483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split vk.rs into multiple files #286

Split vk.rs into multiple files #286

aloucks commented Apr 11, 2020

Ralith left a comment

Ralith Apr 11, 2020

aloucks Apr 11, 2020 •

edited

Loading

MaikKlein commented Apr 12, 2020

aloucks commented Apr 12, 2020

Ralith commented Apr 13, 2020

aloucks commented Apr 14, 2020 •

edited

Loading

Ralith commented Apr 14, 2020

aloucks commented Apr 14, 2020 •

edited

Loading

Ralith commented Apr 14, 2020

MaikKlein left a comment

MaikKlein Apr 15, 2020

MaikKlein Apr 15, 2020

MaikKlein Apr 15, 2020

MaikKlein Apr 15, 2020

MaikKlein Apr 15, 2020

MaikKlein Apr 15, 2020

aloucks commented Apr 15, 2020

		@@ -0,0 +1,254 @@
		use crate::vk::definitions::BaseOutStructure;

Split vk.rs into multiple files #286

Split vk.rs into multiple files #286

Conversation

aloucks commented Apr 11, 2020

Ralith left a comment

Choose a reason for hiding this comment

Ralith Apr 11, 2020

Choose a reason for hiding this comment

aloucks Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

MaikKlein commented Apr 12, 2020

aloucks commented Apr 12, 2020

Ralith commented Apr 13, 2020

aloucks commented Apr 14, 2020 • edited Loading

Ralith commented Apr 14, 2020

aloucks commented Apr 14, 2020 • edited Loading

Ralith commented Apr 14, 2020

MaikKlein left a comment

Choose a reason for hiding this comment

MaikKlein Apr 15, 2020

Choose a reason for hiding this comment

MaikKlein Apr 15, 2020

Choose a reason for hiding this comment

MaikKlein Apr 15, 2020

Choose a reason for hiding this comment

MaikKlein Apr 15, 2020

Choose a reason for hiding this comment

MaikKlein Apr 15, 2020

Choose a reason for hiding this comment

MaikKlein Apr 15, 2020

Choose a reason for hiding this comment

aloucks commented Apr 15, 2020

aloucks Apr 11, 2020 •

edited

Loading

aloucks commented Apr 14, 2020 •

edited

Loading

aloucks commented Apr 14, 2020 •

edited

Loading