domain: add arguments for the function in domain.run()

[micnic]

PR for nodejs/node-v0.x-archive#7193

[rvagg]

fyi this is unnecessary, just use arguments in the fn.apply()

[bnoordhuis]

It looks correct to me. It needs to slice off the fn argument, doesn't it?

[micnic]

@rvagg, what @bnoordhuis says is correct, the function needs all the arguments but the first, which is the function itself.

[rvagg]

Sorry, didn't notice the 1

[vkurchatkin]

Shouldn't it be the same as https://github.com/iojs/io.js/blob/v0.12/lib/events.js#L103 ?

[bnoordhuis]

LGTM FWIW.

[bnoordhuis]

I suppose one question that needs answering is whether domain.run() is performance-sensitive enough that adding a slice+apply introduces noticeable overhead. I'm guessing the answer is 'no' but I don't use domains enough to really know for sure. Opinions?

@micnic Can you make the commit log conform to the template as described in CONTRIBUTING.md? Thanks.

[vkurchatkin]

@bnoordhuis I agree. Also it's possible just to call enter() and exit() manually

[micnic]

@bnoordhuis, I'll add a switch for different cases as @vkurchatkin proposes.

related to the commit log, should I make a new pull request or just another commit with a more verbose description? (it's my first PR)

[bnoordhuis]

@micnic That's a good idea but can you then also add a small benchmark in benchmarks/ that shows that the switch statement actually speeds things up? Thanks.

As to the commit log, you can just git commit --amend it and and force-push it to your feature branch (or git rebase -i it when you add that benchmark.)

[micnic]

@bnoordhuis, I added the benchmark and made some changes to the arguments slicing logic to:

if(arguments.length>=2){args=Array.prototype.slice.call(arguments,1);ret=fn.apply(this,args);}else{ret=fn.call(this);}

it's similar to what is used in setInterval() or setTimeout, from the benchmark I did not observe any performance impact.

Now, what I discovered is that we also should bind the arguments to the domain if they may emit any errors, take in count this use case:

vardomain=require('domain');varevents=require('events');vardom=domain.create();varemitter=newevents.EventEmitter();dom.on('error',function(error){console.log(error.message+' is caught');});dom.run(function(ee){ee.emit('someEvent');},emitter);// This error is not caught by the domainemitter.emit('error',newError('Oh No!'));

What do you think about explicitly binding the arguments to the domain?

[vkurchatkin]

What do you think about explicitly binding the arguments to the domain?

sounds counterintuitive and probably will cause lots of confusion

[caineio]

Hello!

I am pleased to see your valuable contribution to this project. Would you
please mind answering a couple of questions to help me classify this submission
and/or gather required information for the core team members?

Questions:

1. Which part of core do you think it might be related to?
   One of: debugger, http, assert, buffer, child_process, cluster, crypto, dgram, dns, domain, events, fs, http, https, module, net, os, path, querystring, readline, repl, smalloc, stream, timers, tls, url, util, vm, zlib, c++, docs, other (label)
2. Which versions of io.js do you think are affected by this?
   One of: v0.10, v0.12, v1.0.0 (label)
3. PR-only Does make test pass after applying this Pull Request.
   Expected: yes
4. PR-only Is the commit message properly formatted? (See
   CONTRIBUTING.md for more information)
   Expected: yes

Please provide the answers in an ordered list like this:

1. Answer for the first question
2. Answer for the second question
3. ...

Note that I am just a bot with a limited human-reply parsing abilities,
so please be very careful with numbers and don't skip the questions!

In case of success I will say:...summoning the core team devs!.

In case of validation problem I will say:Sorry, but something is not right here:.

Truly yours,
Caine.

Responsibilities

1. indutny: crypto, tls, https, child_process, c++
2. trevnorris: buffer, http, https, smalloc
3. bnoordhuis: http, cluster, child_process, dgram

[micnic]

1. domain
2. v0.12
3. yes
4. yes

[caineio]

...summoning the core team devs!

[bnoordhuis]

I think this conflicts with #66. I'll bring it up in the TC meeting today; if we are going to deprecate domains, it doesn't make sense to extend the API.

[bnoordhuis]

I did some quick research and calling Array.prototype.slice.call(arguments) always deopts the function. You can see it for yourself when you run with --trace_opt --trace_deopt; V8 always bails out with "Bad value context for arguments value". It's unavoidable and probably doesn't matter much in the grand scheme of things but it's still a pity.

[cjihrig]

I believe you can work around this by copying arguments to args using a loop. Not sure if it's worth it in this case.

Ref: https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#32-leaking-arguments

[Fishrock123]

See: matthewmueller/wrap-fn#4

[phpnode]

This is a bit of a fun one because slicing the arguments manually is not usually faster than calling Array::slice() - so whether this is worthwhile or not depends on how much work the rest of your function does. In this case that function isn't doing a lot of work itself but it means that V8 can't inline the function calls. This is a slightly obfuscated version that should be faster:

Domain.prototype.run=function(fn){if(this._disposed)return;if(arguments.length>=2){returnthis._runWithArgs(fn,Array.prototype.slice.call(arguments,1));}else{returnthis._runNaked(fn);}};Domain.prototype._runNaked=function(fn){this.enter();varret=fn.call(this);this.exit();returnret;};Domain.prototype._runWithArgs=function(fn,args){this.enter();varret=fn.apply(this,args);this.exit();returnret;};

[cjihrig]

This section of code from lib/events.js is probably the correct approach to take - https://github.com/iojs/io.js/blob/v0.12/lib/events.js#L110-L127

[micnic]

I think we should have a standard function to handle the arguments in the most optimal way, something like this:

util.getArgumentsAsArray=function(args,slice){varlength=args.length-slice;vararr=newArray(length);varindex=slice;while(index<length){arr[index-slice]=args[index];index++;}returnarr;};// or maybe ES6 Array.from() not sure about itArray.from(arguments).slice(1);

This function may be used in setTimeout(), setInterval(), setImmediate(), EventEmitter.emit(), domain.run(), in some future implementation with custom arguments and in third-party modules.

[phpnode]

@micnic how would you call this function without leaking the arguments from the caller?

[cjihrig]

My understanding is that arguments are just a pain to deal with and that passing them to a function, like your getArgumentsAsArray(), is enough to cause a deoptimization in most cases.

[phpnode]

There's no way to slice or otherwise manipulate arguments without doing it manually if you want to avoid deopts. The only function that it is ok to pass a naked arguments to is Function.prototype.apply as it is special cased. Bluebird uses a macro to make this more convenient, also see https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments for more info.

[micnic]

@phpnode, ah, you are right, there is no way no avoid deoptimization, thanks for clarifying, I hope V8 devs will improve this.

I'll just add the loop manipulation as it is in the EventEmitter.

[micnic]

This PR is ready for merge, I made the demanded changes

cc @bnoordhuis@trevnorris

[bnoordhuis]

@micnic@chrisdickinson will take it from here, he graciously volunteered to shepherd this PR.