Add Maximum Resident Set tracking #321

lu-zero · 2020-09-28T13:56:21Z

Fixes #86

lu-zero · 2020-09-28T13:57:04Z

It isn't complete but should work well as proof of concept

sharkdp · 2020-10-13T12:06:06Z

Thank you very much for your contribution! I am planning to take a closer look soon.

sharkdp

Thank you for your contribution. I added a few comments.

I am inclined to accept such a change, but only if

it is thoroughly tested (not necessarily a unit test, but at least some manual tests on Linux and Windows)
all questions here and in the ticket can be answered. in particular, the question about the intermediate shell and the precise working mechanism of getrusage.
it nicely integrates within the code base (the namings of some structs/types/variables/functions are currently focused on times, not memory consumption)

sharkdp · 2020-10-16T17:10:30Z

src/hyperfine/benchmark.rs

@@ -74,13 +78,15 @@ pub fn time_shell_command(
        time_real = subtract_shell_spawning_time(time_real, spawning_time.time_real);
        time_user = subtract_shell_spawning_time(time_user, spawning_time.time_user);
        time_system = subtract_shell_spawning_time(time_system, spawning_time.time_system);
+        max_rss = subtract_shell_spawning_time(max_rss, spawning_time.max_rss);


Okay, so this subtracts the memory required by the shell? In this case, we should probably give the function a new name?

Indeed, I did the least amount of changes to get something going, if you have idea on function names please tell me :)

sharkdp · 2020-10-16T17:24:33Z

src/hyperfine/timer/unix_timer.rs

