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
82 changes: 82 additions & 0 deletions benchmark/es/foreach-bench.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
'use strict';

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

const bench = common.createBenchmark(main, {
method: ['for', 'for-of', 'for-in', 'forEach'],
count: [5, 10, 20, 100],
millions: [5]
});

function useFor(n, items, count) {
var i, j;
bench.start();
for (i = 0; i < n; i++) {
for (j = 0; j < count; j++) {
/* eslint-disable no-unused-vars */
var item = items[j];
/* esline-enable no-unused-vars */
}
}
bench.end(n / 1e6);
}

function useForOf(n, items) {
var i, item;
bench.start();
for (i = 0; i < n; i++) {
for (item of items) {}
}
bench.end(n / 1e6);
}

function useForIn(n, items) {
var i, j, item;
bench.start();
for (i = 0; i < n; i++) {
for (j in items) {
/* eslint-disable no-unused-vars */
item = items[j];
/* esline-enable no-unused-vars */
}
}
bench.end(n / 1e6);
}

function useForEach(n, items) {
var i;
bench.start();
for (i = 0; i < n; i++) {
items.forEach((item) => {});
}
bench.end(n / 1e6);
}

function main(conf) {
const n = +conf.millions * 1e6;
const count = +conf.count;

const items = new Array(count);
var i;
var fn;
for (i = 0; i < count; i++)
items[i] = i;

switch (conf.method) {
case 'for':
fn = useFor;
break;
case 'for-of':
fn = useForOf;
break;
case 'for-in':
fn = useForIn;
break;
case 'forEach':
fn = useForEach;
break;
default:
throw new Error('Unexpected method');
}
fn(n, items, count);
}
9 changes: 5 additions & 4 deletions lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ var StringDecoder;

util.inherits(Readable, Stream);

const kProxyEvents = ['error', 'close', 'destroy', 'pause', 'resume'];

function prependListener(emitter, event, fn) {
// Sadly this is not cacheable as some libraries bundle their own
// event emitter implementation with them.
Expand Down Expand Up @@ -828,10 +830,9 @@ Readable.prototype.wrap = function(stream) {
}

// proxy certain important events.
const events = ['error', 'close', 'destroy', 'pause', 'resume'];
events.forEach(function(ev) {
stream.on(ev, self.emit.bind(self, ev));
});
for (var n = 0; n < kProxyEvents.length; n++) {
stream.on(kProxyEvents[n], self.emit.bind(self, kProxyEvents[n]));
}

// when we try to consume some more bytes, simply unpause the
// underlying stream.
Expand Down
5 changes: 2 additions & 3 deletions lib/_stream_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,8 @@ StreamWrap.prototype.doWrite = function doWrite(req, bufs) {
const item = self._enqueue('write', req);

self.stream.cork();
bufs.forEach(function(buf) {
self.stream.write(buf, done);
});
for (var n = 0; n < bufs.length; n++)
self.stream.write(bufs[n], done);
self.stream.uncork();

function done(err) {
Expand Down
10 changes: 7 additions & 3 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,12 +321,16 @@ var proxiedMethods = [
];

// Proxy HandleWrap, PipeWrap and TCPWrap methods
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.

if (this._parent[name])
return this._parent[name].apply(this._parent, args);
};
});
}
for (var n = 0; n < proxiedMethods.length; n++) {
tls_wrap.TLSWrap.prototype[proxiedMethods[n]] =
makeMethodProxy(proxiedMethods[n]);
}

