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

benchmark: use 'yes' instead of echo in a loop #8721

Closed

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Sep 22, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

Description of change

This changes child-process-exec-stdout benchmark to use yes instead of echo in a while loop. This makes this benchmark consistent withchild-process-read which already uses yes and allows this benchmark to be executed on Windows.

/cc @nodejs/benchmarking

This changes child-process-exec-stdout benchmark to use 'yes' instead
of echo in a while loop. This makes this benchmark consistent with
child-process-read which already uses `yes` and allows this benchmark
to be executed on Windows.
@bzoz bzoz added the benchmark Issues and PRs related to the benchmark subsystem. label Sep 22, 2016
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Sep 22, 2016
@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 22, 2016
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know Windows had a yes command. LGTM.

@yosuke-furukawa
Copy link
Member

Windows does not have yes command.

@gibfahn
Copy link
Member

gibfahn commented Sep 22, 2016

Yep, I don't have a yes command in cmd. However there is one in Git bash, (default location C:\Program Files\Git\usr\bin\yes.exe), which may be in your CMD path by default, causing one to think there's one built in. Obviously we can't depend on it being there though.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 22, 2016

Note: while … echo itself pipes at 1 MiB/s for a 5-char msg and 70 MiB/s for a 4096-char msg, while yes pipes at 8 GiB/s for the same 5-char msg and 5 GiB/s for a 4096-char msg.

So this could reduce the slowdown (or the cpu usage) introduced by the test itself here. Not sure if that effect is measurable, though.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 22, 2016

This significantly affects the benchmark (in a positive way).

Before:

$ node benchmark/child_process/child-process-exec-stdout.js
child_process/child-process-exec-stdout.js dur=5 len=64: 40872.31295343989
child_process/child-process-exec-stdout.js dur=5 len=256: 42284.49284876121
child_process/child-process-exec-stdout.js dur=5 len=1024: 42556.261440065464
child_process/child-process-exec-stdout.js dur=5 len=4096: 40866.002162569785
child_process/child-process-exec-stdout.js dur=5 len=32768: 45769.970852308645

After:

$ node benchmark/child_process/child-process-exec-stdout.js
child_process/child-process-exec-stdout.js dur=5 len=64: 52294.7307645832
child_process/child-process-exec-stdout.js dur=5 len=256: 51257.097720466736
child_process/child-process-exec-stdout.js dur=5 len=1024: 50287.9926165738
child_process/child-process-exec-stdout.js dur=5 len=4096: 52306.85402878927
child_process/child-process-exec-stdout.js dur=5 len=32768: 52307.0022664649

@imyller
Copy link
Member

imyller commented Sep 22, 2016

@JacksonTian
Copy link
Contributor

the CI not happy.

@bzoz
Copy link
Contributor Author

bzoz commented Sep 23, 2016

I've run the CI again: https://ci.nodejs.org/job/node-test-commit/5258/. It is still not happy. Anyhow, none of those failures are related.

Regarding yes - yes, it is not available by default under Windows, and needs to be installed by the user. I've experimented with shell and python scripts but they were much slower than yes.

@gibfahn
Copy link
Member

gibfahn commented Sep 23, 2016

@bzoz Is there any documentation for the requirements for running benchmarks on Windows? It would be helpful to document the need for a non-standard exe on the system (maybe in https://github.com/nodejs/node/tree/master/benchmark#prerequisites ?).

It would also make sense to have a comment above this line saying that this requires a yes.exe binary, e.g. the one in Git for Windows.

@bzoz
Copy link
Contributor Author

bzoz commented Sep 23, 2016

@gibfahn This is already mentioned in https://github.com/nodejs/node/blob/master/BUILDING.md#windows, but yes, we should also add this to https://github.com/nodejs/node/tree/master/benchmark#prerequisites.

bzoz added a commit that referenced this pull request Sep 26, 2016
This changes child-process-exec-stdout benchmark to use 'yes' instead
of echo in a while loop. This makes this benchmark consistent with
child-process-read which already uses `yes` and allows this benchmark
to be executed on Windows.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #8721
@bzoz
Copy link
Contributor Author

bzoz commented Sep 26, 2016

Landed in d5bc52a

@bzoz bzoz closed this Sep 26, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
This changes child-process-exec-stdout benchmark to use 'yes' instead
of echo in a while loop. This makes this benchmark consistent with
child-process-read which already uses `yes` and allows this benchmark
to be executed on Windows.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #8721
geek pushed a commit to geek/node that referenced this pull request Sep 30, 2016
This changes child-process-exec-stdout benchmark to use 'yes' instead
of echo in a while loop. This makes this benchmark consistent with
child-process-read which already uses `yes` and allows this benchmark
to be executed on Windows.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#8721
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
This changes child-process-exec-stdout benchmark to use 'yes' instead
of echo in a while loop. This makes this benchmark consistent with
child-process-read which already uses `yes` and allows this benchmark
to be executed on Windows.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #8721
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants