Skip to content

Change runtime benchmark definition to always refer to a self-contained function #1470

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Change runtime benchmark definition to always refer to a self-contained function #1470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 58 additions & 12 deletions collector/benchlib/src/benchmark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,24 @@ impl BenchmarkGroup {
}

/// Registers a single benchmark.
/// `benchmark_name` uniquely identifies the benchmark.
/// `constructor` should return a closure that will be benchmarked.
pub fn register<F: Fn() -> Bench + Clone + 'static, R, Bench: FnOnce() -> R + 'static>(
&mut self,
name: &'static str,
benchmark_name: &'static str,
constructor: F,
) {
// We want to type-erase the target `func` by wrapping it in a Box.
let benchmark_func = Box::new(move || benchmark_function(constructor.clone()));
let benchmark_def = BenchmarkWrapper {
func: benchmark_func,
};
if self.benchmarks.insert(name, benchmark_def).is_some() {
panic!("Benchmark {} was registered twice", name);
if self
.benchmarks
.insert(benchmark_name, benchmark_def)
.is_some()
{
panic!("Benchmark {} was registered twice", benchmark_name);
}
}

Expand Down Expand Up @@ -102,19 +107,60 @@ impl BenchmarkGroup {
}

/// Adds a single benchmark to the benchmark group.
/// ```ignore
/// use benchlib::define_benchmark;
///
/// define_benchmark!(group, my_bench, {
/// || do_something()
/// });
/// The first argument should be the `BenchmarkGroup` into which will the benchmark be stored.
/// The second argument should be the name of the benchmark (a string literal).
/// The third argument should be an identifier of a function that will be benchmarked.
///
/// If you do not provide any other arguments, the benchmarked function should not receive any
/// parameters (it can return something though).
///
/// If you provide a fourth argument, it should be an expression `init` that prepares
/// input data for the benchmarked function. The function should then receive a single parameter of
/// the same type as `init`.
/// ```
/// use benchlib::{benchmark_group, define_benchmark};
///
/// fn calc_no_param() -> u32 { 1 + 1 }
///
/// fn calc_param(a: u32) -> u32 {
/// a + 1
/// }
///
/// fn main() {
/// benchmark_group(|group| {
/// define_benchmark!(group, my_bench, calc_no_param);
/// define_benchmark!(group, my_bench, calc_param, 1);
/// });
/// }
/// ```
#[macro_export]
macro_rules! define_benchmark {
($group:expr, $name:ident, $fun:expr) => {
let func = move || $fun;
$group.register(stringify!($name), func);
};
// Version without init
($group:expr, $name:literal, $fun:ident) => {{
let constructor = || {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble with all the nested closures. Are they all necessary? constructor is a zero-arg closure that returns a zero-arg closure. Feels like that could be collapsed into a single closure. And then register wraps the constructor in yet another closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I wanted to achieve this:
You have a constructor, which is a function that returns another function, and that returned function will be benchmarked. You should be able to call the constructor multiple times (e.g. because of wall-time/perf. counter measurement split or simply because we run more iterations). So that's why I need the first closure.

The second closure is needed to apply black_box on the return value of the benchmarked function. Before I could use black_box in the benchmark harness itself, but now that we pass a named function, I can't easily store an arbitrary function in a hashmap, because I couldn't name it's return type. So I apply black_box here directly on whatever the function returns, and then the created closure will just return ().

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, please add more comments to the code explaining all this.

move || {
let ret = $fun();

// Try to avoid optimizing the return value out.
$crate::benchmark::black_box(ret);
}
};
$group.register($name, constructor);
}};
// Version with init
($group:expr, $name:literal, $fun:ident, $init:expr) => {{
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to make this take any number of arguments with $($init:expr),* (or whatever the exact syntax is), like println! and other macros. That would avoid the need for tuples to pass multiple arguments. You might even be able to combine this macro rules with the zero-init one above.

Copy link
Contributor Author

@Kobzol Kobzol Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted to do it, but my macro-fu isn't on par 😆 I can make arbitrary number of arguments work, but I don't know how to avoid the tuples. Normally I would do something like this:

($group:expr, $name:literal, $fun:ident, $($init:expr),*) => {{
        let constructor = || {
             || fun($($init),*);
        };

But in this way the init expression evaluation would be measured by the harness, which I want to avoid. So instead, I'd like to do something like this:

($group:expr, $name:literal, $fun:ident, $($init:expr),*) => {{
        let constructor = || {
             let args = ($($init),*);
             || fun(*args);
        };

With *args meaning: unroll the args tuple into individual arguments. But afaik something like this isn't expressible in Rust. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?

($group:expr, $name:literal, $fun:ident, $($init:expr),*) => {{
        let constructor = || {
             || fun($($init),*);
        };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like the first example that I have sent in my previous commit I think. But in this case the init expression would be evaluated inside the measured closure, which we want to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

It makes me uncomfortable that the evaluation position is so subtle, and not visible from the call sites. E.g. this:

define_benchmark!(group, "nbody_10k", calculate_nbody, nbody::init(10000));

has a very different behaviour to this:

let init = nbody::init(10000);
define_benchmark!(group, "nbody_10k", calculate_nbody, init);

Perhaps the macro syntax could be changed so it doesn't look like a normal call, something like this:

define_benchmark!(group, "nbody_10k", calculate_nbody, delayed_init: init);

It's not great but at least makes it clearer that something non-standard is happening.

But now I'm wondering if the macro is hiding too much subtle stuff :/

let constructor = || {
let init = $init;
move || {
let ret = $fun(init);

// Try to avoid optimizing the return value out.
$crate::benchmark::black_box(ret);
}
};
$group.register($name, constructor);
}};
}

pub use define_benchmark;
Expand Down
3 changes: 3 additions & 0 deletions collector/benchlib/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,6 @@ mod cli;
pub mod comm;
pub mod measure;
pub mod process;

pub use benchmark::black_box;
pub use benchmark::run_benchmark_group;
51 changes: 26 additions & 25 deletions collector/runtime-benchmarks/bufreader/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,37 @@
use std::io::{BufRead, BufReader, Write};
use std::io::{BufRead, BufReader, Cursor, Write};

use snap::{read::FrameDecoder, write::FrameEncoder};

use benchlib::benchmark::{black_box, run_benchmark_group};
use benchlib::define_benchmark;
use benchlib::{black_box, define_benchmark, run_benchmark_group};

const BYTES: usize = 64 * 1024 * 1024;
// Inspired by https://github.com/rust-lang/rust/issues/102727
// The pattern we want is a BufReader which wraps a Read impl where one Read::read call will
// never fill the whole BufReader buffer.
fn read_buf(
mut reader: BufReader<FrameDecoder<Cursor<Vec<u8>>>>,
) -> BufReader<FrameDecoder<Cursor<Vec<u8>>>> {
while let Ok(buf) = reader.fill_buf() {
if buf.is_empty() {
break;
}
black_box(buf);
let len = buf.len();
reader.consume(len);
}
reader
}

fn main() {
// Inspired by https://github.com/rust-lang/rust/issues/102727
// The pattern we want is a BufReader which wraps a Read impl where one Read::read call will
// never fill the whole BufReader buffer.
run_benchmark_group(|group| {
define_benchmark!(group, bufreader_snappy, {
let data = vec![0u8; BYTES];
move || {
let mut compressed = Vec::new();
FrameEncoder::new(&mut compressed)
.write_all(&data[..])
.unwrap();
let mut reader =
BufReader::with_capacity(BYTES, FrameDecoder::new(&compressed[..]));
define_benchmark!(group, "bufreader_snappy", read_buf, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a complex $init value. Is its execution measured as part of the benchmark?

Copy link
Contributor Author

@Kobzol Kobzol Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It shouldn't be, and that is also the point of why the constructor exists. So that I can call the benchmark in a two part way:

  1. Create init data
  2. Start measuring
  3. Pass init data and execute benchmark
  4. Stop measuring

And also ideally, then large init data could be returned from the function and dropped (or mem::forget-ed later). But then we would need to pass the argument by &mut, to make sure that we have control over its destruction.

I think that criterion offers similar tradeoffs by letting users pass data by value or by reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

That all sounds reasonable, it's just not currently clear. More documentation please! :)

const BYTES: usize = 64 * 1024 * 1024;

while let Ok(buf) = reader.fill_buf() {
if buf.is_empty() {
break;
}
black_box(buf);
let len = buf.len();
reader.consume(len);
}
}
let data = vec![0u8; BYTES];
let mut compressed = Vec::new();
FrameEncoder::new(&mut compressed)
.write_all(&data[..])
.unwrap();
BufReader::with_capacity(BYTES, FrameDecoder::new(Cursor::new(compressed)))
});
});
}
31 changes: 19 additions & 12 deletions collector/runtime-benchmarks/hashmap/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,27 @@
use benchlib;
use benchlib::benchmark::run_benchmark_group;
use benchlib::define_benchmark;
use benchlib::{define_benchmark, run_benchmark_group};
use fxhash::FxBuildHasher;
use hashbrown::HashMap;

fn map_insert(
(mut map, count): (HashMap<usize, usize, FxBuildHasher>, usize),
) -> HashMap<usize, usize, FxBuildHasher> {
for index in 0..count {
map.insert(index, index);
}
map
}

fn main() {
run_benchmark_group(|group| {
// Measures how long does it take to insert 10 thousand numbers into a `hashbrown` hashmap.
define_benchmark!(group, hashmap_insert_10k, {
let mut map = hashbrown::HashMap::with_capacity_and_hasher(
define_benchmark!(group, "hashmap_insert_10k", map_insert, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This { isn't necessary? (And the matching } below.)

(
hashbrown::HashMap::with_capacity_and_hasher(
10000,
fxhash::FxBuildHasher::default(),
),
10000,
fxhash::FxBuildHasher::default(),
);
move || {
for index in 0..10000 {
map.insert(index, index);
}
}
)
});
});
}
20 changes: 9 additions & 11 deletions collector/runtime-benchmarks/nbody/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
//! Calculates the N-body simulation.
//! Code taken from https://github.com/prestontw/rust-nbody

use benchlib::benchmark::run_benchmark_group;
use benchlib::define_benchmark;
use benchlib::{define_benchmark, run_benchmark_group};

mod nbody;

fn calculate_nbody(mut body: nbody::BodyStates) -> nbody::BodyStates {
for _ in 0..3 {
body = nbody::compute_forces(body);
}
body
}

fn main() {
run_benchmark_group(|group| {
define_benchmark!(group, nbody_10k, {
let mut nbody_10k = nbody::init(10000);
|| {
for _ in 0..10 {
nbody_10k = nbody::compute_forces(nbody_10k);
}
nbody_10k
}
});
define_benchmark!(group, "nbody_10k", calculate_nbody, nbody::init(10000));
});
}