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

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

@richardlau

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

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

@bmeurer

This comment has been minimized.

Copy link
Member

commented Feb 27, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

benchmark/es/foreach-bench.js Outdated
const common = require('../common.js');

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

This comment has been minimized.

Copy link
@mscdex

mscdex Feb 27, 2017

Contributor

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

@mscdex mscdex added the performance label Feb 27, 2017

@jasnell jasnell force-pushed the jasnell:avoid-for-each branch Feb 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

Some forEach not in critical path.

@mscdex

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

@mcollina
Copy link
Member

left a comment

LGTM

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

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 4, 2017

Contributor

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

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 5, 2017

Author Member

Either way works for me really

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 15, 2017

Contributor

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) {

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 4, 2017

Contributor

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

@cjihrig
cjihrig approved these changes Mar 4, 2017
Copy link
Contributor

left a comment

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.

benchmark/es/foreach-bench.js Outdated
for (i = 0; i < n; i++) {
for (j in items) {
/* eslint-disable no-unused-vars */
item = items[i];

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Mar 5, 2017

Contributor

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

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 6, 2017

Author Member

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},

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Mar 5, 2017

Contributor

Can we drop writable? By default it is false

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 6, 2017

Author Member

Ok

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

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

@jasnell jasnell force-pushed the jasnell:avoid-for-each branch Mar 6, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

@thefourtheye
Copy link
Contributor

left a comment

Sorry for the delay. Belated LGTM.

italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
util: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
tls: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
stream: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
repl: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
readline: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
net: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
module: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
fs: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 20, 2017
benchmark: benchmark comparing forEach with for
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
benchmark: benchmark comparing forEach with for
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
stream: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
tls: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
fs: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
lib: avoid using forEach in LazyTransform
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
net: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
readline: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
repl: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
module: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
util: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
lib: avoid using forEach
PR-URL: nodejs#11582
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

@jasnell this PR need backport to v7

@jasnell jasnell self-assigned this Mar 21, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

+1 for v6.x backport

@ZYSzys ZYSzys referenced this pull request Nov 29, 2018
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.