-
Notifications
You must be signed in to change notification settings - Fork 193
Conversation
Results from my machine:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I think we should do better with feature handling; see comments.
.unwrap(); | ||
for file_entry in read_dir { | ||
match file_entry { | ||
Ok(entry) => match entry.path().extension() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this just be:
Ok(entry) => if entry.path().extension() == extension {
...
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require some shenanigans on going from OSStr to str to be able to compare them
#[cfg(not(feature = "wgsl-in"))] | ||
fn gather_modules() -> Vec<naga::Module> { | ||
Vec::new() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little confusing that when necessary features aren't available, we do still run benchmarks and get meaningless numbers. The loop over the modules is inside the b.iter
call, not outside it. It would be clearer to somehow just not run benchmarks that require features we don't have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a bit of an edge case. We don't really care about the numbers when features aren't enabled. What I did was a minimal thing to make it compile for these cases
.collect::<Vec<_>>() | ||
}; | ||
#[cfg(not(feature = "validate"))] | ||
let inputs = Vec::<(naga::Module, naga::valid::ModuleInfo)>::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we shouldn't run benchmarks on empty input lists because a feature was turned off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar reasoning to the last one - this is the minimal change to compile
Thanks for the review! Some of the notes are addressed, other left for follow-ups |
Integrates Criterion and also adds support for bincode-encoded IR form. The purpose here is to see the upper bound for shader loading performance. Today, bincode IR deserialization is roughly 7x faster than WGSL parsing.