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 wasm_of_ocaml benchmarks with current-bench output #1842

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

OlivierNicole
Copy link
Contributor

@vouillon and I (so far, mostly @vouillon) have started to look at ways of improving performance of Wasm code. To keep track of our progress, we need to set up a benchmarking suite that gets run regularly.

This PR makes the repository compatible for monitoring by https://github.com/ocurrent/current-bench/: the benchmark suite is extended with the current-bench output format and I modified the scripts so that Wasm benchmarks are first-class citizens. The existing benchmarking utilities should still work, I ran a few checks to verify that.

This is a first step, we will soon want to add macrobenchmarks as well — presumably by having a benchmarks/sources/macro directory where each subdirectory is a macrobenchmark with its own dune file.

@OlivierNicole OlivierNicole marked this pull request as ready for review February 24, 2025 17:06
@OlivierNicole
Copy link
Contributor Author

Note: this is ready for review but I still need to verify details—namely, how current-bench deals with multiple opam files, and what should be the name of the results file—so we should hold off from merging yet.

@OlivierNicole
Copy link
Contributor Author

Thanks to @punchagan’s advice, this should now hopefully work with current-bench.

@vouillon
Copy link
Member

I think make bench needs to build wasm_of_ocaml. The opam file is here only to install external dependencies.

@vouillon
Copy link
Member

I would put all the results together and grouping them?

{
  "name": "Wasm_of_ocaml",
  "results": [
    {
      "name": "Microbenchmarks",
      "metrics": [
        {
          "name": "microbenchmark/almabench",
          "value": 1.6087265,
          "units": "s"
        },
        {
          "name": "microbenchmark/bdd",
          "value": 0.31883700000000004,
          "units": "s"
        },

@OlivierNicole
Copy link
Contributor Author

This will cause all the run times to appear on one unique graph. I’m not sure we want that?

@vouillon
Copy link
Member

This will cause all the run times to appear on one unique graph. I’m not sure we want that?

Maybe not, actually, since they have very different running times. This is done for Ocaml, but it is not very readable.

We should still put them in the same list of metrics. This will be more readable when we will add other benchmarks.

Maybe we could report the geometric mean of all the running times as well?

@OlivierNicole
Copy link
Contributor Author

I think make bench needs to build wasm_of_ocaml. The opam file is here only to install external dependencies.

Actually, I disagree. When you look at the last line of https://github.com/ocurrent/current-bench/blob/b0528d005b34b8ea1ff4cd7f893165ce782ef5db/pipeline/lib/custom_dockerfile.ml#L78-L96, it also installs regular dependencies.

I have rearranged the metrics list as requested.

Maybe we could report the geometric mean of all the running times as well?

A geometric mean of absolute values? Isn’t the geometric mean usually used on relative evolution numbers?

@OlivierNicole
Copy link
Contributor Author

This repo has been added to the current-bench infra and the results for this PR can be checked here.

@OlivierNicole OlivierNicole force-pushed the wasm-benchmarks branch 15 times, most recently from 9e15f25 to 9087508 Compare March 4, 2025 13:25
RUN sudo apt-get update && \
sudo apt-get install -qq -yy --no-install-recommends pkg-config libgmp-dev \
wget
RUN sudo ln -sf /usr/bin/opam-2.1 /usr/bin/opam
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use an old version of opam ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I took inspiration from https://github.com/ocaml-multicore/picos/blob/main/bench.Dockerfile which does this. I’ll try without this line.

opam exec -- git commit -m dummy && \
opam exec -- git tag -a $(cat VERSION)-dev -m dummy
RUN opam pin -yn --with-version=dev .
RUN opam install -y --depext-only wasm_of_ocaml-bench && \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the purpose of wasm_of_ocaml-bench, why don't we install wasm_of_ocaml-compiler instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-bench’s default Dockerfile needed a dedicated opam file for the benchmarks. But now that we have a custom Dockerfile, I can probably drop it.

@hhugo
Copy link
Member

hhugo commented Mar 4, 2025

I don't understand why we need all the logic in the docker file.
Other open PR seem to fail with

make: *** No rule to make target 'bench'.  Stop.
docker-run failed with exit-code 2

Why isn't it just a matter of added a makefile target ?

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.

3 participants