Use `wait4` and measure memory usage of children in the runner

[filipeom]

Should Close #478

The PR is not ready yet, I still have to format the code, remove some comments, and I'm going to split this into two commits: one which focuses on introducing wait4 and replacing the usage of waitpid and another which counts and displays maxrss.

Also, I'm benchmarking this locally to see if the results are consistent with main.

But feel free to comment already :)

[filipeom]

I also made Marvin give us the memory statistics:

Btw, I know that you've told me you don't like that we're always iterating through the results list to accumulate the results we want to print/compare. Do you think we should make an issue to address it?

We could try to use owl to create a results data frame and then do some statistics/graphs?

[filipeom]

On a side note, do you think that adding wait4 to an existing Unix library might be more appropriate than explicitly including it here?

I was thinking of ocaml-rusage or, more appropriately, extunix, since they are already dependencies.

[redianthus]

Btw, I know that you've told me you don't like that we're always iterating through the results list to accumulate the results we want to print/compare. Do you think we should make an issue to address it?

I don't understand what you're refering to :sweat_smile:

[redianthus]

On a side note, do you think that adding wait4 to an existing Unix library might be more appropriate than explicitly including it here?

Yes, I'd be very happy not to have to maintain this piece of C code haha. IIRC @chambart initially wrote some C bindings for wait4 but they were much more lightweight, they should still be in the commit history if you want to have a look

[redianthus]

I also made Marvin give us the memory statistics:

This looks good but why is the min much bigger than max ? haha

[redianthus]

We could try to use owl to create a results data frame and then do some statistics/graphs?

I tried that at some point but I finally ended-up using the gnuplot ocaml bindings (see the code in bench/report/time_distribution.ml for instance)

[filipeom]

Btw, I know that you've told me you don't like that we're always iterating through the results list to accumulate the results we want to print/compare. Do you think we should make an issue to address it?

I don't understand what you're refering to 😅

I was talking the added functions in bench/report/runs.ml

On a side note, do you think that adding wait4 to an existing Unix library might be more appropriate than explicitly including it here?

Yes, I'd be very happy not to have to maintain this piece of C code haha. IIRC @chambart initially wrote some C bindings for wait4 but they were much more lightweight, they should still be in the commit history if you want to have a look

I will look into these and try to get a pr on extunix :crossed_fingers:

I also made Marvin give us the memory statistics:

This looks good but why is the min much bigger than max ? haha

It's because, in the min_max_maxrss function, the neutral element for the min operator is Int64.max_int, and since I ran the testcomp runner without files, it returned Int64.max_int. But it should be fine unless you want me to change it for this particular case.

We could try to use owl to create a results data frame and then do some statistics/graphs?

I tried that at some point but I finally ended-up using the gnuplot ocaml bindings (see the code in bench/report/time_distribution.ml for instance)

Ah yes, I forgot we already had graphs :sweat_smile: my bad

[redianthus]

I was talking the added functions in bench/report/runs.ml

It seems OK! :)

I will look into these and try to get a pr on extunix 🤞

Would be great, thanks for taking the time!

It's because, in the min_max_maxrss function, the neutral element for the min operator is Int64.max_int, and since I ran the testcomp runner without files, it returned Int64.max_int. But it should be fine unless you want me to change it for this particular case.

Haha OK, I was just wondering if you inverted two numbers or something. No I don't mind about it haha, it's just some benchmarking code, it does not matter.

Ah yes, I forgot we already had graphs 😅 my bad

Yes, I'm not sure how easily we can post them to Zulip. Maybe we can post them in a base64 encoding or something? To avoid having to upload them etc.

[filipeom]

Made it depend on https://github.com/ygrek/extunix/pull/58 directly. So we need to wait for a new release or pin it.

Yes, I'm not sure how easily we can post them to Zulip. Maybe we can post them in a base64 encoding or something? To avoid having to upload them etc

This is a nice feature! I might experiment with this in a separate PR :smiley:

[redianthus]

@filipeom, it seems this should be ready now that a new extunix version was published on opam, right? It may simply be missing a dev-setup dependency on extunix and this should be good to go?

[filipeom]

@filipeom, it seems this should be ready now that a new extunix version was published on opam, right? It may simply be missing a dev-setup dependency on extunix and this should be good to go?

I'm testing it now, but there is something weird going on. I think the latest version of extunix pulls in a ppxlib version which is conflicting with sedlex and since scfg depends on sedlex it is asking to remove smtml :sweat:

I'm investigating

[redianthus]

My guess is that a rebase is needed for the CI to pass (simply to get the new dependencies on main right)

[filipeom]

My guess is that a rebase is needed for the CI to pass (simply to get the new dependencies on main right)

I rebased it. The problem is still the same, extunix in 0.4.4 upgraded the ppxlib dep to ppxlib >= 0.36.0 while most of the packages we depend on have: ppxlib <= 0.35.0. And, it's a bit painful to go to all of these and make PRs to support the latest ppxlib. So maybe we'll just have to wait and see :sweat_smile:

[redianthus]

I see. I'm going to have a look at the packages with ppxlib <= 0.35.0 and open issues (I don't know ppxlib so I won't be able to do PR haha). Thanks for making the PR! Hopefully we'll be able to merge it at some point :)

[filipeom]

I see. I'm going to have a look at the packages with ppxlib <= 0.35.0 and open issues (I don't know ppxlib so I won't be able to do PR haha). Thanks for making the PR! Hopefully we'll be able to merge it at some point :)

I think it's just an API change: https://github.com/ygrek/extunix/commit/7c189801d2dbd4178d90ba127df35880b932a915. It's probably easy to do, but doing PR + waiting for a release of the library is pretty time consuming and draining

[redianthus]

I'm thinking more and more to move the benchmarking code to its own repository. I believe this would fix the issue we are having here, right?

[filipeom]

I'm thinking more and more to move the benchmarking code to its own repository. I believe this would fix the issue we are having here, right?

I think so, the runner is very self-contained in the dependencies that it includes so it might be ok as a stand-alone package. But that said, in your local switch you will still have the issue when installing both owi and the runner due to the ppxlib version incompatibility

[redianthus]

Moved to https://github.com/OCamlPro/symbocalypse ; do you mind pushing your changes there?

[filipeom]

Moved to https://github.com/OCamlPro/symbocalypse ; do you mind pushing your changes there?

Will do :)

[filipeom]

closing in favour of https://github.com/OCamlPro/symbocalypse/pull/2

[redianthus]

Thanks