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

perf: add synthetic benchmark #7189

Merged
merged 12 commits into from
Feb 28, 2023

Conversation

jchavarri
Copy link
Collaborator

@jchavarri jchavarri commented Feb 26, 2023

Description

  1. Adds some helper in bench to generate synthetic projects:
dune exec ../bench/gen_synthetic.exe -- -n 4 test

Will create a folder test with:

  • a library with 4*4 dirs with 4*4 modules each. Each module includes mli
  • an exe that uses the library

Based on https://github.com/rescript-lang/build-benchmark.

  1. Configures ci to include results from a synth benchmark with n = 6.

Results

Check newly added chart "Synthetic benchmark" in https://jchavarri.github.io/dune/dev/bench/. Note the first commit in the chart had too short build time due to dune clean not being called properly.

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri self-assigned this Feb 26, 2023
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri force-pushed the bench/add-synthetic-benchmark branch from 52bd596 to 48db2dd Compare February 27, 2023 11:30
@jchavarri
Copy link
Collaborator Author

jchavarri commented Feb 27, 2023

@rgrinberg I am trying to add this new benchmark with a synthetic project, but dune seems to choke when building it, see sample build (already running for 2+ hours): https://github.com/jchavarri/dune/actions/runs/4281888888/jobs/7455464113

One can replicate locally following these steps, assuming you have the branch checked out:

cd bench
dune exec ./gen_synthetic.exe -- -n 6 test
dune exec ./test/main.exe

It remains stuck on Saving digest db... and never gets over that step. Do you have any hints what could be going on?

@jchavarri
Copy link
Collaborator Author

jchavarri commented Feb 27, 2023

