lib: avoid using Array.prototype.forEach #11582

jasnell · 2017-02-27T18:19:15Z

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

richardlau · 2017-02-27T18:35:40Z

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

bmeurer · 2017-02-27T18:41:09Z

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

jasnell · 2017-02-27T18:47:01Z

@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 · 2017-02-27T18:59:52Z

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 · 2017-02-27T22:11:03Z

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

mscdex · 2017-02-27T22:19:13Z

benchmark/es/foreach-bench.js

+const common = require('../common.js');
+
+const bench = common.createBenchmark(main, {
+  method: ['array', 'forEach'],


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

jasnell · 2017-02-27T22:55:29Z

@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 · 2017-02-28T04:02:15Z

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 · 2017-02-28T04:24:54Z

Some forEach not in critical path.

mscdex · 2017-02-28T04:29:26Z

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 · 2017-03-04T22:19:25Z

Ping @nodejs/collaborators

mcollina

LGTM

mscdex · 2017-03-04T22:30:54Z

lib/_tls_wrap.js

-proxiedMethods.forEach(function(name) {
-  tls_wrap.TLSWrap.prototype[name] = function methodProxy(...args) {
+function makeMethodProxy(name) {
+  return function methodProxy(...args) {


I'm curious why ...args is being used when we're just using it with .apply() which should handle arguments just fine.

Either way works for me really

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

mscdex · 2017-03-04T22:32:35Z

lib/internal/bootstrap_node.js

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


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

cjihrig

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.

thefourtheye · 2017-03-05T19:13:24Z

benchmark/es/foreach-bench.js

+  for (i = 0; i < n; i++) {
+    for (j in items) {
+      /* eslint-disable no-unused-vars */
+      item = items[i];


This doesn't use j at all. Wouldn't this be optimized away?

whoops, this is just a typo

thefourtheye · 2017-03-05T19:15:49Z

lib/fs.js

-    enumerable: true, value: constants[key] || 0, writable: false
-  });
+Object.defineProperties(fs, {
+  F_OK: {enumerable: true, writable: false, value: constants.F_OK || 0},


Can we drop writable? By default it is false

thefourtheye · 2017-03-05T19:20:24Z

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

jasnell · 2017-03-06T21:39:17Z

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

jasnell · 2017-03-06T21:40:07Z

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

jasnell · 2017-03-15T05:26:20Z

ping @thefourtheye

thefourtheye

Sorry for the delay. Belated LGTM.

PR-URL: nodejs#11582 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

italoacasas · 2017-03-21T14:22:59Z

@jasnell this PR need backport to v7

MylesBorins · 2017-04-17T22:49:30Z

+1 for v6.x backport

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

mscdex reviewed Feb 27, 2017

View reviewed changes

mscdex added the performance Issues and PRs related to the performance of Node.js. label Feb 27, 2017

jasnell force-pushed the avoid-for-each branch from 9f7a58c to 32a726a Compare February 27, 2017 22:54

mcollina approved these changes Mar 4, 2017

View reviewed changes

mscdex reviewed Mar 4, 2017

View reviewed changes

cjihrig approved these changes Mar 4, 2017

View reviewed changes

thefourtheye reviewed Mar 5, 2017

View reviewed changes

jasnell force-pushed the avoid-for-each branch from 32a726a to 109e334 Compare March 6, 2017 21:39

jasnell added 5 commits March 14, 2017 22:30

benchmark: benchmark comparing forEach with for

b2c00ce

stream: avoid using forEach

4d0d400

tls: avoid using forEach

a2d8baa

fs: avoid using forEach

ebe8926

lib: avoid using forEach in LazyTransform

36dc38a

thefourtheye reviewed Mar 18, 2017

View reviewed changes

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017

util: avoid using forEach

9a59913

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

tls: avoid using forEach

646ee55

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

stream: avoid using forEach

90be5a1

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

repl: avoid using forEach

62e7261

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

readline: avoid using forEach

a0b1aa1

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

net: avoid using forEach

c0a2e02

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

module: avoid using forEach

7e23072

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

fs: avoid using forEach

d0fb578

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

benchmark: benchmark comparing forEach with for

dcac2d8

PR-URL: nodejs#11582 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

italoacasas mentioned this pull request Mar 20, 2017

v7.7.4 Release Proposal #11941

Merged

jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017

benchmark: benchmark comparing forEach with for

fd3d1b4

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

stream: avoid using forEach

72ff71e

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

tls: avoid using forEach

d76080d

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

fs: avoid using forEach

d3c9ca0

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

lib: avoid using forEach in LazyTransform

5b707bc

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

net: avoid using forEach

ac0a25a

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

readline: avoid using forEach

8abe291

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

repl: avoid using forEach

65e66a3

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

module: avoid using forEach

c100130

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

util: avoid using forEach

09dbaef

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

lib: avoid using forEach

5b0fd7e

PR-URL: nodejs#11582 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added backport-to-v6.x labels Mar 21, 2017

jasnell self-assigned this Mar 21, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn removed the backport-requested-v7.x label Jun 14, 2017

ZYSzys mentioned this pull request Nov 29, 2018

tls: use for loop instead of forEach #24715

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: avoid using Array.prototype.forEach #11582

lib: avoid using Array.prototype.forEach #11582

jasnell commented Feb 27, 2017

richardlau commented Feb 27, 2017

bmeurer commented Feb 27, 2017

jasnell commented Feb 27, 2017

joyeecheung commented Feb 27, 2017 •

edited

Loading

jasnell commented Feb 27, 2017

mscdex Feb 27, 2017

jasnell commented Feb 27, 2017

bmeurer commented Feb 28, 2017

JacksonTian commented Feb 28, 2017

mscdex commented Feb 28, 2017 •

edited

Loading

jasnell commented Mar 4, 2017

mcollina left a comment

mscdex Mar 4, 2017

jasnell Mar 5, 2017

mscdex Mar 15, 2017 •

edited

Loading

mscdex Mar 4, 2017

cjihrig left a comment

thefourtheye Mar 5, 2017

jasnell Mar 6, 2017

thefourtheye Mar 5, 2017

jasnell Mar 6, 2017

thefourtheye commented Mar 5, 2017

jasnell commented Mar 6, 2017

jasnell commented Mar 6, 2017

jasnell commented Mar 15, 2017

thefourtheye left a comment

italoacasas commented Mar 21, 2017

MylesBorins commented Apr 17, 2017

lib: avoid using Array.prototype.forEach #11582

lib: avoid using Array.prototype.forEach #11582

Conversation

jasnell commented Feb 27, 2017

Checklist

Affected core subsystem(s)

richardlau commented Feb 27, 2017

bmeurer commented Feb 27, 2017

jasnell commented Feb 27, 2017

joyeecheung commented Feb 27, 2017 • edited Loading

jasnell commented Feb 27, 2017

Choose a reason for hiding this comment

jasnell commented Feb 27, 2017

bmeurer commented Feb 28, 2017

JacksonTian commented Feb 28, 2017

mscdex commented Feb 28, 2017 • edited Loading

jasnell commented Mar 4, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Mar 5, 2017

jasnell commented Mar 6, 2017

jasnell commented Mar 6, 2017

jasnell commented Mar 15, 2017

thefourtheye left a comment

Choose a reason for hiding this comment

italoacasas commented Mar 21, 2017

MylesBorins commented Apr 17, 2017

joyeecheung commented Feb 27, 2017 •

edited

Loading

mscdex commented Feb 28, 2017 •

edited

Loading

mscdex Mar 15, 2017 •

edited

Loading