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

Use wait4 and measure memory usage of children in the runner #510

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

filipeom
Copy link
Collaborator

@filipeom filipeom commented Feb 23, 2025

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 filipeom changed the title Use wait4 and measure memory usage of child in runner Use wait4 and measure memory usage of children in the runner Feb 23, 2025
@filipeom filipeom marked this pull request as draft February 23, 2025 13:17
@filipeom filipeom force-pushed the bench-measure-maxrss branch from dabfb50 to 461b929 Compare February 23, 2025 22:33
@filipeom
Copy link
Collaborator Author

filipeom commented Feb 23, 2025

I also made Marvin give us the memory statistics:

image

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 filipeom marked this pull request as ready for review February 23, 2025 22:40
@filipeom
Copy link
Collaborator Author

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.

@filipeom filipeom removed the pending label Feb 23, 2025
@zapashcanon
Copy link
Member

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 😅

@zapashcanon
Copy link
Member

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

@zapashcanon
Copy link
Member

zapashcanon commented Feb 24, 2025

I also made Marvin give us the memory statistics:

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

@zapashcanon
Copy link
Member

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
Copy link
Collaborator Author

filipeom commented Feb 24, 2025

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 🤞

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 😅 my bad

@filipeom filipeom force-pushed the bench-measure-maxrss branch from 461b929 to c0d56bc Compare February 24, 2025 19:28
@zapashcanon
Copy link
Member

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 filipeom force-pushed the bench-measure-maxrss branch 2 times, most recently from 70aca93 to 567473e Compare February 27, 2025 09:48
@filipeom filipeom force-pushed the bench-measure-maxrss branch from 567473e to 20fd63e Compare February 27, 2025 12:21
@filipeom
Copy link
Collaborator Author

filipeom commented Feb 27, 2025

Made it depend on ygrek/extunix#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 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mesure memory usage in benchmarks
2 participants