lib: copy arguments object instead of leaking it by nwoltman · Pull Request #4361 · nodejs/node · GitHub

Instead of leaking the arguments object by passing it as an argument to a function, copy it's contents to a new array, then pass the array. This allows V8 to optimize the function that contains this code, improving performance.

Benchmarks

Using benchmark.js

ClientRequest.setNoDelay / ClientRequest.setSocketKeepAlive

before: 15,286,485 ops/sec
after: 20,160,118 ops/sec
~33% faster

net.connect / tls.connect

before: 315,000 ops/sec
after: 345,000 ops/sec
~10% faster

code:

net.connect({port: 80, host: 'google.com'}, () => {});
// same for tls

assert.throws

before: 98,403 ops/sec 89,592 ops/sec
after: 145,609 ops/sec 146,464 ops/sec

code:

``` js
function throws() {
  throw Error('error message');
}
// in test:
assert.throws(throws);
```
#### assert.doesNotThrow

before: ~~495,686 ops/sec~~ 424,867 ops/sec
after: ~~7,807,355 ops/sec~~ 14,513,552 ops/sec
~~~1475% faster~~ ~3316% faster

code:

``` js
function doesNotThrow() {
  return 1 + 3;
}
// in test:
assert.doesNotThrow(doesNotThrow);
```
#### console.assert

before: 2,498,701 ops/sec
after: 4,163,156 ops/sec
~67% faster

code:

``` js
console.assert(true);
```

Why not do new Array() and copy the items like the previous changes?

I thought it might look a little messy since there would need to be a couple - 1s since the first item is being sliced off, but I agree it would be better to make it more similar to the other instances.

Instead of leaking the arguments object by passing it as an argument to a function, copy it's contents to a new array, then pass the array

intuitively, asking the system to make extra copies does not seem to be the sort of programming that results in such speedup. of course the v8 compiler performs code transforms on our behalf as well as other robust forms of alchemy.

I heard the arrow functions are an improvement on regular bind() latency, you mentioned doing:

net.connect({port: 80, host: 'google.com'}, () => {});

can you point me to the benchmark code, I would like to check it out?

@reqshark I'll admit that benchmark is a little sketchy. At first I tried making the benchmark asynchronous where I would use the callback to close the connection and then signal that the benchmark test was complete, but that caused a bunch of errors related to opening too many connections in a short amount of time. What I ended up doing is placing a return statement inside the net/tls.connect function to return before actually opening a connection and then just running that code that I provided. It's not an accurate benchmark. It's mainly just to show that there is some performance improvement.

@woollybogger You are targeting the master branch, which already has v8 4.7.
v8 4.7 ships rest parameters enabled by default.

Perhaps using those would be better/faster than copying arguments?

is this something we want to backport? If so using rest parameters would make that more difficult

@thealphanerd Is there a reason not to use rest parameters in master for the sake of backporting to other branches? I don't think so.

If rest parameters are good enough, those should be used in master. If older branches need to be fixed, those should use arguments copying approach, imo.

Rest parameters should turn this:

Console.prototype.assert = function(expression) {
  if (!expression) {
    var argsLength = arguments.length || 1;
    var arr = new Array(argsLength - 1);
    for (var i = 1; i < argsLength; i++) {
      arr[i - 1] = arguments[i];
    }
    require('assert').ok(false, util.format.apply(this, arr));
  }
}

into this:

Console.prototype.assert = function(expression, ...arr) {
  if (!expression) {
    require('assert').ok(false, util.format.apply(this, arr));
  }
}

I thought rest parameters were still slow in v8?

@mscdex In a single micro-benchmark, rest parameters are indeed about 2% slower than copying arguments with Node.js v5.3.0 (v8 4.6). I have not tried various cases, though. But they were not shipped by default then, and I did not check master branch (v8 4.7) yet.

@mscdex Tested master branch, v8 4.7, and a few different cases (no arguments, several arguments, various types). It appears that you are correct and that rest parameters are indeed slow atm.

@woollybogger Disregard my previous comments about rest parameters for now, perhaps those will be good to use in some future v8 version.

While you're at it, I suggest just optimizing as much as possible:

https://gist.github.com/Fishrock123/98c35a0c745cb59d7496