These are the last syscalls that i can see before killing the process, produced with strace -rT -o strace.txt _build/default/bin/main.exe exec ./bench/test/main.exe:

     0.000103 brk(NULL)                 = 0x55fe7d9e6000 <0.000010>
     0.000030 brk(0x55fe7da07000)       = 0x55fe7da07000 <0.000010>
     0.000088 mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f9bf3f23000 <0.000010>
     0.000180 mmap(NULL, 2105344, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f9bf3d21000 <0.000010>
     0.000036 mmap(NULL, 1024000, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f9bf3c27000 <0.000010>
     0.000048 sigaltstack({ss_sp=0x55fe7d9fd480, ss_flags=0, ss_size=8192}, NULL) = 0 <0.000011>
     0.000037 rt_sigaction(SIGSEGV, {sa_handler=0x55fe7bcf5b30, sa_mask=[], sa_flags=SA_RESTORER|SA_ONSTACK|SA_NODEFER|SA_SIGINFO, sa_restorer=0x7f9bf414e140}, NULL, 8) = 0 <0.000012>
     0.000041 readlink("/proc/self/exe", "/home/me/code/dune/_build/defaul"..., 256) = 53 <0.000031>
     0.000084 stat("/home/me/code/dune/_build/default/bench/test/main.exe", {st_mode=S_IFREG|0555, st_size=4372552, ...}) = 0 <0.000021>
     0.000059 brk(0x55fe7da30000)       = 0x55fe7da30000 <0.000011>
     0.000036 lseek(0, 0, SEEK_CUR)     = -1 ESPIPE (Illegal seek) <0.000010>
     0.000033 mmap(NULL, 794624, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f9bf3b65000 <0.000010>
     0.000032 lseek(1, 0, SEEK_CUR)     = -1 ESPIPE (Illegal seek) <0.000009>
     0.000030 lseek(2, 0, SEEK_CUR)     = -1 ESPIPE (Illegal seek) <0.000011>
    10.531237 --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
     0.000497 +++ killed by SIGINT +++

@jchavarri
Copy link
Collaborator Author

jchavarri commented Feb 27, 2023

it seems an issue with too long args for linker. Edit: this was not the case.

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Copy link
Collaborator Author

This is ready to review now.

I did not manage to solve the issue with dune hanging when trying to execute the binary, but I ended up using dune build which works and is actually better because we want to track build performance, not execution. I will open a separate ticket for that mysterious problem.

The new benchmark results can be seen in https://jchavarri.github.io/dune/dev/bench/. Note the first commit in the chart had too short build time due to dune clean not being called properly.

The only remaining questions would be about:

  • is the current amount of modules in the synthetic benchmark (1296) enough? or having a project with more modules would be desirable for some reason?
  • it is possible to measure incremental build times in a separate benchmark, would this be interesting to measure somehow?

@jchavarri jchavarri marked this pull request as ready for review February 27, 2023 22:08
@jchavarri jchavarri requested a review from rgrinberg February 27, 2023 22:08
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@rgrinberg
Copy link
Member

Do you think we can simplify the script a little?

  1. I don't think we need multiple libraries. You should be able to show the regression with 1 lib
  2. A single directory should be enough as well

@rgrinberg
Copy link
Member

In summary, the only parameter we should need is the number of modules to generate

Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri force-pushed the bench/add-synthetic-benchmark branch from 839b0ab to 7de1cbd Compare February 28, 2023 17:49
@jchavarri
Copy link
Collaborator Author

Simplified gen_synthetic in 7de1cbd.

I only removed the subfolders, because the generated code was only using one library already.

bench/gen_synthetic.ml Outdated Show resolved Hide resolved
bench/gen_synthetic.ml Outdated Show resolved Hide resolved
bench/gen_synthetic.ml Outdated Show resolved Hide resolved
bench/gen_synthetic.ml Outdated Show resolved Hide resolved

- name: Run synthetic benchmark
working-directory: bench/synthetic
run: ../gen-benchmark.sh 'opam exec -- ../../_boot/dune.exe build ./main.exe' 'opam exec -- ../../_boot/dune.exe clean' 'synthetic build time (${{ runner.os }})' > synthetic-benchmark-result.json
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 run clean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Otherwise you don't get the "full picture" of the build. But we could go the other way: prepare the build first by running once the build, and only measure how long it takes to build again. Would that be better?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure which one is better. It's fine as is for now.

bench/gen_synthetic.ml Outdated Show resolved Hide resolved
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri force-pushed the bench/add-synthetic-benchmark branch from a51c384 to 98c759b Compare February 28, 2023 19:26
@rgrinberg rgrinberg merged commit f36109e into ocaml:main Feb 28, 2023
@jchavarri jchavarri deleted the bench/add-synthetic-benchmark branch February 28, 2023 20:01
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 15, 2023
* main: (56 commits)
  feature: add terminal ui backend based on NoTTY (ocaml#6996)
  doc(coq): update documentation about coqdep
  fix(rules): don't descend into automatic subdirs infinitely (ocaml#7208)
  benchmark: add warm run (ocaml#7198)
  test: vendored and public libs (ocaml#7197)
  test: use sh in concurrent test (ocaml#7205)
  fix: custom log file path (ocaml#7200)
  test(melange): add test exercising ocaml#7104 (ocaml#7204)
  test(melange): add a test that introduces rules in the target dir (ocaml#7196)
  test: duplicate packages in vendor dir (ocaml#7194)
  melange: interpret `melc --where` as a list of `:`-separated paths (ocaml#7176)
  perf: add synthetic benchmark (ocaml#7189)
  Test case for bug report (ocaml#6725)
  Add test illustrating ocaml#6575 (ocaml#6576)
  chore: add rule streaming proposal (ocaml#7195)
  test(stdlib): merge wrapped/unwrapped tests
  test: move all stdlib tests
  fix: allow unwrapped libraries with `(stdlib ..)`
  test: demonstrate crash in modules.ml when `(stdlib .. )` used with `(wrapped false)`
  fix(install): respect display options (ocaml#7116)
  ...
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.

2 participants