ocaml-bench / sandmark

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

Add Owl GC regression test case. #464

Closed tmcgilchrist closed 6 months ago

tmcgilchrist commented 7 months ago

Originally taken from https://gitlab.inria.fr/fangelet/ocaml-gc-regression-examples and supplied by Florian Angeletti (@Octachron). This code was supplied on Performance regression with multiple domains between 5.0 and 5.1.

This test will output timings to stdout of the form 4.14.1 | 2.395s | 1949 | 487 representing compiler version, time, minor GC, major GC. How will that output be captured by sandmark or is there instrumentation already to capture this sort of GC information?

punchagan commented 6 months ago

Thanks for taking the time to contribute this PR!

I've updated this PR to correctly install the dependencies for owl. The problem was that the opam file changes had owl and owl-base version specified incorrectly with the = missing ( "owl" {"1.1"} instead of "owl" {= "1.1"} ). I've also added missing OS-level dependencies. So, the benchmark dependencies are installed, and the code compiles.

But, I've been unable to run the benchmark itself (on navajo).

punchagan@navajo:~/sandmark/_build/5.2.0+trunk_1/benchmarks/owl$ ./owl_gc.exe 
Fatal error: exception Invalid_argument("index out of bounds")

This test will output timings to stdout of the form 4.14.1 | 2.395s | 1949 | 487 representing compiler version, time, minor GC, major GC. How will that output be captured by sandmark or is there instrumentation already to capture this sort of GC information?

We use orun as a wrapper to capture this information. So, you wouldn't need to do anything additionally in the benchmark code.

Octachron commented 6 months ago

Rather than adding empty parameters, I think it is better to remove the reporting part from the benchmark (the last let _ = ...) since sandmark will take care better of this aspect.

punchagan commented 6 months ago

Rather than adding empty parameters, I think it is better to remove the reporting part from the benchmark (the last let _ = ...) since sandmark will take care better of this aspect.

The reporting part from the benchmark can be removed, yes. :+1:

The last commit adds empty params field to the run_config.json, since the sandmark rungen library expects there to be a params field. I'm not sure if you are referring to that change or something else.

Octachron commented 6 months ago

Sorry, I misread the last commit as a fix for the invalid argument error which comes from the report section of the benchmark.

punchagan commented 6 months ago

Sorry, I misread the last commit as a fix for the invalid argument error which comes from the report section of the benchmark.

I've squashed the last commit into the first one; And also removed the reporting part of the benchmark. I think we should be good to merge this, once the github action finishes.

tmcgilchrist commented 6 months ago

Thanks @punchagan