I thought traversing/setting arrays forwards was now faster than going backwards (see #3976)?

I'm not sure about backwards vs. forwards, but generally I've seen that for loops are faster than while loops in V8. Take a look at this benchmark I just ran (I put some of my results in a comment at the bottom): https://gist.github.com/woollybogger/7364d4cf4599a1fc0ae7

(It also just makes the statement smaller)

The size of the statement really just depends on coding style. I could make it the same number of lines as the while loop if I changed it to this:

var argsLength = arguments.length;
var args = new Array(argsLength);
for (var i = 0; i < argsLength; i++) args[i] = arguments[i];

Would that style be preferable?

IMHO I've always found that style (putting if/loop/etc. bodies on the same line) less readable when visually scanning code, but I would suggest removing the braces for one line bodies.

Alright, I'll remove the braces throughout the PR.

Marking as a watch for v4.x but we'll need to see how this ends up before landing it there.

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

Are there 4 Windows test runs to show that at least one of them can pass?

@woollybogger ... it's kind of like playing the powerball lottery, if you roll the dice and get all green, you win big! ... unfortunately there's some current wonkiness with the windows machines due to a number of issues (a bad commit that's being looked at + unrelated build machine failures).

given that LTS is mostly green and the failures look unrelated, LGTM!

Cool 👍

Bump. Resolved merge conflicts.

I completely rewrote my changes for assert.js and updated the benchmark results.

minor nit: these can be const now

LGTM with a nit

Instead of leaking the arguments object by passing it as an argument to a function, copy it's contents to a new array, then pass the array. This allows V8 to optimize the function that contains this code, improving performance.

Notable changes: * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

Notable changes: * **contextify**: Fixed a memory consumption issue related to heavy use of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh) #5392 * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231 - breakout events from v8 to offer better support for external debuggers * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

Instead of leaking the arguments object by passing it as an argument to a function, copy it's contents to a new array, then pass the array. This allows V8 to optimize the function that contains this code, improving performance. PR-URL: #4361 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Notable Changes * https: - Under certain conditions ssl sockets may have been causing a memory leak when keepalive is enabled. This is no longer the case. - (Alexander Penev) #5713 * lib: - The way that we were internally passing arguments was causing a potential leak. By copying the arguments into an array we can avoid this - (Nathan Woltman) #4361 * repl: - Previously if you were using the repl in strict mode the column number would be wrong in a stack trace. This is no longer an issue. - (Prince J Wesley) #5416 PR-URL: #5961

Notable Changes * https: - Under certain conditions ssl sockets may have been causing a memory leak when keepalive is enabled. This is no longer the case. - (Alexander Penev) #5713 * lib: - The way that we were internally passing arguments was causing a potential leak. By copying the arguments into an array we can avoid this - (Nathan Woltman) #4361 * npm: - Upgrade to v2.15.1. (Forrest L Norvell) * repl: - Previously if you were using the repl in strict mode the column number would be wrong in a stack trace. This is no longer an issue. - (Prince J Wesley) #5416 PR-URL: #5961

Notable Changes * https: - Under certain conditions ssl sockets may have been causing a memory leak when keepalive is enabled. This is no longer the case. - (Alexander Penev) #5713 * lib: - The way that we were internally passing arguments was causing a potential leak. By copying the arguments into an array we can avoid this - (Nathan Woltman) #4361 * npm: - Upgrade to v2.15.1. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) * repl: - Previously if you were using the repl in strict mode the column number would be wrong in a stack trace. This is no longer an issue. - (Prince J Wesley) #5416 PR-URL: #5961

mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 20, 2015

mscdex reviewed Dec 20, 2015
View reviewed changes

nwoltman force-pushed the no-arguments branch from 06019e0 to abe2657 Compare December 20, 2015 17:11

ChALkeR mentioned this pull request Dec 20, 2015

lib: Prevent leaking arguments in several places. #1752

Closed

Fishrock123 reviewed Dec 21, 2015
View reviewed changes

nwoltman force-pushed the no-arguments branch from abe2657 to ab61135 Compare December 21, 2015 23:42

jasnell added the lts-watch-v4.x label Dec 24, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

nwoltman force-pushed the no-arguments branch from ab61135 to 3a9477e Compare February 19, 2016 02:51

nwoltman force-pushed the no-arguments branch from 3a9477e to 89d19b5 Compare February 24, 2016 02:47

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 24, 2016

jasnell reviewed Mar 2, 2016
View reviewed changes

lib: copy arguments object instead of leaking it

b557517

Instead of leaking the arguments object by passing it as an argument to a function, copy it's contents to a new array, then pass the array. This allows V8 to optimize the function that contains this code, improving performance.

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

This was referenced Mar 30, 2016

v4.4.2 proposal #5961

Merged

V4.4.1 proposal #5835

Merged

leonardoadame mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015

Open

leonardoadame mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 leonardoadame/node#646

Open

aliscco mentioned this pull request Feb 12, 2024

[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 aliscco/alisco-node#1250

Open

lib: copy arguments object instead of leaking it by nwoltman · Pull Request #4361 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: copy arguments object instead of leaking it - #4361

lib: copy arguments object instead of leaking it#4361nwoltman wants to merge 1 commit intonodejs:masternodejs/node:masterfrom nwoltman:no-argumentsnwoltman/node:no-argumentsCopy head branch name to clipboard

Conversation

nwoltman commented Dec 20, 2015

Benchmarks

ClientRequest.setNoDelay / ClientRequest.setSocketKeepAlive

net.connect / tls.connect

assert.throws

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reqshark commented Dec 20, 2015

Uh oh!

nwoltman commented Dec 20, 2015

Uh oh!

ChALkeR commented Dec 20, 2015

Uh oh!

MylesBorins commented Dec 20, 2015

Uh oh!

ChALkeR commented Dec 20, 2015

Uh oh!

ChALkeR commented Dec 20, 2015

Uh oh!

mscdex commented Dec 20, 2015

Uh oh!

ChALkeR commented Dec 20, 2015

Uh oh!

ChALkeR commented Dec 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 24, 2015

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

nwoltman commented Jan 15, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

nwoltman commented Jan 16, 2016

Uh oh!

nwoltman commented Feb 19, 2016

Uh oh!

nwoltman commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lib: copy arguments object instead of leaking it#4361
nwoltman wants to merge 1 commit into
nodejs:masterfrom
nwoltman:no-arguments