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.

Sign up for GitHub

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

Jump to bottom

Minor improvements #8906

Closed
wants to merge 2 commits into from
Closed

Minor improvements #8906

wants to merge 2 commits into from

Conversation

rmeja
Copy link
Contributor

@rmeja rmeja commented Oct 3, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Change '==' to '==='
Change 'var' to 'let'
Delete unused argument in function

@rmeja
Minors improvements
2d06f56 
Change '==' to '==='
Change 'var' to 'let'
Delete unused argument in function
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 3, 2016
evanlucas
evanlucas reviewed Oct 3, 2016
View reviewed changes
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

I would be curious if any of the other changes have performance effects.

lib/internal/bootstrap_node.js
@@ -182,7 +182,7 @@
process._setupProcessObject(pushValueToArray);

function pushValueToArray() {
for (var i = 0; i < arguments.length; i++)
for (let i = 0; i < arguments.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.

This is used via a few c++ functions to improve the performance of pushing onto an array. I'm pretty sure let is still a bit slower than var, so I would probably leave this one alone

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

mscdex
mscdex reviewed Oct 3, 2016
View reviewed changes
lib/internal/bootstrap_node.js
@@ -443,7 +443,7 @@
if (EXPOSE_INTERNALS) {
NativeModule.nonInternalExists = NativeModule.exists;

NativeModule.isInternal = function(id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is best left unchanged since the other isInternal() implementation does use id and we won't know which is being used at runtime. IIRC V8 still prefers it (performance-wise) when the argument/parameter counts match.

@mscdex
Copy link
Contributor

mscdex commented Oct 3, 2016 •
edited
Loading

I know others have said otherwise about being able to use let "safely" (without a performance hit), but I'm still inclined to disagree with the let changes. I just recall seeing even var=>const changes affect performance in fairly hot paths not long ago (in a separate project) and also since let still has performance issues when used with a loop, I am still very hesitant to start using it in core yet.

Trott
Trott requested changes Oct 3, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

  • The == -> === changes seem good to me.
  • The var -> let changes are problematic, especially the ones involving for loops. Currently, V8 performance of let in loops is often not great. Unless you are prepared to benchmark the changes somehow, I'd stick with var instead.
  • I'd also leave the id argument in the isInternal() function for the performance/optimization reasons described by @mscdex.

If the let and function argument changes were happening in code in the test directory rather than the lib/internal directory, I'd be inclined to give them a thumbs up. But here, we have to be a bit more wary of things that might negatively impact performance.

@rmeja
Copy link
Contributor Author

rmeja commented Oct 4, 2016

I humbly incline and apply the changes requested, gentlemen. 😉

@mscdex
Copy link
Contributor

mscdex commented Oct 4, 2016 •
edited
Loading

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/4374/

lpinca
lpinca approved these changes Oct 4, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Oct 4, 2016

CI failures seem to be the usual known problems and not related to this change. We can maybe re-run after #8900 and #8924 land if we want to be especially meticulous.

@jasnell jasnell changed the title Minors improvements Minor improvements Oct 6, 2016
jasnell
jasnell approved these changes Oct 6, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Oct 6, 2016

CI again: https://ci.nodejs.org/job/node-test-pull-request/4409/

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

Bit too much red in that last run... trying again: https://ci.nodejs.org/job/node-test-pull-request/4411/

@jasnell jasnell self-assigned this Oct 7, 2016
jasnell pushed a commit that referenced this pull request Oct 7, 2016
@rmeja @jasnell
lib: minor improvements to bootstrap_node.js
ae9d146 
Change '==' to '==='

PR-URL: #8906
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

Landed in ae9d146.
@rmeja ... just an fyi, I squashed the commit and reworded it so that it matches our contribution guidelines.

@jasnell jasnell closed this Oct 7, 2016
jasnell pushed a commit that referenced this pull request Oct 10, 2016
@rmeja @jasnell
lib: minor improvements to bootstrap_node.js
4cad3da 
Change '==' to '==='

PR-URL: #8906
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@rmeja @Fishrock123
lib: minor improvements to bootstrap_node.js
a1925a7 
Change '==' to '==='

PR-URL: #8906
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlucas evanlucas evanlucas left review comments

@mscdex mscdex mscdex left review comments

@mhdawson mhdawson mhdawson approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@jasnell jasnell

Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@rmeja @mscdex @Trott @jasnell @evanlucas @lpinca @mhdawson @MylesBorins @nodejs-github-bot