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: fix child-process-read on Windows #6971

Closed

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented May 25, 2016

Checklist
  • the commit message follows commit guidelines
Affected core subsystem(s)

benchmark

Description of change

Under Windows ipc communication requires the other process to format its messages with 'IPC framing protocol'. Otherwise, an assert is triggered in libuv. This commit changes child-process-read.js benchmark to use stdout to communicate with parent process. It also adds child-process-read-ipc.js to benchmark IPC communication using child node process.

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label May 25, 2016
@mscdex mscdex added windows Issues and PRs related to the Windows platform. child_process Issues and PRs related to the child_process subsystem. labels May 25, 2016
@bzoz bzoz force-pushed the Bartosz-ChildProcessBenchmarkFix branch from ec91385 to 7402cc2 Compare May 25, 2016 14:37
@@ -1,7 +1,14 @@
'use strict';
var common = require('../common.js');
var os = require('os');

var messagesLenght = [64, 256, 1024, 4096];
Copy link
Contributor

Choose a reason for hiding this comment

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

Length is spelled wrong throughout this file.

bzoz added 2 commits May 31, 2016 11:43
Under Windows 'ipc' communication requires the other process to format
its messages with 'IPC framing protocol'. Otherwise, an assert is
triggered in libuv. This commit changes child-process-read benchmark
to use stdout to communicate with parent process. It also adds
child-process-read-ipc.js to benchmark IPC communication using
child node process.
@bzoz bzoz force-pushed the Bartosz-ChildProcessBenchmarkFix branch from a96c1c6 to 4961a5d Compare May 31, 2016 09:46
@bzoz
Copy link
Contributor Author

bzoz commented May 31, 2016

I've corrected the spelling, PTAL

@joaocgreis
Copy link
Member

LGTM

cc @nodejs/benchmarking

if (process.argv[2] === 'child')
{
const len = +process.argv[3];
const msg = '"' + Array(len).join('.') + '"';
Copy link
Member

Choose a reason for hiding this comment

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

const msg = `"${'.'.repeat(len)}"`;

@jasnell
Copy link
Member

jasnell commented Jun 3, 2016

LGTM with a nit

@bzoz
Copy link
Contributor Author

bzoz commented Jun 6, 2016

Updated, PTAL

@joaocgreis
Copy link
Member

Thanks @bzoz . Will land tomorrow if there are no further issues.

joaocgreis pushed a commit that referenced this pull request Jun 9, 2016
Under Windows 'ipc' communication requires the other process to format
its messages with 'IPC framing protocol'. Otherwise, an assert is
triggered in libuv. This commit changes child-process-read benchmark
to use stdout to communicate with parent process. It also adds
child-process-read-ipc.js to benchmark IPC communication using
child node process.

PR-URL: #6971
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joaocgreis
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/2964/ (failures unrelated)

Landed in cbbdc29

Thanks!

@joaocgreis joaocgreis closed this Jun 9, 2016
evanlucas pushed a commit that referenced this pull request Jun 16, 2016
Under Windows 'ipc' communication requires the other process to format
its messages with 'IPC framing protocol'. Otherwise, an assert is
triggered in libuv. This commit changes child-process-read benchmark
to use stdout to communicate with parent process. It also adds
child-process-read-ipc.js to benchmark IPC communication using
child node process.

PR-URL: #6971
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request Jun 16, 2016
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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants