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

include benches for crc crate software implementation to compare against crc32c #45

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

Conversation

rukai
Copy link

@rukai rukai commented Sep 7, 2023

Update benchmarks to include comparisons against the more generic and purely software implementation from the crc crate.

I refactored the way benchmark_groups were named as they were previously creating redundant names like: crc32_update_megabytes/crc32_update_megabytes
Now they are named like 1mb/crc32c
This also sets us up to name the new benchmarks like 1mb/crc_crate

I also noticed that the definition of 1 megabyte was not consistent with the definition used for kilobyte.
So I changed all megabyte benches to use 1024 * 1024 instead of 1000 * 1000.

All the original benches have functionally equivalent benches just under new names.

I removed:

[target.aarch64-unknown-linux-gnu]
linker = "aarch64-linux-gnu-gcc"

Because it was causing a warning in Cargo.toml due to being ignored and having no effect.
I think this sort of config is supposed to go in .cargo/config.toml would you rather me move it there instead of delete it?

@zowens
Copy link
Owner

zowens commented Sep 13, 2023

Can you speak to the reason we will be benching against another crate? I'm skeptical of deeply embedding a separate crate into the benchmarks.

@rukai
Copy link
Author

rukai commented Sep 13, 2023

The main draw of this crate is the higher performance other over crates.
If this crate werent actually faster I would be better off just using the crc crate as that is more flexible.
So as a user I want to be able to actually compare the two and go "yep crc32c faster"

I can understand your reasoning though, I would be happy to host the proposed changes in a separate repo and then link to it in the crc32c repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants