Skip to content

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