Add benchmark for ppxlib driver #376
Conversation
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
deec90d to
66f3761
Compare
|
This is awesome, thanks @gridbugs!! 🎉 I'm curious to know if we could integrate it with current-bench? (that would allow us to have benchmark reports in GitHub PRs automatically IIUC?) Do you know how easy that would be @shakthimaan? |
|
Thanks @tmattio! I forgot to mention in the description, but integrating with current-bench is the goal for this. |
db00a43 to
d1f15be
Compare
|
This fails to build on 4.04 and 4.05 because it uses standard library functions that didn't exist in those versions of the language. Also, the pinned version of ppx_sexp_conv is too new to be built by those versions of the compiler. Is it making sure that the benchmarks build on old versions of the compiler? |
|
Thanks a lot @gridbugs for the great contribution! It is awesome that we will able to have benchmarks integrated with I'll review the code next week.
I think the last solution is the best one, I hope it works! |
|
Hey @gridbugs. Also from my side, thanks a lot! I'm really looking forward to enrolling the To add a fourth possible solution for the CI failure: what we usually do to locally turn off CI builds on old compilers is to add a field to the |
|
There is actually another problem with not separating the benchmark in another package: the dependencies of the benchmark and the core library are mixed. This means that:
Since If you do not have that much time to give to ppxlib, I could try to implement this solution myself. In any case, thanks a lot for the work! As @pitag-ha said, it is really great result! |
To avoid that, @gridbugs has added it as a |
Yes, that's right, sorry I forgot about that! So in fact, the reasoning for separating benchmark from the tests in yojson was that it can be time costly (and also, depending on your definition of test, it is not exactly a test as there are no exact failing/succeeding conclusion). In this case, if the tests run sufficiently fast and are not computation-intensive, I'm fine with any solution! |
Yes. Please follow the steps in |
Yes, I'll do that as soon as this PR is merged. Btw, im currently at the MirageOs retreat and won't have much time to review this and next week. If you have time, @panglesd , that'd be very cool. If not, I'll review as soon as I'm back. |
38e6bf8 to
939762d
Compare
|
I added a An alternative would be to move the benchmark to a separate repo. How important is it that ppxlib continue to support versions of ocaml older than 4.10? |
Short and concrete answer to this: I'd prefer to avoid dropping compiler support up to 4.10 to add benchmarks. Longer and more general answer to this: For the same reason @panglesd mentioned above why we never add new hard dependencies to
As mentioned above, to fix this I think you could have a compiler >= 4.04.1 bound in the to their dune files. |
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
This prevents ppx_sexp_conv from being compiled in CI on versions of the ocaml compiler which it does not support. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
15547d2 to
f87e922
Compare
|
I've updated all the benchmarking dune files so that they only build on ocaml >= 4.10, and now the tests for older version of the compiler are running again. |
|
Thanks! Sorry for leading you into the wrong direction with the package thing. I'll review the code soon! |
bench/bench.ml
Outdated
| let n_warmup = 10 in | ||
| let n = 100 in | ||
| Benchmark_suite.run_benchmarks ~n_warmup ~n | ||
| |> Yojson.to_string |> print_endline |
There was a problem hiding this comment.
If that's not impacting current-bench (I guess no!) it could be nice to print the results in a more user-friendly way, to be able to read the results locally as well.
| |> Yojson.to_string |> print_endline | |
| |> Yojson.pretty_to_string |> print_endline |
bench/bench.ml
Outdated
| module Path_to_current_exe = struct | ||
| let via_procfs () = | ||
| (* Look up the current executable's path in the proc filesystem. *) | ||
| let pid = Unix.getpid () in | ||
| let proc_exe_path = Printf.sprintf "/proc/%d/exe" pid in | ||
| if Sys.file_exists proc_exe_path then Some (Unix.readlink proc_exe_path) | ||
| else None | ||
|
|
||
| let via_cwd () = | ||
| (* Assume the current working directory is the root of the project (as it | ||
| would be if this was run via `make bench`) and find the path to the | ||
| current exe relative to the project root *) | ||
| let cwd = Unix.getcwd () in | ||
| let relative_path_parts = "_build/default/bench/bench.exe" in | ||
| let absolute_path = Filename.concat cwd relative_path_parts in | ||
| if Sys.file_exists absolute_path then Some absolute_path else None | ||
|
|
||
| let methods = [ via_procfs; via_cwd ] | ||
|
|
||
| let get () = | ||
| let maybe_path = List.find_map (fun m -> m ()) methods in | ||
| match maybe_path with | ||
| | Some path -> path | ||
| | None -> failwith "couldn't determine the path to the current exe" | ||
| end |
There was a problem hiding this comment.
This code is just to deal with the case where bench is called somewhere else than the root of the project. That's a nice attention to details 👍
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
All good! I still ended up making a ppx-bench package to handle extra dependencies required only for benchmarking. |
f87e922 to
61b8a85
Compare
| "dune" {>= "2.7"} | ||
| "ocaml" {>= "4.04.1"} | ||
| "ppxlib" {= version} | ||
| "base" |
There was a problem hiding this comment.
Could you add this for consistency with the dune-project file?
| "base" | |
| "base" {>= "v0.15" & < "v0.16"} |
There was a problem hiding this comment.
I ended up removing the versions from dune-project instead because they were causing CI to limit the versions of ocaml used for testing to just those compatible with that range of versions.
9d1293d to
ce4b411
Compare
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
ce4b411 to
a899d17
Compare
|
Thanks, @gridbugs! Looking forward to integrating this with |
|
I tried to enrol this repository in ocaml-benchmark, but "This action must be performed by an organization owner". Also, there are no public people in the |
|
I already enrolled it on Monday: https://autumn.ocamllabs.io/ocaml-ppx/ppxlib . I think anyone of us could have done it (we should have the same permissions). There seems to be a bug, though: notice the "No data for selected interval.". We think it's on the |
|
Hi, this change broke all most of the packages relying on mirage. Is there a particular reason why ppx_sexp_conv is vendored? Why not pulling it up as a normal opam dependency of the ppxlib-bench package? |
|
Ouch, sorry for the mess! I can't remember or find any reason to vendor instead of pulling it as normal dependency. Maybe it was needed before the Without reason to do otherwise, I'll open a PR removing the vendored bench dependency, and make a |
|
Last I spoke to @pitag-ha about it, it was vendored in order to have more control over its versioning. I don't know any deeper than that what the reasoning was, perhaps Sonja remembers. |
|
I may understand why it was done this way, after trying to patch this. I'm not sure what's the best way to fix that. So far I'm leaning into reduce the vendor by removing everything that's not used and removing the public libraries, which should fix the issues when vendoring ppxlib itself. I'm having an issue with the extender which i can't get to compile for some reason tho at the moment so i haven't opened a PR yet. |
|
Arrg. Thanks for pointing this out, @kit-ty-kate! We can choose a different PPX for benchmarking and vendor that one (and get rid of |
|
If I remember correctly I chose to vendor @kit-ty-kate mind sharing why vendoring |
|
Excellent, thanks @kit-ty-kate! |
This adds a target
make benchwhich measures the time taken to run the ppxlib driver on some input files. I've added two drivers to be benchmarked:Each driver specifies some input files copied from MIT licensed projects. The benchmark runner invokes each driver on each of its specified inputs and measures the mean and stddev of the time it took to run each driver on each of its inputs.