mirage / bechamel

Agnostic benchmark in OCaml (proof-of-concept)
MIT License
44 stars 15 forks source link

Fix how many run the benchmark is allowed to do #12

Closed dinosaure closed 3 years ago

dinosaure commented 3 years ago

Fix #11

lyrm commented 3 years ago

Actually, from what I see in the stats returned by the Benchmark.run function, run may also be the total number of runs done during the overall benchmark, and not the maximum allowed during one turn of the benchmark loop.

So you have two different possible definitions for this value :

I clearly prefer the first option. Anyway, you also need to change the value written in the returned stats record.

And finally, you should also change the name run, since it is the same name used for a function and the metric itself. I would rather call it max_run or limit or anything else that removes this confusion.

dinosaure commented 3 years ago

Yes, I understand the point. Currently, it is not a real fix. I agree to change run (from configuration and stats) to limit which is better. However, I think that this PR is wrong. limit should be the limit of samples produced by the benchmark (independently than run or any others metrics).

Then, the maximum of run can be calculate by:

let rec max_run stats =
  let rec go i acc =
    if i = 0 then acc
    else match stats.sampling with
      | `Geometric v ->
        let n = int_of_float (float_of_int acc *. v) in
        if n > acc + 1 then go (pred i) n else go (pred i) (acc + 1)
      | `Linear k -> go (pred i) (acc + k) in
  go stats.limit stats.start

Should we expose such function instead?

dinosaure commented 3 years ago

After discussion it seems that, at least, max_run is may be needed. But the fix is not the right fix. However, in #10, the renaming of run to limit was done.