-
Notifications
You must be signed in to change notification settings - Fork 13
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
chore: upgrade benchmark suite to divan and add phf #8
Conversation
let mut big_set = phf_codegen::Set::<&str>::new(); | ||
const OW_1984: &str = include_str!("data/1984.txt"); | ||
for s in OW_1984 | ||
.split(|c: char| c.is_whitespace()) |
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 good catch. I didn't even thing about there being line breaks in the file. Duh
This is beautiful! I have never heard of I am going to mark this pr
Thanks! |
} | ||
} | ||
|
||
fn args() -> impl Iterator<Item = Input> { |
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.
This is so much cleaner than the nasty, macro cartesian product I was doing before. 👏
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.
Thanks for the feedback! 🫶
I like your suggestion of having this run in a |
The downside of the |
Could it be an option to add the build.rs to the |
We're probably over thinking it. If we had a script that ran the benchmarks and wrote them to a file, we could run that manually in our weekly syncs. We can add an automatic bench marker later if there's a bigger need. |
@johnhurt do you suggest I use this model instead then? https://github.com/beeb/trie-hard/blob/divan-bench-nobuild/benches/bench.rs This would remove the build.rs for now and we have the option to automate later. |
I just merged this into the main branch as part of our weekly sync. Thanks again! I will work on putting in a way to run and publish the benchmarks when we publish ... maybe with some more charts 😂 |
I wanted to compare the performance of
trie-hard
vsphf
but the existing benchmark suite was a bit hard to grasp, and the output was not pretty.I took a bit of time to refactor it using divan and thought it might be a good contribution to the repo. Let me know if it's acceptable or if I need to change anything.
Note that there is no insert benchmark for PHF as it is an immutable data structure generated at compile time.
Ideally, I'd like to have a separate
build.rs
file for the benchmark suite, but I don't think that's something that cargo supports. I could generate the sets inside the bench and remove the build script entirely but that would need updating any time the text files changes, and would only be practical for the "headers" and "small" cases. Let me know if this is preferable!Sample output: