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

lib: avoid using Array.prototype.forEach #11582

Closed
wants to merge 11 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 27, 2017

Using Array.prototype.forEach() is still significantly slower than traditional array iteration. This PR adds a benchmark and replaces many (but not all) forEach() uses within lib.

 count  method        rate confidence.interval
     5   array 166.1422406          8.76899908
     5 forEach   7.5412201          0.48229307
    10   array  75.8375035          4.85803234
    10 forEach   4.3405191          0.29654176
    20   array  32.5185750          2.20360039
    20 forEach   2.5431252          0.14517229
   100   array   8.9249754          0.44583628
   100 forEach   0.5816331          0.03092516
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 27, 2017
@richardlau
Copy link
Member

How many forEach's are left? Is it worth having a lint rule to catch?

@bmeurer
Copy link
Member

bmeurer commented Feb 27, 2017

We are aware of this, and work on the Array builtins (forEach, filter, and friends) already started.

@jasnell
Copy link
Member Author

jasnell commented Feb 27, 2017

@richardlau ... only a handful but they are either a bit more complicated to refactor or are otherwise not in code that is that performance critical. At this point I don't believe a lint rule is required.

@bmeurer ... yeah, I figured as much :-) It's a significant enough of a gap right now that making this change makes sense. I've separated each changed file into a separate commit so that we can selectively roll back as necessary once performance improvements are made :-)

@joyeecheung
Copy link
Member

joyeecheung commented Feb 27, 2017

I am guessing https://bugs.chromium.org/p/v8/issues/detail?id=5269 is the related tracking issue? Just linking stuff :).

EDIT: or maybe this: https://bugs.chromium.org/p/v8/issues/detail?id=1956

@jasnell
Copy link
Member Author

jasnell commented Feb 27, 2017

const common = require('../common.js');

const bench = common.createBenchmark(main, {
method: ['array', 'forEach'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just 'for' would be more descriptive since they are both iterating over an array.

@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Feb 27, 2017
@jasnell
Copy link
Member Author

jasnell commented Feb 27, 2017

@mscdex ... I renamed the array to for as suggested and also went ahead and added for-of and for-in variants so that we can track the performance variance on those as well.

@bmeurer
Copy link
Member

bmeurer commented Feb 28, 2017

Relevant upstream bugs are https://bugs.chromium.org/p/v8/issues/detail?id=1956 and https://bugs.chromium.org/p/v8/issues/detail?id=2229. Full parity with for-of/for vs. forEach requires shipping TurboFan for all JS first, so probably V8 5.9 or 6.0 can provide the performance.

@JacksonTian
Copy link
Contributor

Some forEach not in critical path.

@mscdex
Copy link
Contributor

mscdex commented Feb 28, 2017

I've always been in favor of replacing all instances of forEach(), even though others have shot it down in previous PRs by others. IMHO there's really no reason to use forEach(), especially since we now have const which (when used carefully) works for most use cases where you want to scope the variable value to each iteration.

@jasnell
Copy link
Member Author

jasnell commented Mar 4, 2017

Ping @nodejs/collaborators

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

proxiedMethods.forEach(function(name) {
tls_wrap.TLSWrap.prototype[name] = function methodProxy(...args) {
function makeMethodProxy(name) {
return function methodProxy(...args) {
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 curious why ...args is being used when we're just using it with .apply() which should handle arguments just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Either way works for me really

Copy link
Contributor

@mscdex mscdex Mar 15, 2017

Choose a reason for hiding this comment

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

I would vote for changing to using arguments while we're in here.

['GLOBAL', 'root'].forEach(function(name) {
// getter
const get = util.deprecate(function() {
function makeGetter(name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these could be named a bit clearer, like makeDeprecateGetter() or maybe makeDepGetter(), and similarly with makeSetter().

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. Tiniest nit: would you mind using i as the loop variable instead of n. We aren't consistent, but i seems to be used more.

for (i = 0; i < n; i++) {
for (j in items) {
/* eslint-disable no-unused-vars */
item = items[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 doesn't use j at all. Wouldn't this be optimized away?

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, this is just a typo

lib/fs.js Outdated
enumerable: true, value: constants[key] || 0, writable: false
});
Object.defineProperties(fs, {
F_OK: {enumerable: true, writable: false, value: constants.F_OK || 0},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop writable? By default it is false

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

@thefourtheye
Copy link
Contributor

Hope none of the changes here involve sparse arrays. That would change the semantics of the loop iteration.

@jasnell
Copy link
Member Author

jasnell commented Mar 6, 2017

None of the loops appear to involve sparse arrays that I can see.

@jasnell
Copy link
Member Author

jasnell commented Mar 6, 2017

@thefourtheye ... updated to address your feedback. PTAL

@jasnell
Copy link
Member Author

jasnell commented Mar 15, 2017

ping @thefourtheye

Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Belated LGTM.

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas
Copy link
Contributor

@jasnell this PR need backport to v7

@MylesBorins
Copy link
Member

+1 for v6.x backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet