Wrong benchmarks in library/core/benches/char/methods.rs and possibly other places

[safinaskar]

As I learned from https://gendignoux.com/blog/2022/01/31/rust-benchmarks.html#application-to-real-benchmarks file library/core/benches/char/methods.rs and possibly other files in this repo contain wrong benchmarks.

As author of the article points, call c.to_digit(2) optimizes to None and thus doesn't get benchmarked.

So, please, find all such places and fix them. The faulty example still present in latest master

[chenyukang]

@rustbot claim

[gendx]

Author of https://gendignoux.com/blog/2022/01/31/rust-benchmarks.html here.

Apologies for not having reported a bug here as I didn't have the bandwidth to follow-up - but I'm glad that someone else picked it up! Thanks @safinaskar and @chenyukang 🙏

---

Beyond this specific example, I think one broader problem (as I mentioned in my conclusion) is that the "box" name misleadingly implies that it "contains" a computation put into it, i.e. that in the following example (close to the Bencher API) the function f will not be optimized (as it's "inside the box").

loop {
    black_box(f());
}

In reality, the "box" is rather a barrier/fence, and only "contains" a value (the result of evaluating f), so the compiler can optimize the computation as follows:

loop {
    let x = f();
    black_box(x);
}

or even:

let x = f();
loop {
    black_box(x);
}

In fact, if the compiler is perfect, we want it to optimize the computation of f outside of the loop (assuming f is a pure function with no side effects) - otherwise there is room to improve the compiler's optimizations.

Back to the Bencher interface, it'd IMO make more sense to think of benchmarking a function f as the transition between f's input(s) and output(s), rather than black_box-ing the result of an input-less function. The API could look like this:

impl Bencher {
    /// Callback for benchmark functions to run in their body.
    pub fn iter<I, O, F>(&mut self, input: I, mut inner: F)
    where
        I: Copy,
        F: FnMut(I) -> O,
    {
        // Omitting the measurements, statistics, etc.
        loop {
            black_box(f(black_box(input)))
        }
    }

On the other hand, the current Bencher interface puts an automatic black_box on the output, but nothing on the input, so any benchmark that doesn't put a manual black_box on the inputs is prone to measure the wrong thing (or hope the compiler won't optimize the function because it exceeded some threshold for inlining optimization).

---

I don't really have the bandwidth to propose an RFC at the moment, but leaving my thoughts here in case someone wants to pick up this idea.

[safinaskar]

@chenyukang , @gendx . The fix is completely wrong. Please, reopen the bug.

First, the patch doesn't fix all bugs. For example, the article also mentions file library/core/benches/num/dec2flt/mod.rs , it is not fixed. So, please review all benches in whole repo.

Second, patched files are patched wrong. You need to pass inputs through black_box and also you need to pass outputs to black_box or to Bencher::iter. I. e. both inputs and outputs should be prevented from optimization. Now let's consider library/core/benches/num/flt2dec/strategy/dragon.rs after patch:

rust/library/core/benches/num/flt2dec/strategy/dragon.rs

Line 11 in fe84cec

format_shortest(black_box(&decoded), &mut buf);

. As you can see, ; prevents result of computation from passing to Bencher::iter. So ; should be removed:

     b.iter(|| {
-        format_shortest(black_box(&decoded), &mut buf);
+        format_shortest(black_box(&decoded), &mut buf)
     });

Also, I think some lint should be added to clippy, which will prevent FnMut() -> () from passing to Bencher::iter

[chenyukang]

@chenyukang , @gendx . The fix is completely wrong. Please, reopen the bug.

First, the patch doesn't fix all bugs. For example, the article also mentions file library/core/benches/num/dec2flt/mod.rs , it is not fixed. So, please review all benches in whole repo.

Second, patched files are patched wrong. You need to pass inputs through black_box and also you need to pass outputs to black_box or to Bencher::iter. I. e. both inputs and outputs should be prevented from optimization. Now let's consider library/core/benches/num/flt2dec/strategy/dragon.rs after patch:

rust/library/core/benches/num/flt2dec/strategy/dragon.rs

Line 11 in fe84cec

format_shortest(black_box(&decoded), &mut buf);

. As you can see, ; prevents result of computation from passing to Bencher::iter. So ; should be removed:

     b.iter(|| {
-        format_shortest(black_box(&decoded), &mut buf);
+        format_shortest(black_box(&decoded), &mut buf)
     });

Also, I think some lint should be added to clippy, which will prevent FnMut() -> () from passing to Bencher::iter

Thanks for correcting!
I will submit a follow-up patch for it.

[chenyukang]

Not all bench mark test should return a value in the block, the change:

     b.iter(|| {
-        format_shortest(black_box(&decoded), &mut buf);
+        format_shortest(black_box(&decoded), &mut buf)
     });

will be rejected by compiler:

  --> library/core/benches/num/flt2dec/strategy/grisu.rs:18:9
   |
16 |     let mut buf = [MaybeUninit::new(0); MAX_SIG_DIGITS];
   |         ------- variable defined here
17 |     b.iter(|| {
   |             - inferred to be a `FnMut` closure
18 |         format_shortest(black_box(&decoded), &mut buf)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---^
   |         |                                         |
   |         |                                         variable captured here
   |         returns a reference to a captured variable which escapes the closure body
   |
   = note: `FnMut` closures only have access to their captured variables while they are executing...
   = note: ...therefore, they cannot allow references to captured variables to escape

And this test will write the result to &mut buf, it should not be optimized without writing? 🤔

Another common scenario is we loop in the block of iter, so we will return ():

  b.iter(|| {
       for i in 1..Nums {
           ...blah
       }
  });

Anyway, benchmark testing do need some careful tweaks considering black_box and optimizations in compiler.

[safinaskar]

@chenyukang , I propose wrap result in black_box in both cases