Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Owl GC regression test case. #464

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

tmcgilchrist
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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. 👍

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
Copy link

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

@punchagan punchagan force-pushed the owl_gc_regression branch 2 times, most recently from cd0fba9 to b334554 Compare January 31, 2024 13:09
@punchagan
Copy link
Contributor

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.

@punchagan punchagan merged commit ac6e5a0 into ocaml-bench:main Feb 1, 2024
4 checks passed
@tmcgilchrist tmcgilchrist deleted the owl_gc_regression branch February 1, 2024 06:19
@tmcgilchrist
Copy link
Contributor Author

Thanks @punchagan

punchagan added a commit to punchagan/sandmark-nightly-config that referenced this pull request Feb 6, 2024
@tmcgilchrist added an Owl GC regression test case in ocaml-bench/sandmark#464
and the results for 5.0 and 5.2 versions are required for a comparison.
punchagan added a commit to ocaml-bench/sandmark-nightly-config that referenced this pull request Feb 6, 2024
@tmcgilchrist added an Owl GC regression test case in ocaml-bench/sandmark#464
and the results for 5.0 and 5.2 versions are required for a comparison.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants