Runtime benchmarking tweaks

[nnethercote]

I was trying to understand the code better for providing more feedback about #1470, and I ended up doing some refactoring, which is how I best learn how code works :)

Best reviewed one commit at a time. The third commit is the boldest, but I really think the define_benchmarks! macro is confusing and should be removed, because it hides when computations happen. Sticking to vanilla Rust make it much clearer.

These changes are largely orthogonal to #1470. Having named benchmark functions will still be possible. The trickiness with the macro arguments in #1470 goes away. The only downside is that black_box will have to be used in the benchmarks themselves, rather than being done automatically, but I can live with that.

[Kobzol]

The black box thing is maybe not that important, because it's hard to validate that it actually does something, and it would only work when the author of the benchmark remembers to return something calculated from the benchmarked function (otherwise we black box ()).

I mainly wanted to use a macro to achieve better forward compatibility. The runtime benchmark harness is still very much a moving target, and it's probable that it will change a lot. If there will be a lot of benchmarks, it might be annoying to change their interaction with the harness if they call a function. If we use a macro instead, we can probably get away with a lot more changes on the inside without needing to modify the benchmarks themselves. Using a macro also makes the definition more declarative (especially with named functions). Of course, the resulting opaqueness is a disadvantage.

But possibly a bigger issue that I realize now is that I don't see a straightforward way to make this approach work with named functions (https://github.com/rust-lang/rustc-perf/pull/1470). It's not just about passing a named function to register. To enable codegen diff, it would be really useful to get the name of that function from the name of the registered benchmarks (the names might not be the same). With a macro, I was able to stringify! the identifier of the passed function, so that I could store it as a string. I'm not sure if we can do that with a function.

To support functions with and without arguments, we would also probably need two versions of register, but I see that as a smaller issue.

[nnethercote]

If there will be a lot of benchmarks, it might be annoying to change their interaction with the harness if they call a function.

But right now there are only three.

With a macro, I was able to stringify! the identifier of the passed function, so that I could store it as a string. I'm not sure if we can do that with a function.

Can we just pass name and "name"?

The closure-within-a-closure thing is subtle, it took me some time to understand it. So I have a strong preference to not obscure it, because I think that will lead to mistakes, e.g. confusion between the "initialize" and "run" phases.

[Kobzol]

I tried to compare both approaches, while taking into account named identifiers for functions.

1. Macro approach Usage:

// Simple expression
register_benchmark!(group, "hashmap_insert_10k", map_insert, 10000);

// Complex expression
register_benchmark!(group, "hashmap_insert_10k", map_insert, {
    let map = HashMap::new()
    ...
    map
});

Implementation:

// macro
($group:expr, $name:literal, $fun:ident, $init:expr) => {
    let constructor = || {
        let init = $init;
        move || { $fun(init); }
    };
    $group.register_benchmark($name, stringify!($fun), constructor);
};

fn register_benchmark(bench_name, fn_name, fun) {
    // store Box::dyn(fun)
}

2. Function approach Usage:

// Simple expression
group.register_benchmark("hashmap_insert_10k", hashmap_insert, "hashmap_insert", || 10000);

// Complex expression
group.register_benchmark("hashmap_insert_10k", hashmap_insert, "hashmap_insert", || {
    let map = HashMap::new();
    ...
    map
});

Implementation

fn register_benchmark(bench_name, func, fn_name, init) {
    let fun = || {
       let init = init();
       move || { func(init) }
    };
    // store Box::dyn(fun)
}

It seems to me that by requiring the function to be a named identifier, we make the usage of your function implementation more complex - the complexity actually comes from the function being passed as a named identifier, not from the macro itself. So it's true that this PR is simpler, but if we added #1470 here, it wouldn't be much better than the macro, while requiring us to repeat the function name and with worse forward compatibility.

The nested closure is required if we want to be able to repeat the benchmark. Before, the users themselves have created the nested closure in the benchmark definition. Now we move the nested closure from the benchmark definition to the macro/register_benchmark function.

[nnethercote]

With the function approach, do you really need both "hashmap_insert_10k" and "hashmap_insert"?

Anyway, you are the one doing most of the implementation, so if you really want the macro, I will accept that, so long as it's well documented exactly when the initialization runs vs. when the benchmark runs.

Are you happy with the first two commits in this PR?

[Kobzol]

With the function approach, do you really need both "hashmap_insert_10k" and "hashmap_insert"?

The original idea behind the named identifier was to disentangle the function being benchmarked (so that we have it as a separate entity and we can do e.g. codegen diff) and the specific benchmarked inputs. So we could have a function that inserts data into a hashmap, and try it in different situations (100 elements, 10k elements, inserting different values etc.). For this I think that we need the two names.

But to be honest, I'm not even sure if codegen diff is a feature that would be useful or if it would work practically. As you said, right now we have three benchmarks, so maybe it's not needed to make such complicated changes to benchmark registration for now.

Let's merge this whole PR now and postpone #1470. Unless we suddenly get an influx of tens of benchmarks, it shouldn't be difficult to change the benchmark definitions later.

In the meantime, I'll try to get codegen diff working. If we find that it's usable, would be useful for rustc developers, and that it requires named identifiers, we can revisit this. Otherwise we might not even have the named function requirement, which complicates the code both for the function and macro approaches.

[nnethercote]

Thanks for being flexible. I agree revisiting benchmark registration is a good idea if/when we need to.