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

Run benchmarks with current-bench #500

Merged
merged 1 commit into from
May 31, 2023
Merged

Run benchmarks with current-bench #500

merged 1 commit into from
May 31, 2023

Conversation

Sudha247
Copy link
Contributor

@Sudha247 Sudha247 commented May 3, 2023

Moves the benchmarks to use continuous benchmarking with current-bench. This PR makes benchmark files emit ocaml-cb compliant json format results.

The code needs more clean-up. Opening this draft early to see how the results get displayed in the current-bench front-end.

I've added yojson as a test dependency for now. We can maybe move it to a separate bench opam package.

@talex5
Copy link
Collaborator

talex5 commented May 3, 2023

The link doesn't work (it goes to https://autumn.ocamllabs.io/ocaml-multicore/eio/pull/500; looks like it should be https://autumn.ocamllabs.io/ocaml-multicore/eio/pull/500/base/main). But then it just says "No data for selected interval".

@Sudha247
Copy link
Contributor Author

Sudha247 commented May 3, 2023

Looks like the benchmarks timed out on the default worker - this tends to happen for multicore benchmarks as the server is configured to run on a single core. @punchagan is configuring this to run on a different server that supports multicore benchmarks.

Before it timed out, the status was shown as Passed incorrectly. This may or may not be a bug. In any case Puneeth is aware of this and is trying to find the cause.

@punchagan
Copy link

punchagan commented May 3, 2023

The link doesn't work

Yes, that's a bug. We've a fix for this that we hope to merge soon.

@Sudha247
Copy link
Contributor Author

Making it ready for review as we see the graphs - https://autumn.ocamllabs.io/ocaml-multicore/eio/pull/500/base/main/benchmark/Eio?worker=fermat&image=ocaml%2Fopam%3Adebian-ocaml-5.0. Will look into cleaning them up.

@talex5
Copy link
Collaborator

talex5 commented May 11, 2023

The graphs are a bit hard to read. We should probably remove the "promotions" ones (they're not very useful).

@talex5
Copy link
Collaborator

talex5 commented May 13, 2023

Now it says "No data for selected interval" again.

@talex5
Copy link
Collaborator

talex5 commented May 25, 2023

Seems to be working now. Needs rebasing on main though.

eio.opam Outdated Show resolved Hide resolved
@Sudha247
Copy link
Contributor Author

Rebased on main. The CI failures on 32-bit look unrelated, IINM.

@talex5
Copy link
Collaborator

talex5 commented May 29, 2023

I don't think the rebase worked. Those 32-bit failures were fixed on main last week and main is passing CI. The current head commit is bf25e24.

@Sudha247
Copy link
Contributor Author

Oops, rebased on the right head now!

@talex5 talex5 force-pushed the current-bench branch 2 times, most recently from 8966a8c to 5bbe6da Compare May 30, 2023 10:33
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

I squashed your commits into one, and then added a few more:

  • It now uses Yojson to generate the JSON, since we're adding a dependency on it anyway.
  • I simplified the benchmarks code so the JSON generation is done from main.
  • You can now choose to run only one benchmark.
  • I tried to improve the labels and descriptions.
  • I changed the dune build so that the benchmarks are attached to eio_main.

If that looks OK to you, we can squash and merge. Thanks!

dune exec -- ./bench/bench_stream.exe
dune exec -- ./bench/bench_semaphore.exe
dune exec -- ./bench/bench_cancel.exe
if ocamlc -config | grep -q '^system: linux'; then dune exec -- ./lib_eio_linux/tests/bench_noop.exe; fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth noting that this benchmark got dropped.

@talex5
Copy link
Collaborator

talex5 commented May 30, 2023

Are / characters special in metric names? The format doesn't seem to be defined explicitly; there is an example at https://github.com/ocurrent/current-bench#json-format, but it doesn't use /. But I see your version had multiple lines on each graph, whereas my version has separate graphs (that isn't necessarily a problem, but it is strange).

The Cancel benchmark at https://autumn.ocamllabs.io/ocaml-multicore/eio/pull/500/base/main?worker=fermat&image=ocaml%2Fopam%3Adebian-ocaml-5.0 is a bit odd too. Both graphs are labelled "take-first". In reality, one is labelled "take-first/domain". Also, one is missing a description for some reason (it's in the JSON though).

@Sudha247
Copy link
Contributor Author

Are / characters special in metric names?

Yes, sorry I forgot to mention that. Paraphrasing what @ElectreAAS told me in a private thread - when we name benchmarks something like bench_1/sub_plot_1 and bench_1/sub_plot_2, they're different lines in the same graph bench_1. I'm okay with keeping them as separate graphs too.

Cancel bench is a bit weird indeed. Makes me wonder if - is a special character too?

@talex5 talex5 force-pushed the current-bench branch 3 times, most recently from bc512fb to a6fdecd Compare May 31, 2023 08:44
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

I got the CI passing on Windows. Ready to merge once CI has finished (and looks sensible).

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