Add json output and make target for benchmarking layered store. #1146
Conversation
@icristescu suggested 2005 as the number of commits since every 1000 commits, a tree is committed and there's 2 freeze cycles so that time can also be taken into account.
bench/irmin-pack/layers.ml
Outdated
| @@ -267,7 +267,31 @@ let run config = | |||
| Fmt.epr "After freeze thread finished : "; | |||
| FSHelper.print_size config.root) | |||
|
|
|||
| let main () ncommits ncycles depth clear no_freeze show_stats = | |||
| type result = { | |||
| number_of_commits : int; | |||
There was a problem hiding this comment.
I guess this is not very important to report on the benchmark outputs (as hopefully they should always stay the same? :-))
There was a problem hiding this comment.
That's a good point!
|
Nice :-) Looking forward to see the benchmark results in action ;-) |
CHANGES.md
Outdated
| @@ -21,6 +21,8 @@ | |||
| - **irmin-bench** (_new_): | |||
| - Created a new package to contain benchmarks for Irmin and its various | |||
| backends. (#1142, @CraigFe) | |||
| - Added ability to get json output and a make target to tun layers benchmark. | |||
There was a problem hiding this comment.
| - Added ability to get json output and a make target to tun layers benchmark. | |
| - Added ability to get json output and a make target to run layers benchmark. |
CHANGES.md
Outdated
| @@ -189,7 +191,7 @@ | |||
| for generating Irmin generics. | |||
|
|
|||
| - **irmin-unix**: | |||
| - Added a `--hash` parameter to the command-line interface, allowing the hash | |||
| - Added a `--hash` parameter to the command-line interface, the hash | |||
There was a problem hiding this comment.
unintended change, I think
gs0510
force-pushed
the
make_bench_layers
branch
from
4a4a063 to
9c27b7b
Compare
| @@ -229,7 +229,7 @@ let first_5_cycles config repo = | |||
| print_commit_stats config c 0 0.0; | |||
| let rec aux i c = | |||
| add_min c; | |||
| if i >= 4 then Lwt.return c | |||
| if i > 4 then Lwt.return c | |||
There was a problem hiding this comment.
Only 4 cycles were running before, fixed to run 5 cycles as the function name suggests 😸
There was a problem hiding this comment.
One day, we'll live in an Lwt utopia with helper functions for these things...
| @@ -281,13 +302,15 @@ let main () ncommits ncycles depth clear no_freeze show_stats = | |||
| in | |||
| init config; | |||
| let d, _ = Lwt_main.run (with_timer (fun () -> run config)) in | |||
| let all_commits = ncommits * ncycles in | |||
| let all_commits = ncommits * (ncycles + 5) in | |||
There was a problem hiding this comment.
Seems odd to always run 5 more cycles than requested. Perhaps we could run max(5, ncycles) and update the CLI doc to say that at least 5 runs are necessary?
There was a problem hiding this comment.
The first 5 cycles are run in addition to the number of cycles passed as arg.
https://github.com/mirage/irmin/blob/master/bench/irmin-pack/layers.ml#L263 runs the first 5 cycles followed by run_cycles. Maybe we should remove the first_5_cycles and change the doc to say running at least 5 is necessary?
There was a problem hiding this comment.
Indeed, I think it's better to have a more explicit behaviour.
Out of interest, why is it necessary to have at least 5 cycles? It seems that I might want to just run ~1 cycle when working on the benchmarks, for instance.
There was a problem hiding this comment.
I don't know! Paging @icristescu 😁
There was a problem hiding this comment.
The benchmarks are meant to be somewhat similar to the tezos usecase where every freeze copies the last 5 cycles. So we need to have the first 5 cycles before calling the first freeze. Then each cycle we call freeze again (done by run_cycles). You are free to change the benchmarks, just I think the default behaviour should be this one (and more documentation is always welcomed:) )
There was a problem hiding this comment.
Merge when ready then, @gs0510 🙂
| let doc = | ||
| Arg.info | ||
| ~doc: | ||
| "Number of cycles. This will be in addition to the 5 cycles that run \ |
There was a problem hiding this comment.
Added this documentation here, hopefully it makes it clearer that it will be 5 + ncycles running. cc @craigfe @icristescu
|
Thanks! |
|
Does it mean that I can rebase my other PRs to see the benchmark results now? |
|
@samoht Sorry this will take one more day because I have to add a config to run |
@icristescu suggested 2005 as the number of commits since every 1000 commits, a tree is committed and there's 2 freeze cycles so that time can also be taken into account, I'd be open to just using the default value as well.