Another case of possibly deoptimizing use of `arguments` in libs

[vsemozhetbyt]

In this issue I've missed a case which is much more difficult to trace. That report has caused some fixes, so maybe this is also worth reporting.

However, some disclaimer is needed: I could get the theory or its application wrong here; I could miss or detect wrongly some cases; I am not sure if optimization is worth refactoring in each case. Moreover, spread is increasingly gaining speed now, so maybe upgrade to it is more worth efforts.

I. Definition

Leaking arguments is considered safe only within func.apply(context, arguments). However, this call site must be monomorphic. See the last warning in this chapter.

To be monomorphic, func must have the same hidden class. According to this comment, these functions have different hidden classes:

• function with an additional property / function without this property
• common function / bound function (via bind())
• common function / arrow function
• strict mode function / sloppy mode function

So, each func.apply(context, arguments) fragment should be checked for func monomorphism.

II. Example of deoptimization

This is the code to compare two cases with monomorphic and polymorphic call sites. In the comment below the code, you can see the time difference and deoptimization warning.

III. Types of cases

If I get it right, there are three group of cases here (from the most dangerous to the least dangerous):

1. func is an argument gotten from a userland via public API (directly or via a call chain). We can't guarantee monomorphism here (for example, a user can give both common and arrow functions as arguments). So maybe in these cases arguments should be copied before the call, as in many other lib places.

2. func is a declared public method. Here is a small danger still: a user can add a property to the public function at any time. Therefore, maybe these cases are worth to be refactored as well.

3. func is a private function (declared or given as direct/indirect argument). The monomorphism of this func should be checked across all the code base chains.

IV. Occurrences in Node.js libs

1. func is an argument gotten from a userland via public API

• dns.js: callback in makeAsync()/ asyncCallback() is given from exports.lookup(), exports.lookupService() and possibly via many exports.resolve***() functions (fixed).
• events.js: this.listener in onceWrapper() is given from this chain: _onceWrap() <- EventEmitter.prototype.once() or EventEmitter.prototype.prependOnceListener(). (fixing seems to cause performance downgrading)
• fs.js: cb in anonymous / makeCallback() is given from many (more than 20) fs.***() (i.e. exports.***()) functions. (this anonymous ones are one-time unoptimizable functions)
• repl.js: this.completer in REPLServer.prototype.complete() is given from this chain: REPLServer() <- exports.start().
• _stream_readable.js: stream[method] in Readable.prototype.wrap(stream) is given directly.
• internal/util.js: exports.deprecate(fn, msg, code) and fn in exports.deprecate() is given from util.deprecate() (under consideration).

2. func is a declared public method

• net.js: stream.Duplex.prototype.write in Socket.prototype.write()
• util.js: exports.format in debugs[set]() / exports.debuglog()
• util.js:- exports.format in exports.log()
• _http_server.js: this.writeHead in ServerResponse.prototype.writeHeader()
• internal/process.js: require('util').format in process._rawDebug() / setupRawDebug()

3. func is a private function. Most detected cases seem to be OK.

In addition, there are some cases I could not define which group they belong to and if they are monomorphic. I can't trace all the chain here (got out of memory and stack overflow in my mind).

• _tls_wrap.js: this._parent[name] in tls_wrap.TLSWrap.prototype[name]() (fixed)
• internal/cluster/utils.js: fn in module.exports.internal()
• internal/cluster/child.js: close in handle.close() / shared()
• internal/cluster/worker.js: this.destroy in Worker.prototype.kill().
• internal/cluster/worker.js: this.process.send in Worker.prototype.send()

Sorry for my bad English an overall mess in explanation.

// cc @nodejs/v8 , @bmeurer , @vhf