Improve performance of event and timer interfaces

[RubenVerborgh]

This pull request speeds up multi-listener event callbacks, setImmediate, setTimeout, and setInterval by differentiating between zero-, one-, and two-argument callbacks. This technique was previously only applied to single-listener event callbacks—and even they have been sped up.

Current situation

The EventEmitter#emit code has special cases for zero-, one-, and two-argument callbacks as follows:

len = arguments.length;
switch (len) {
  // fast cases
  case 1: handler.call(this); break;
  case 2: handler.call(this, arguments[1]); break;
  case 3: handler.call(this, arguments[1], arguments[2]); break;
  // slower
  default:
    args = new Array(len - 1);
    for (i = 1; i < len; i++)
      args[i - 1] = arguments[i];
    handler.apply(this, args);
}

Improvements in this pull request

On the one hand, this pull request speeds up this technique by avoiding the use of arguments indexing, instead adding actual function arguments arg1 and arg2.

On the other hand, this pull request also applies the same technique to listeners with multiple callbacks, as well as the timer functions setImmediate, setTimeout, and setInterval.

Below are my results of performance tests that examine these cases (ran on a 2010 MacBook Pro).

	a7f5309	← diff →	17701f939db247233e574bb1ff07191f8c00c877
event with one listener	631 ms	-24.25%	478 ms
event with two listeners	2,899 ms	-44.98%	1,595 ms
setImmediate	6,758 ms	-40.17%	4,043 ms
setTimeout	8,828 ms	-65.17%	3,075 ms
setInterval	8,830 ms	-71.77%	2,493 ms

Considerations

Given the importance of callbacks in io.js, delivering maximum performance for common cases is crucial. The only drawback of this pull request seems to be the increased code length; but it brings consistency since the optimization technique was previously only applied to one particular case (and slightly suboptimally).

I added each change in a different commit, so you can decide which ones to merge. All commits are independent of each other, except for the second, which depends on the first.

If you want these commits squashed, or applied to a different branch, please let me know and I'll do so.

PS This pull request is the equivalent of nodejs/node-v0.x-archive#9007.

[mscdex]

+10

[jbergstroem]

@RubenVerborgh: It's somewhat interesting that setImmediate seems to be slower than in nodejs. Did you stumble on anything while researching this that would explain why?

[RubenVerborgh]

@jbergstroem I've only looked at the JavaScript level, which has remained unchanged. It must have something to do with one of the underlying layers. Note that setImmediate was quite slow in Node 0.10.x, so something has happened to make it faster in the current master.

[RubenVerborgh]

Thinking about this some more, it seems that io.js did not implement the patch that made one-listener events in Node slower after v0.10.x. I wonder if that same patch is responsible for the other differences.

[RubenVerborgh]

Rebased on v1.x to resolve a merge conflict.

[Fishrock123]

This is a typical de-opt; I wonder how much difference it makes already not having this.

[RubenVerborgh]

There will already be some difference for sure. But why stop there, if the other cases show an improvement by using the switch as well?

[Fishrock123]

SGTM

Little odd to me that setImmediate is slower after this, perhaps someone else can chip in.

[XadillaX]

so you are somewhat like this way: https://github.com/XadillaX/Toshihiko/blob/develop/util/damnarguments.js and https://github.com/XadillaX/Toshihiko/blob/develop/util/damnargumentsgenerator.js.

maybe you can try more arguments.

[RubenVerborgh]

@Fishrock123 Slower?

@XadillaX 2 seems like a good number to stop. Events with more arguments are rare. Plus there is a slight overhead in allocating the variables, as they are used in a closure. (Also, I'd suggest to convert the long chain of if statements in the code you linked into switch.)

[Fishrock123]

I mean slower than setTimeout.

[RubenVerborgh]

@Fishrock123 No meaningful comparison is possible between different tests; they only allow comparing the performance of different io.js versions for the same test. The number of repetitions for each test was chosen such that their timings were roughly in the 1–10 second range. setTimeout was invoked 2,000,000 times with a 0ms delay; setImmediate 3,000,000 times. Extrapolating this quickly, setImmediate would still be faster for the same number of repetitions.

[brendanashworth]

switch to Array.isArray()?

[RubenVerborgh]

Fine with me; is there a guideline for such decisions in general? I followed the existing convention in the file.

[RubenVerborgh]

I just noted during rebasing that the conflicts are due to code changes moving away from util operations. So I'll do this in general.

[brendanashworth]

I like what this PR is getting at and I'd certainly like to see it merged in. Looks like it needs a rebase though.

[RubenVerborgh]

Here you go, rebased on v1.x.

[RubenVerborgh]

…and rebased again.
@brendanashworth Would it be possible to merge this anytime soon? Rebasing is getting a bit tough 😉

@mscdex might want to check whether the optimizations from b677b84 are relevant here as well. One of my original commits was not necessary anymore after b677b84.

[brendanashworth]

could you put the unassigned variables (args, i) in front of the assigned ones?

[RubenVerborgh]

Done.

[brendanashworth]

Right now the code looks good to me but I have yet to run tests (with the libuv issue for 1.4 I can't properly test) so I'm not gonna officially sign off on this until tomorrow when they have that fixed.

By the way, I think we'd be happy to have your benchmarks added into the repository. I'd shepherd in a pull request for those if you'd be willing to patch them into how we do them here.

@Fishrock123 was your SGTM a sign-off or just a +1?

[brendanashworth]

I got the tests to pass. However I'm not really seeing any improvement on the ee-emit-multi-args benchmark (am seeing improvement on ee-emit though); could you run that benchmark and see if you get any improvement? I'm not able to reproduce your 25%-45% performance boost.

[RubenVerborgh]

Confirmed that the event emitter improvements no longer make a difference, due to b677b84. Will remove the commit in question if needed.

[brendanashworth]

Yeah, if the commit no longer makes a difference I'd remove it. I'll run your benches for the timers to test performance before I give my official sign off.

Since this pull request deals with timers, a locked API, I can't be the sole signee to be sure this wouldn't have consequences I don't see. Summoning @iojs/collaborators

[trevnorris]

I prefer var len = arguments.length; on a different line. single assignation per line, and all unused variable on another. :)

[RubenVerborgh]

Done.

[trevnorris]

Left a couple comments, and the commits squashed, and their messages cleaned up, but LGTM otherwise.

[RubenVerborgh]

@trevnorris Please detail what you mean with “messages cleaned up”. Just one commit then with “Improve timer performance?”

[trevnorris]

@RubenVerborgh Squashed, the message should read timer: <message here> and the message body is delimited at 72 chars. Explain what was done, and possibly include some of the benchmark results as well. Makes it easier when we revisit old commits.

[RubenVerborgh]

Squashed and commit message edited.

[tellnes]

len = arguments.length on it's own line?

[RubenVerborgh]

Done.

[tellnes]

One comment, otherwise LGTM.