mirage / bechamel

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

Suggestion: have measures define their own units #9

Closed craigfe closed 3 years ago

craigfe commented 4 years ago

Currently, the output renderers have the responsibility of assigning units to the measures. For instance:

let () =
  Bechamel_notty.Unit.add Instance.monotonic_clock "ns";
  Bechamel_notty.Unit.add Instance.minor_allocated "words";
  Bechamel_notty.Unit.add Instance.major_allocated "words";
  (* run benchmark *)

I consider this to be backwards: the provider of the monotonic_clock instance is the only one who knows what its units are, as they are providing the get : unit -> float function for this particular measure. I propose adding the following field to the S.MEASURE signature:

val unit : witness -> string option

(Leaving the None case for unit-less quantities like major_collections.) The renderer would then consume measures as { name: string; unit: string option } products (rather than the current string just for the name), and be responsible for rendering that appropriately.

An alternative that gives more flexibility to the renderer would be to have each MEASURE define its own unit type (like [> `Words ] or an extensible variant), which the renderer / user can then supply a to_string function for.

dinosaure commented 4 years ago

I agree that the measure should have its unit - currently, it does not do that due to a separation between the layout logic and the core (I mean, it's useless to know the unit when we benchmark - but it's not when we want to show result). I will make a PR about that next week,

An alternative that gives more flexibility to the renderer would be to have each MEASURE define its own unit type (like [> `Words ] or an extensible variant), which the renderer / user can then supply a to_string function for.

Hmmhmm, it's possible indeed but I think it's not necessary (or too over enginering for a small improvement).