@@ -55,6 +55,7 @@ fn get_cpu_times() -> CPUTimes {
            + i64::from(result.ru_utime.tv_usec),
        system_usec: i64::from(result.ru_stime.tv_sec) * MICROSEC_PER_SEC
            + i64::from(result.ru_stime.tv_usec),
+        max_rss_byte: i64::from(result.ru_maxrss),


From man 2 getrusage: "in kilo-bytes"

ru_maxrss (since Linux 2.6.32) This is the maximum resident set size used (in kilo‐ bytes). For RUSAGE_CHILDREN, this is the resident set size of the largest child, not the maximum resident set size of the process tree.

Also, I'm not sure what to make of the "For RUSAGE_CHILDREN, this is the resident set size of the largest child, not the maximum resident set size of the process tree" comment. Does this mean that we would not get the total amount of memory used by the tree

+ /bin/sh +--+ command-to-be-benchmarked

but rather the memory usage of EITHER the shell or the command-to-be-benchmarked?!

I wonder now if libc::wait4 wouldn't give us better information.

sharkdp · 2020-10-16T17:27:03Z

src/hyperfine/timer/unix_timer.rs

@@ -63,6 +64,7 @@ fn cpu_time_interval(start: &CPUTimes, end: &CPUTimes) -> CPUInterval {
    CPUInterval {
        user: ((end.user_usec - start.user_usec) as f64) * 1e-6,
        system: ((end.system_usec - start.system_usec) as f64) * 1e-6,
+        max_rss: ((end.max_rss_byte - start.max_rss_byte) as f64) * 1e-6,


I would prefer if we used kibibytes and mebibytes here, i.e. divide by 1024 or 1024². All references of MegaBytes should then be changed to KibiBytes or MebiBytes. In the output we would ideally use "KiB" or "MiB".

This is to avoid the confusion with "KB" and "MB" where it's unclear if they refer to the SI standard (as they should, in my opinion), or if it's an old Unix program which uses "KB" and "MB" when they actually refer to "1024 byte" or "1024² byte".

Sounds correct.

sharkdp · 2020-10-16T17:32:16Z

src/hyperfine/benchmark.rs


    if options.output_style != OutputStyleOption::Disabled {
        println!(
-            "  Time ({} ± {}):     {:>8} ± {:>8}    [User: {}, System: {}]",
+            "  Time ({} ± {}):     {:>8} ± {:>8}    [User: {}, System: {}, MaxRSS: {}]",


I'm pretty sure that most users would not know what "MaxRSS" refers to. Maybe just "Mem:" and then explain the precise meaning in the --help text and man page?

That's a good idea.

sharkdp · 2020-10-16T17:34:28Z

src/hyperfine/benchmark.rs

@@ -358,16 +371,18 @@ pub fn run_benchmark(

    let (user_str, user_unit) = format_duration_unit(user_mean, None);
    let system_str = format_duration(system_mean, Some(user_unit));
+    let max_rss_str = format!("{:.3} MB", max_rss_mean);


I would prefer if we would show either KiB or MiB (depending on the actual usage), with fewer digits in the fractional part.

Yes, probably a format_memory function would make that better :)

sharkdp · 2021-08-22T14:35:02Z

Closing this for now. I plan to restructure the hyperfine code a little bit and will keep this use-case in mind.

lu-zero · 2021-08-22T14:57:19Z

Great, let me know if I can help you :)

I prepared https://crates.io/crates/wait4 a while ago to improve a bit the backend, but then I had to focus on other tasks.

sharkdp · 2021-08-22T15:05:33Z

Thank you. This PR is not forgotten. But probably won't be merged in this form.

I prepared https://crates.io/crates/wait4 a while ago to improve a bit the backend, but then I had to focus on other tasks.

Nice! I still don't fully understand whether it would be possible to get the memory usage of the process in question, given that we launch it through an intermediate shell (related: #336).

lu-zero · 2021-08-22T15:28:53Z

IF we have the intermediate shell we'd have to use getrusage(RUSAGE_CHILDREN) and then subtract what wait4() is telling for the shell PID, I guess.

Worst case we'd have to know by some mean the PID of the program we care about and pass it to the wait4 or equivalent.

lu-zero · 2021-08-22T17:33:15Z

I played a bit and I discovered that wait4(child_pid) is behaving exactly like getrusage(RUSAGE_CHILDREN), that means that the shell resources would be measured and subtracted from an empty shell spawn as well.

Alex-PLACET · 2021-10-08T12:55:47Z

@lu-zero Have you tested that on Windows ? I'm testing your branch and the result is wrong

Memory (mean ± σ): 0.260 KiB ± 0.000 KiB
Range (min … max): 0.260 KiB … 0.260 KiB

For a process which takes 2 Gb

lu-zero force-pushed the max_rss branch from d579707 to f3e623f Compare September 28, 2020 16:31

sharkdp requested changes Oct 16, 2020

View reviewed changes

wip: Add Maximum Resident Set

9082aee

lu-zero force-pushed the max_rss branch from f3e623f to 9082aee Compare October 20, 2020 13:10

sharkdp added the needs-work label Feb 15, 2021

sharkdp closed this Aug 22, 2021

sharkdp mentioned this pull request Nov 20, 2022

Support for benchmarking memory usage, etc. #86

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Maximum Resident Set tracking #321

Add Maximum Resident Set tracking #321

lu-zero commented Sep 28, 2020

lu-zero commented Sep 28, 2020

sharkdp commented Oct 13, 2020

sharkdp left a comment •

edited

sharkdp Oct 16, 2020

lu-zero Oct 16, 2020

sharkdp Oct 16, 2020

sharkdp Oct 16, 2020

lu-zero Oct 20, 2020

sharkdp Oct 16, 2020

lu-zero Oct 16, 2020

sharkdp Oct 16, 2020

lu-zero Oct 16, 2020

sharkdp Oct 16, 2020

lu-zero Oct 16, 2020

sharkdp commented Aug 22, 2021

lu-zero commented Aug 22, 2021

sharkdp commented Aug 22, 2021

lu-zero commented Aug 22, 2021

lu-zero commented Aug 22, 2021

Alex-PLACET commented Oct 8, 2021 •

edited

Add Maximum Resident Set tracking #321

Add Maximum Resident Set tracking #321

Conversation

lu-zero commented Sep 28, 2020

lu-zero commented Sep 28, 2020

sharkdp commented Oct 13, 2020

sharkdp left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp commented Aug 22, 2021

lu-zero commented Aug 22, 2021

sharkdp commented Aug 22, 2021

lu-zero commented Aug 22, 2021

lu-zero commented Aug 22, 2021

Alex-PLACET commented Oct 8, 2021 • edited

sharkdp left a comment •

edited

Alex-PLACET commented Oct 8, 2021 •

edited