lib: Prevent leaking arguments in several places.

[ChALkeR]

This prevents leaking arguments in several places.

Not yet final, I will probably update this with more patches, waiting for comments.

Currently the three fixed places are util.format (called for example by console.log), console.assert, and path.win32.join.

In path.win32.join this PR changes the stack of the TypeError('Arguments to path.join must be strings'), removing the top at f (path.js:190:13) and at Object.filter (native) so now it is similar to the stack in path.posix.join.

util.format results:

• ~50% speedup on util.format('abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc1234567890') (16700000 ⇒ 32900000 ops/second).
• ~8% speedup on util.format({a:2,b:10,c:10},20,'d') (50000 ⇒ 54000 ops/second).
• ~28% speedup on util.format('Hello %s %s',20,'d') (580000 ⇒ 800000 ops/second).

console.assert results:

• ~27% speedup on common console.assert(true, 'text') (2550000 ⇒ 3230000 ops/second).
• ~23% speedup on console.assert(true) ( 3000000 ⇒ 3700000 ops/second).

path.win32.join results:

• ~33% speedup on path.join('a','b','c') (400000 ⇒ 598000 ops/second).

See #1749 (comment), https://gist.github.com/Fishrock123/98c35a0c745cb59d7496 and https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments

[ChALkeR]

/cc @Fishrock123

[ChALkeR]

The total list of issues found:

• lib/_tls_wrap.js:860: var args = normalizeConnectArgs(arguments);
• lib/path.js:195: var paths = Array.prototype.filter.call(arguments, f); Fixed by this PR.
• lib/net.js:58: var args = normalizeConnectArgs(arguments);
• lib/net.js:859: var args = normalizeConnectArgs(arguments);
• lib/dns.js:64: process.nextTick(callMakeAsyncCbNT, callback, arguments);
• lib/console.js:86: var arr = Array.prototype.slice.call(arguments, 1); Fixed by this PR.
• lib/_http_client.js:562: this._deferToConnect('setNoDelay', arguments);
• lib/_http_client.js:565: this._deferToConnect('setKeepAlive', arguments);
• lib/util.js:19: var args = arguments; Fixed by this PR.
• lib/domain.js:239: return intercepted(this, self, cb, arguments); Deprecated
• lib/domain.js:267: return bound(this, self, cb, arguments); Deprecated
• lib/crypto.js:655: return filterDuplicates(getCiphers.call(null, arguments));
• lib/crypto.js:660: return filterDuplicates(getHashes.call(null, arguments));

I'm not sure if fixing most of those matters, but at least some should be fixed imo.

[Fishrock123]

If I get time, I'll run IRHydra against my patch and see what comes out. That would be the best first step, to compare the actual compiled result.

[brendanashworth]

Perhaps specifying the length up front, var paths = new Array(arguments.length); and assigning at the index would be faster?

[ChALkeR]

@brendanashworth Done.

[chrisdickinson]

A while back, we had support for macro expansion in our native modules. If this is proving to be a generally viable speedup, it would be nice to put it in macro form so that it's easy to rip out again if V8 changes things.

[bnoordhuis]

There's another obvious performance optimization here: cache the result of require('assert'). :-)

[bnoordhuis]

A comment and a question:

1. al is a terrible name, prefer something like argslen or argc. (argc is terrible too but at least it has a well-understood meaning.)
2. Is there a reason to use var instead of const?

[Fishrock123]

@bnoordhuis

2: Iirc last time we checked const is still slower than var.

[bnoordhuis]

If you're referring to the OSR issue, I think I only saw that with let, not const. Also, it was a while ago, it would be good to retest.

[brendanashworth]

this jsperf shows that const is faster than var right now

[ChALkeR]

@Fishrock123, @bnoordhuis Compound assignment deoptimizes let: let x = 10; x += 1; is slow in both 4.2 and 4.3, but let x = 10; x = x + 1; is fine. This shouldn't be an issue for const because you don't want compound assigments with const =).

[ChALkeR]

@bnoordhuis Done:

• assert is now cached;
• al got renamed to argsleft because that's the number of not yet processed/copied arguments;
• arr and paths are now const.

[ChALkeR]

@brendanashworth Ow. That jsperf shows that var is faster than const right in Chromium 43.
Will test in iojs + 4.2 and iojs + 4.3 in a moment.
Edit: I would say that there is no difference between var and const performance for the testcase supplied above (as the jsperf link) in iojs + 4.2 or 4.3.

[Fishrock123]

We should probably move to preferring const (where correct/possible) in all new patches then.

[bnoordhuis]

LGTM

[ChALkeR]

Should I patch other places from #1752 (comment) ?

[Fishrock123]

@ChALkeR yes please :)

[trevnorris]

The let performance patch landed in v8 4.4.30.

[chrisdickinson]

I'd be a lot more comfortable with this if it were reintroduced as a macro. It'd be a lot easier to document / spread knowledge about its use, as well as to back out of it if perf characteristics change.

[ChALkeR]

@chrisdickinson This is a temporary solution anyways.
The proper way would be using rest params.

Related: https://code.google.com/p/v8/issues/detail?id=2159
Once this is properly implemented and optimized (as it should be), it should be used instead of arguments and copy-arguments-into-an-array methods.

A macro (and documenting it) seems like a bit undue for a temporary hack.

[trevnorris]

@ChALkeR In my experience rest params are already almost on par with using arguments. It's the spread operator that will eat your face.

[ChALkeR]

@trevnorris arguments is not an array, also it introduces a deopt if you do anything to it except taking arguments.length (in fact this also should be done carefully) and accessing arguments[i] where 0 <= i < arguments.length. This is why manually copying arguments to an array and passing that array is faster than passing the arguments object.

«On par with arguments» is not good enough. Afaik, rest params are deoptimized (or, rather, not yet optimized) for now. Also, I am not sure what is the v8 version requirements for them to work without issues. And they are behind a flag.

See https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments.

[trevnorris]

@ChALkeR I'm aware of all that. Was just stating that rest params being well optimized isn't far off.

[ChALkeR]

@trevnorris Yes. And that's why I don't see an advantage in hacking this with something more complex, as macro (and documenting it), as @chrisdickinson proposed here: #1752 (comment)

[jasnell]

@ChALkeR @trevnorris ... what's the status on this?

[ChALkeR]

@jasnell I need to re-apply this (some places mentioned in the above list were already fixed by other patches), and then re-test to see if there is any actual improvement with the current v8 version in master.

[ChALkeR]

Closing, obsolete. See #4361, also current master has rest parameters enabled by default, perhaps this should be redone using those.