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

chore(bench): add throughput for integer benchmarks #1741

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

soonum
Copy link
Contributor

@soonum soonum commented Oct 31, 2024

No description provided.

@soonum soonum self-assigned this Oct 31, 2024
@cla-bot cla-bot bot added the cla-signed label Oct 31, 2024
Comment on lines +386 to +392
/// Generate a number of threads to use to saturate current machine for throughput measurements.
pub fn throughput_num_threads() -> u64 {
let num_threads = rayon::current_num_threads() as f64;
// Add 20% more to maximum threads available.
(num_threads + (num_threads * 0.2)) as u64
}
Copy link
Member

Choose a reason for hiding this comment

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

unclear this is enough, check with @tmontaigu probably

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes its unclear to know if its enough, for some ops it might for others not, or it could be enough for all,

I think a manual check needs to be done by running and trying different values and see if the throughput drastically changes

Comment on lines +151 to +149
bench_id = format!("{bench_name}::throughput::{param_name}::{bit_size}_bits");
let elements = throughput_num_threads();
bench_group.throughput(Throughput::Elements(elements));
bench_group.bench_function(&bench_id, |b| {
Copy link
Member

Choose a reason for hiding this comment

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

unclear to me if this is the right way to call the throughput, did you confirm it works as expected ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it works as expected, at least on my laptop. We'll need to double check on target machine to see if results are coherent.

@agnesLeroy
Copy link
Contributor

We'll need also to add throughput benches for the GPU 🙂

@soonum
Copy link
Contributor Author

soonum commented Oct 31, 2024

We'll need also to add throughput benches for the GPU 🙂

Yes for sure, I just wanted to check for the design before deploying to all the functions.

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.

4 participants