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

generator: Replace Extends{Root} with StructExtends<Root> generic #971

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Collaborator

Works towards #879, yet without swapping the generic and the struct to implement it for.


Instead of emitting a new trait for every Root struct that is being implemented by one or more "child" structs (those that have Root in their structextends), create one trait that takes the root struct as a generic parameter, and implement that directly instead.

This not only saves on having to define the trait for every Root struct but also paves the way towards providing default trait implementations for any pair of root and child struct, such as the p_next builder methods.

@MarijnS95 MarijnS95 requested a review from Ralith December 6, 2024 23:55
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 added this to the Ash 0.39 with Vulkan 1.4 milestone Dec 7, 2024
@MarijnS95 MarijnS95 force-pushed the unify-extends-trait branch 2 times, most recently from 5e377f7 to 8e98f8d Compare December 7, 2024 17:23
ash/src/vk/prelude.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the unify-extends-trait branch from 8e98f8d to 1cf5047 Compare December 8, 2024 11:11
@MarijnS95
Copy link
Collaborator Author

It looks like this PR comes with a ±150ms build speedup:

master (92a0ee6)

$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      7.964 s ±  0.109 s    [User: 7.768 s, System: 0.902 s]
  Range (min … max):    7.797 s …  8.133 s    10 runs

This PR (1cf5047)

$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      7.817 s ±  0.092 s    [User: 7.669 s, System: 0.845 s]
  Range (min … max):    7.707 s …  8.008 s    10 runs

@Ralith
Copy link
Collaborator

Ralith commented Dec 8, 2024

Awesome! Glad we finally stumbled on something that helps there, and great that it's a readability/concision win too.

@MarijnS95
Copy link
Collaborator Author

Sneak-preview: moving the functions to AnyTaggedStructure is going to give us another ±90ms :)

@MarijnS95 MarijnS95 force-pushed the unify-extends-trait branch 2 times, most recently from b2b971f to 28ffd5e Compare December 8, 2024 21:28
Instead of emitting a new `trait` for every `Root` struct that is being
implemented by one or more "child" structs (those that have `Root` in
their `structextends`), create one trait that takes the root struct as a
generic parameter, and implement that directly instead.

This not only saves on having to define the `trait` for every
`Root` struct but also paves the way towards providing default trait
implementations for any pair of root and child struct, such as the
`p_next` builder methods.
@MarijnS95 MarijnS95 force-pushed the unify-extends-trait branch from 28ffd5e to 4305ec7 Compare December 8, 2024 21:37
@MarijnS95 MarijnS95 requested a review from Ralith December 9, 2024 13:35
@MarijnS95
Copy link
Collaborator Author

Performance optimization is now less significant, ±105ms, perhaps because of the expanded lifetimes?

(Retested master too and its performance measurement was within margin of error on my machine)

This PR (4305ec7):

hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      7.858 s ±  0.103 s    [User: 7.702 s, System: 0.876 s]
  Range (min … max):    7.745 s …  8.026 s    10 runs

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

We're done here, right? I'm happy to discuss changing the lifetime semantics to try to recapture some of that buildtime if you want, but it's a net win regardless.

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.

3 participants