tls_wrap.TLSWrap.prototype.close = function close(cb) {
let ssl;
Expand Down
9 changes: 5 additions & 4 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,11 @@ function statsFromValues() {
}

// Don't allow mode to accidentally be overwritten.
['F_OK', 'R_OK', 'W_OK', 'X_OK'].forEach(function(key) {
Object.defineProperty(fs, key, {
enumerable: true, value: constants[key] || 0, writable: false
});
Object.defineProperties(fs, {
F_OK: {enumerable: true, value: constants.F_OK || 0},
R_OK: {enumerable: true, value: constants.R_OK || 0},
W_OK: {enumerable: true, value: constants.W_OK || 0},
X_OK: {enumerable: true, value: constants.X_OK || 0},
});

function handleError(val, callback) {
Expand Down
65 changes: 40 additions & 25 deletions lib/internal/bootstrap_node.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,23 +206,34 @@
global.process = process;
const util = NativeModule.require('util');

// Deprecate GLOBAL and root
['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().

return util.deprecate(function() {
return this;
}, `'${name}' is deprecated, use 'global'`, 'DEP0016');
// setter
const set = util.deprecate(function(value) {
}

function makeSetter(name) {
return util.deprecate(function(value) {
Object.defineProperty(this, name, {
configurable: true,
writable: true,
enumerable: true,
value: value
});
}, `'${name}' is deprecated, use 'global'`, 'DEP0016');
// define property
Object.defineProperty(global, name, { get, set, configurable: true });
}

Object.defineProperties(global, {
GLOBAL: {
configurable: true,
get: makeGetter('GLOBAL'),
set: makeSetter('GLOBAL')
},
root: {
configurable: true,
get: makeGetter('root'),
set: makeSetter('root')
}
});

global.Buffer = NativeModule.require('buffer').Buffer;
Expand Down Expand Up @@ -326,27 +337,31 @@
// With no argument, getVersion() returns a comma separated list
// of possible types.
const versionTypes = icu.getVersion().split(',');
versionTypes.forEach((name) => {
// Copied from module.js:addBuiltinLibsToObject

function makeGetter(name) {
return () => {
// With an argument, getVersion(type) returns
// the actual version string.
const version = icu.getVersion(name);
// Replace the current getter with a new property.
delete process.versions[name];
Object.defineProperty(process.versions, name, {
value: version,
writable: false,
enumerable: true
});
return version;
};
}

for (var n = 0; n < versionTypes.length; n++) {
var name = versionTypes[n];
Object.defineProperty(process.versions, name, {
configurable: true,
enumerable: true,
get: () => {
// With an argument, getVersion(type) returns
// the actual version string.
const version = icu.getVersion(name);
// Replace the current getter with a new
// property.
delete process.versions[name];
Object.defineProperty(process.versions, name, {
value: version,
writable: false,
enumerable: true
});
return version;
}
get: makeGetter(name)
});
});
}
}

function tryGetCwd(path) {
Expand Down
66 changes: 41 additions & 25 deletions lib/internal/streams/lazy_transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,47 @@ function LazyTransform(options) {
}
util.inherits(LazyTransform, stream.Transform);

[
'_readableState',
'_writableState',
'_transformState'
].forEach(function(prop, i, props) {
Object.defineProperty(LazyTransform.prototype, prop, {
get: function() {
stream.Transform.call(this, this._options);
this._writableState.decodeStrings = false;

if (!this._options || !this._options.defaultEncoding) {
this._writableState.defaultEncoding = crypto.DEFAULT_ENCODING;
}

return this[prop];
},
set: function(val) {
Object.defineProperty(this, prop, {
value: val,
enumerable: true,
configurable: true,
writable: true
});
},
function makeGetter(name) {
return function() {
stream.Transform.call(this, this._options);
this._writableState.decodeStrings = false;

if (!this._options || !this._options.defaultEncoding) {
this._writableState.defaultEncoding = crypto.DEFAULT_ENCODING;
}

return this[name];
};
}

function makeSetter(name) {
return function(val) {
Object.defineProperty(this, name, {
value: val,
enumerable: true,
configurable: true,
writable: true
});
};
}

Object.defineProperties(LazyTransform.prototype, {
_readableState: {
get: makeGetter('_readableState'),
set: makeSetter('_readableState'),
configurable: true,
enumerable: true
},
_writableState: {
get: makeGetter('_writableState'),
set: makeSetter('_writableState'),
configurable: true,
enumerable: true
},
_transformState: {
get: makeGetter('_transformState'),
set: makeSetter('_transformState'),
configurable: true,
enumerable: true
});
}
});
5 changes: 2 additions & 3 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -676,9 +676,8 @@ Module._preloadModules = function(requests) {
throw e;
}
}
requests.forEach(function(request) {
parent.require(request);
});
for (var n = 0; n < requests.length; n++)
parent.require(requests[n]);
};

Module._initPaths();
Expand Down
11 changes: 5 additions & 6 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -1524,9 +1524,9 @@ Server.prototype.getConnections = function(cb) {
if (--left === 0) return end(null, total);
}

this._slaves.forEach(function(slave) {
slave.getConnections(oncount);
});
for (var n = 0; n < this._slaves.length; n++) {
this._slaves[n].getConnections(oncount);
}
};


Expand Down Expand Up @@ -1562,9 +1562,8 @@ Server.prototype.close = function(cb) {
this._connections++;

// Poll slaves
this._slaves.forEach(function(slave) {
slave.close(onSlaveClose);
});
for (var n = 0; n < this._slaves.length; n++)
this._slaves[n].close(onSlaveClose);
} else {
this._emitCloseIfDrained();
}
Expand Down
5 changes: 2 additions & 3 deletions lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,8 @@ Interface.prototype._normalWrite = function(b) {
// either '' or (conceivably) the unfinished portion of the next line
string = lines.pop();
this._line_buffer = string;
lines.forEach(function(line) {
this._onLine(line);
}, this);
for (var n = 0; n < lines.length; n++)
this._onLine(lines[n]);
} else if (string) {
// no newlines this time, save what we have for next time
this._line_buffer = string;
Expand Down
Loading