echo: Reduce memory allocation - #10090
Conversation
|
This is great work, I'd love to see if we had a way to track these changes over time. What was the main method you used for determining the memory usage, did you set up a benchmark? |
|
GNU testsuite comparison: |
2 similar comments
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
|
Hey @ChrisDryden , for benchmarking I used I believe the |
| @@ -523,12 +528,30 @@ fn full_version_argument() { | |||
| .stdout_matches(&Regex::new(r"^echo \(uutils coreutils\) (\d+\.\d+\.\d+)\n$").unwrap()); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
seems unrelated to this pr ?!
There was a problem hiding this comment.
This PR removes the length check from args in case of --help and --version and instead uses other logic to infer the length
These test cases serve as additional confirmation that the changes did not regress any behaviour.
There was a problem hiding this comment.
Improved the patchset. Please let me know if any further improvements are required.
| #[test] | ||
| fn full_help_argument() { | ||
| assert_ne!(new_ucmd!().arg("--help").succeeds().stdout(), b"--help\n"); | ||
| assert_ne!(new_ucmd!().arg("--help").succeeds().stdout(), b"--help"); // This one is just in case. | ||
| } | ||
|
|
||
| #[test] | ||
| fn multiple_help_argument() { |
|
GNU testsuite comparison: |
|
The CI failure logs seem to indicate its unrelated to the PR changes? It was working with the same code earlier... |

Replaced the vector allocation in
filter_flagswith direct iterator usage for reduced memory overhead.Added new tests to cover edge cases encountered during previous implementation attempts.
Memory Usage Analysis:
Using /usr/bin/time (Linux)
printf "00 %.0s" {1..100000} > args.txtResults:
main: Maximum resident set size (kbytes):19364This PR: Maximum resident set size (kbytes):
12280System
echo: Maximum resident set size (kbytes):2544Using dhat-rs:
main:This PR: