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:

var domain = require('domain');
var events = require('events');

var dom = domain.create();
var emitter = new events.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 domain
emitter.emit('error', new Error('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) {
    return this._runWithArgs(fn, Array.prototype.slice.call(arguments, 1));    
  } else {
    return this._runNaked(fn);
  }
};

Domain.prototype._runNaked = function (fn) {
  this.enter();
  var ret = fn.call(this);
  this.exit();
  return ret;
};

Domain.prototype._runWithArgs = function (fn, args) {
  this.enter();
  var ret = fn.apply(this, args);
  this.exit();
  return ret;
};

[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) {
  var length = args.length - slice;
  var arr = new Array(length);
  var index = slice;

  while (index < length) {
    arr[index - slice] = args[index];
    index++;
  }

  return arr;
};

// or maybe ES6 Array.from() not sure about it

Array.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.