-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b2c00ce
benchmark: benchmark comparing forEach with for
jasnell 4d0d400
stream: avoid using forEach
jasnell a2d8baa
tls: avoid using forEach
jasnell ebe8926
fs: avoid using forEach
jasnell 36dc38a
lib: avoid using forEach in LazyTransform
jasnell abdd6c6
net: avoid using forEach
jasnell e8738ca
readline: avoid using forEach
jasnell 59d1fd6
repl: avoid using forEach
jasnell e9e5c0e
module: avoid using forEach
jasnell 2e0cb91
util: avoid using forEach
jasnell 142bf54
lib: avoid using forEach
jasnell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps these could be named a bit clearer, like |
||
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; | ||
|
@@ -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) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 handlearguments
just fine.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.