search

ocaml-bench / sandmark

A benchmark suite for the OCaml compiler
The Unlicense
82 stars 40 forks source link

Remove macro_bench tag on select benches #389

Closed ElectreAAS closed 2 years ago

ElectreAAS commented 2 years ago

This resolves #348 in the simple way: removing the macro_bench tag from the benches mentionned in the issue.

shakthimaan commented 2 years ago

LGTM!

kayceesrk commented 2 years ago

This fix is wrong. As I had mentioned elsewhere, a number of problematic benchmarks mentioned in #348 have been "fixed" to run longer. This PR removes ones that run longer as well. Did you actually check the latest running time for the mentioned benchmarks and only remove those which ran quicker than 1s (the limit for macro benchmarks)?

Consider a recent sandmark nightly run on turing:

image

The benchmarks that are declassifed as being macro benchmark by this PR are: yojson_ydump, setrip, floyd_warshall, zarith_pi, durand-kerner-aberth, soli, hamming.

Of these, zarith_pi, durand-kerner-aberth, soli, hamming, floyd_warshall all run longer than 1s in the latest run! Only setrip runs slower than 1s. I would revert this PR to only remove setrip for now.

I would recommend reverting this PR and only removing setrip (or ideally, make it run longer).

In general, removing a benchmark from being macro benchmark is an easy task. But adding new ones is extremely hard. So I would be doubly careful if you are relegating something from not being a macro benchmark. Removing benchmarks from the suite adds blind spots to proposed compiler improvements.