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

child_process: refactor internal/child_process.js #11366

Closed

Conversation

notarseniy
Copy link
Contributor

@notarseniy notarseniy commented Feb 14, 2017

  • Prefer === to == where possible
  • Remove condition that will always be false
  • Prefer for-loop statements to forEach where possible for perfomance reasons
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Feb 14, 2017
stream.resume();
});
if (subprocess.stdio === null || subprocess.stdio === undefined) return;
for (let i = 0; i < subprocess.stdio.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use var instead of let here for better backportability (let used in this way is still slower at least in v6.x and v4.x).

Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion goes for all of the other occurrences below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! Is there a guide on code style?

Copy link
Member

Choose a reason for hiding this comment

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

@notarseniy make lint should have reported that issue. make lint is part of make test so if you ran tests, that should have come up as a lint error after the tests all passed. If the linter didn't report it, then there is probably a bug in our custom lint rule.

return;
stream.resume();
});
if (subprocess.stdio === null || subprocess.stdio === undefined) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this change is really worth it, the previous conditional is shorter and is equivalent in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Motivation of this change was this line with 'that-style' condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well IMHO that other line could/should be changed to a non-strict equality check. The performance difference probably isn't measurable anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok!

});
if (subprocess.stdio === null || subprocess.stdio === undefined) return;
for (let i = 0; i < subprocess.stdio.length; i++) {
if (!subprocess.stdio[i] || !subprocess.stdio[i].readable || subprocess.stdio[i]._readableState.readableListening)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of looking up the value each time, add a temporary variable before the conditional. For example:

const stdio = subprocess.stdio;
for (var i = 0; i < stdio.length; ++i) {
  const stream = stdio[i];
  // .... keep rest of the code the same
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion goes for all of the other occurrences below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sounds good. Will fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good comment. Will fix.

@@ -800,7 +800,7 @@ function _validateStdio(stdio, sync) {
stdio = i < 3 ? 'pipe' : 'ignore';
}

if (stdio === null || stdio === 'ignore') {
if (stdio === 'ignore') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? This would change behavior...

Copy link
Contributor Author

@notarseniy notarseniy Feb 14, 2017

Choose a reason for hiding this comment

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

In previous condition we're handling stdio === null, so this condition never will be executed with stdio === null

@notarseniy
Copy link
Contributor Author

Commited with fixes of comments.

@Trott
Copy link
Member

Trott commented Feb 14, 2017

@mscdex
Copy link
Contributor

mscdex commented Feb 14, 2017

More linting issues it seems. Run make jslint locally to find the specifics.

I'm guessing it's due to the duplicate var i; declarations in the two new for-loops.

return;
if (subprocess.stdio === null || subprocess.stdio === undefined) return;

const stdio = subprocess.stdio;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be moved to the top of the function, where the first conditional can take advantage of it.

* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons
@notarseniy
Copy link
Contributor Author

Recommited with fixes. /cc @Trott @mscdex

@mscdex
Copy link
Contributor

mscdex commented Feb 14, 2017

@mscdex
Copy link
Contributor

mscdex commented Feb 14, 2017

CI is green. LGTM.

@notarseniy
Copy link
Contributor Author

Bump! 😅 What the status of PR? Any other comments or additions?

targos pushed a commit that referenced this pull request Feb 18, 2017
* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons

PR-URL: #11366
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Brian White <[email protected]>
@targos
Copy link
Member

targos commented Feb 18, 2017

@notarseniy I think it has just been forgotten. I went ahead and landed it in 8fc362e. Thanks for the contribution!

@targos targos closed this Feb 18, 2017
@notarseniy
Copy link
Contributor Author

Thats okay :) Thank you too!

@notarseniy notarseniy deleted the internal-child-process-fix branch February 18, 2017 19:04
addaleax pushed a commit that referenced this pull request Feb 22, 2017
* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons

PR-URL: #11366
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Brian White <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons

PR-URL: #11366
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons

PR-URL: #11366
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons

PR-URL: #11366
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* Prefer === to == where possible
* Remove condition that will always be false
* Prefer for-loop statements to forEach where possible for perfomance reasons

PR-URL: #11366
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants