ocaml-bench / sandmark

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

Using eventring in pausetimes benchmarks instead of outdated eventlog #390

Closed ElectreAAS closed 1 year ago

ElectreAAS commented 2 years ago

This PR aims to resolve #358, by removing calls to eventlog-based tools that don't work with OCaml 5.0 with eventring, and in particular its wrapper olly. This PR requires first merging my PR on runtime_events_tools to work properly.

ElectreAAS commented 2 years ago

I think it would be easier for anyone reading about this to keep the discussion on the original issue

ElectreAAS commented 2 years ago

I tested the current state of the branch on navajo and it seems to work

shakthimaan commented 2 years ago

Can you please update the PR on how to run and view the latency measurements in the Running benchmarks section in the README?

Firobe commented 1 year ago

I've revived this branch and it should now be ready for review (modulo the CI).

There are a number of "hacks" to make the installation of runtime_events_tools (olly) possible, since it needs dune > 3 (for the ctypes extension):

I've also removed the pausetimes/pausetimes_trunk script since it's apparently not needed anymore (see original issue), and updated the README with correct instructions.

Note that pausetimes will only work for 5.1.0*, currently

shakthimaan commented 1 year ago

Thanks for the changes. We need to support 4.14 and 5.0.0 development branches as well and hence the SYS_DUNE_HACK is used. When I test for 5.0.0~alpha0 using the following commands, I get a package conflict resolution error.

$ make clean; TAG='"run_in_ci"' make run_config_filtered.json
$ OPT_WAIT=0 USE_SYS_DUNE_HACK=1 RUN_CONFIG_JSON=run_config_filtered.json make ocaml-versions/5.0.0~alpha0.bench

We can install dune 3.5 on the local switch, and proceed with the existing workflow. Would it be possible to gate the necessary steps for 5.1.0 in a case statement, so that it does not get triggered for the other development branches?

Firobe commented 1 year ago

Right, the errors were triggered by a spurious version change in an unrelated package, this should now be fixed.

We need to support 4.14 and 5.0.0 development branches as well and hence the SYS_DUNE_HACK is used. When I test for 5.0.0~alpha0 using the following commands, I get a package conflict resolution error.

Do you mean that:

kayceesrk commented 1 year ago

I don't think we need pausetimes in 4.14. It is a lot of work to revive pause times -- eventlog has in 4.14 compiler has been replaced by runtime events. I don't think we should invest any time in dead features. We should be aiming to do very little or no work on 4.14. Out of curiosity, why do we care about 4.14?

In order to review this PR, and other similar ones in sandmark, is there a test deployment or instructions for deploying locally? I can use it to understand how the feature works. Easier for me to see the whole thing in action than reading the code.

shakthimaan commented 1 year ago

5.0.0 and 4.14 should work like before (without pausetimes)? (this should be the case now)

Yes, but, only for 5.0.0 as KC mentioned. I can confirm that the build is clean now for 5.0.0~alpha0.

We should be aiming to do very little or no work on 4.14.

Okay.

is there a test deployment or instructions for deploying locally?

The following commands help produce the pausetimes results:

$ make clean; TAG='"run_in_ci"' make run_config_filtered.json
$ OPT_WAIT=0 RUN_BENCH_TARGET=run_pausetimes RUN_CONFIG_JSON=run_config_filtered.json make ocaml-versions/5.1.0+trunk.bench 

Sample output:

{"version":"5.4.0-131-generic","hostname":"godel","kernel":"Linux","arch":"x86_64"}
{
  "name": "revcomp2.",
  "mean_latency": 1,
  "max_latency": 1,
  "distr_latency": [
    0.560639,
    0.863231,
    1.579007,
    1.604607,
    1.620991,
    1.639423,
    1.652735,
    1.665023,
    1.677311,
    1.683455,
    1.687551,
    1.697791,
    1.709055,
    1.739775,
    1.832959,
    1.832959,
    1.832959,
    1.832959
  ]
}
...
Firobe commented 1 year ago

Okay, the build should now be clean for every version, and run_pausetimes should work for all versions >= 5.

kayceesrk commented 1 year ago

Don't we also need a new notebook to visualise the pause times? Such a thing used to exist in the sequential and parallel notebooks earlier: https://github.com/ocaml-bench/sandmark/blob/main/notebooks/sequential/sequential.ipynb towards the end. Observe that the sequential notebook says Throughput at the top. There used to be a Latency section towards the end. If you go back in history towards March or April 2020, you should see Latency section in the sequential and parallel notebooks.

Rather than delay merging this PR, I would recommend merging this in, then starting to run these in the nightly runs first. We can have a separate sequential and parallel latency pages separately. (IIRC sandmark nightly also had pausetimes support earlier, which has been removed).

shakthimaan commented 1 year ago

Don't we also need a new notebook to visualise the pause times?

Yes.

Such a thing used to exist in the sequential and parallel notebooks earlier

Yes, at https://github.com/ocaml-bench/sandmark/blob/429f52f7b0ea9dbe760b3e249a17cd0b7b629e95/notebooks/sequential/sequential.ipynb - Latency, Max latency and 99.9th percentile latency.

I would recommend merging this in, then starting to run these in the nightly runs first.

Sure!