Potential arguments leakage?

[dashed]

Saw this in another issue: pixijs/pixijs#853

Other sources on this:

• https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments
• http://stackoverflow.com/questions/23509312/does-node-js-really-not-optimize-calls-to-slice-callarguments

---

May be relevant in reflux where arguments is potentially leaked. AFAIK, this is something for V8.

Points of interest:

1. https://github.com/spoike/refluxjs/blob/ff67e1d996f44cf87e38d45a855f3b4291572d01/src/createStore.js
   Leak at line 44, which is caught in line 34. We shouldn't trust what happens in EventEmitter.emit.
2. https://github.com/spoike/refluxjs/blob/ff67e1d996f44cf87e38d45a855f3b4291572d01/src/all.js#L58
   line 58: classic pattern. Use patch mentioned here: Make this an optimizable function tj/node-thunkify#12

/cc @spoike @bripkens

[dashed]

Another point of interest is: https://github.com/spoike/refluxjs/blob/ff67e1d996f44cf87e38d45a855f3b4291572d01/src/createAction.js#L17

[spoike]

Thanks for the heads up @dashed

Only skimmed through the issues and it is true that the emitters are "hot" code, depending on how often they are really emitted. So it might be a problem in some instances.

We should probably add some benchmark test as a grunt task, a lot like what the node-thunkify patch talks about.

[dashed]

I never liked how arguments is explicitly passed as a single argument in EventEmitter.emit.

I think we should do something like the following in /src/createAction.js with two proposed changes to the action functor. It'll be interesting to see benchmarks for either implementations. I have a 'feeling' that the one using Function.prototype.bind() may be faster.

// /src/createAction.js

// ...

    // uses Function.prototype.bind() that will need to be shimmed for IE8
    functor = function() {
        var args = arguments;
        _.nextTick(function() {
            functor.preEmit.apply(functor, args);
            if (functor.shouldEmit.apply(functor, args)) {
                action.emit.bind(action, eventLabel).apply(action, args);
            }
        });
    };

    // explicit use of Array.prototype.slice()
    functor = function() {
        var args = arguments;
        _.nextTick(function() {
            functor.preEmit.apply(functor, args);
            if (functor.shouldEmit.apply(functor, args)) {
                var _args = Array.prototype.slice.call(args, 0);
                _args.unshift(eventLabel);
                action.emit.apply(action, _args);
            }
        });
    };

    functor.listen = function(callback, bindContext) {
        var eventHandler = function() {
            callback.apply(bindContext, arguments);
        };
        action.addListener(eventLabel, eventHandler);

        return function() {
            action.removeListener(eventLabel, eventHandler);
        };
    };

// ...

[dashed]

Another reference with regards of converting arguments to an array: http://blog.izs.me/post/7746314700/benchmark-array-ification-of-arguments

I'm not intimately familiar with V8, but Crankshaft is mentioned; and a new compiler called Turbofan is mentioned in the wild: https://news.ycombinator.com/item?id=8117091

Here's hoping this type of use case becomes a non-issue in Turbofan =P. But alas, we should take care of it now.

[bripkens]

Nice catch @dashed. The sad thing about this is that we cannot put this logic (arguments to js array) into util.js as this will effectively leak the arguments object and therefore the function will be unoptimizable.

Is there some kind of Grunt task (or browserify transformer) that we can use to turn the [].slice.call(arguments) pattern into the optimizable version?

[spoike]

The npm package isn't using the browserified build but is instead using the source directly. As far as I've read about it seems to be an issue in V8 so it needs to work both on the browser (Chrome) and server (Node).

I don't know if we should point towards dist/reflux.js instead, but if we did then we could try to use the browserify transform approach to apply this fix for the both browser and server-side.

[bripkens]

I don't think it is a good idea to point to dist/reflux.js. I would prefer to keep the nice stack traces (without source maps) in node. Using a transformer might require us to have an optimized and unoptimized version in the repository. That's probably not a good idea though considering the few lines of code that we would be saving.

[dashed]

Made a benchmark:

"use strict";
/**
 * Deps:
 * npm install benchmark EventEmitter3
 */
var EventEmitter = require('EventEmitter3').EventEmitter;
var bus = new EventEmitter();

function nextTick(callback) {
  // setTimeout(callback, 0);
  callback();
};

function getOptmizationStatus(fn) {
    switch(%GetOptimizationStatus(fn)) {
        case 1: return("Function is optimized"); break;
        case 2: return("Function is not optimized"); break;
        case 3: return("Function is always optimized"); break;
        case 4: return("Function is never optimized"); break;
        case 6: return("Function is maybe deoptimized"); break;
    }
};


/**
 *
 * Test functions
 */
var slice = [].slice;

var preEmit = function() {};
var shouldEmit = function() {return true};

function original() {

  var args = arguments;

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      bus.emit('action', args);
    }
  });

};

function originalBind() {

  var args = arguments;

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      bus.emit.bind(bus, 'action').apply(bus, args);
    }
  });
};

var emit = bus.emit.bind(bus, 'action');

function originalApply() {

  var args = arguments;

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      emit.apply(emit, args);
    }
  });

};

function sliceArr() {
  var args = [].slice.call(arguments);

  args.unshift('action');

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      bus.emit.apply(bus, args);
    }
  });
};

function arrayApply() {
  var args = arguments.length === 1 ? [arguments[0]] : Array.apply(null, arguments);

  args.unshift('action');

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      bus.emit.apply(bus, args);
    }
  });
};


function manualMap() {
  var l = arguments.length
  var args = new Array(l)
  for (var i = 0; i < l; i ++) args[i] = arguments[i]

  args.unshift('action');

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      bus.emit.apply(bus, args);
    }
  });
};

/**
 * Set up test cases
 */
var tests = [];
tests.push(original);
tests.push(originalBind);
tests.push(originalApply);
tests.push(sliceArr);
tests.push(arrayApply);
tests.push(manualMap);



//
// Optimize check
//

tests.forEach(function(fn) {
  fn();
  fn();
  fn();
  %OptimizeFunctionOnNextCall(fn);
  fn();

  console.log("STATUS OF " + fn.name + ": " + getOptmizationStatus(fn));
});

console.log();

// benchmark
var Benchmark = require('benchmark');
var suite = new Benchmark.Suite;

tests.forEach(function(fn) {

  suite.add(fn.name, function() {
    fn(Math.random());
    fn(Math.random(), fn(Math.random()));
  });

});

suite
.on('cycle', function(event) {
  console.log(String(event.target));
})
.on('complete', function() {
    console.log('\nFastest is ' + this.filter('fastest').pluck('name'));
})
.run();

/*
$ node --allow-natives-syntax bench.js
STATUS OF original: Function is not optimized
STATUS OF originalBind: Function is not optimized
STATUS OF originalApply: Function is not optimized
STATUS OF sliceArr: Function is not optimized
STATUS OF arrayApply: Function is optimized
STATUS OF manualMap: Function is optimized

original x 587,560 ops/sec ±3.91% (86 runs sampled)
originalBind x 21,084 ops/sec ±7.65% (68 runs sampled)
originalApply x 266,257 ops/sec ±2.43% (95 runs sampled)
sliceArr x 123,433 ops/sec ±2.48% (93 runs sampled)
arrayApply x 407,646 ops/sec ±2.08% (93 runs sampled)
manualMap x 422,805 ops/sec ±2.34% (92 runs sampled)

Fastest is original
 */

[bripkens]

Thanks @dashed, nice stuff.

My data for comparison:

$ node --allow-natives-syntax bench.js
STATUS OF original: Function is not optimized
STATUS OF originalBind: Function is not optimized
STATUS OF originalApply: Function is not optimized
STATUS OF sliceArr: Function is not optimized
STATUS OF arrayApply: Function is optimized
STATUS OF manualMap: Function is optimized

original x 908,390 ops/sec ±1.07% (93 runs sampled)
originalBind x 50,450 ops/sec ±2.06% (91 runs sampled)
originalApply x 346,908 ops/sec ±2.84% (91 runs sampled)
sliceArr x 207,527 ops/sec ±2.53% (95 runs sampled)
arrayApply x 651,630 ops/sec ±0.92% (99 runs sampled)
manualMap x 704,258 ops/sec ±0.58% (100 runs sampled)

Fastest is original

[bripkens]

Even though the original is the fastet, it might not be the best solution. In the current situation we are passing an arguments object to the event bus. Users of reflux therefore need to know that they are not getting an Array and thus cannot use forEach and other function that only exist on Array.

arrayApply looks concise on first sight, but manualMap seems makes the intention a bit more obvious. Based on the information that @dashed collected I added another test case that combines some of the concepts.

function toArray() {
  var l = arguments.length
  var args = new Array(l)
  for (var i = 0; i < l; i ++) args[i] = arguments[i]
  return args;
}

function utilToArray() {
  var args = toArray.apply(null, arguments);
  args.unshift('action');

  nextTick(function(){
    preEmit.apply(null, args);
    if (shouldEmit.apply(null, args)) {
      bus.emit.apply(bus, args);
    }
  });
};

Results:

$ node --allow-natives-syntax bench.js
STATUS OF original: Function is not optimized
STATUS OF originalBind: Function is not optimized
STATUS OF originalApply: Function is not optimized
STATUS OF sliceArr: Function is not optimized
STATUS OF arrayApply: Function is optimized
STATUS OF manualMap: Function is optimized
STATUS OF utilToArray: Function is optimized

original x 896,894 ops/sec ±0.61% (95 runs sampled)
originalBind x 47,898 ops/sec ±2.54% (87 runs sampled)
originalApply x 371,403 ops/sec ±2.12% (95 runs sampled)
sliceArr x 198,411 ops/sec ±1.17% (95 runs sampled)
arrayApply x 623,442 ops/sec ±1.39% (98 runs sampled)
manualMap x 678,715 ops/sec ±0.96% (100 runs sampled)
utilToArray x 656,391 ops/sec ±0.78% (97 runs sampled)

Fastest is original

utilToArray seems like a good trade-off. It is not as fast as manualMap, but we don't need to duplicate the array-copying-logic. What do you think?

[bripkens]

Just created a JSPerf test as we should also take Firefox and Internet Explorer into account:

http://jsperf.com/arguments-to-javascript-array

[dashed]

Woah I "optimized" everything! 😆

https://gist.github.com/Dashed/77ee677c46879d2919eb#file-oldbench-js

[dashed]

My previous benchmark is meant to show that nextTick is irrelevant, so I flattened the lexical scope, and did some more optimizations: https://gist.github.com/Dashed/77ee677c46879d2919eb

manualMap2, utilToArray2, and utilToArray4 now consistently beats original.

@bripkens I recommend utilToArray4 for implementation.

Node 0.10.28:

STATUS OF original: Function is not optimized
STATUS OF originalBind: Function is not optimized
STATUS OF originalApply: Function is optimized
STATUS OF sliceArr: Function is not optimized
STATUS OF arrayApply: Function is optimized
STATUS OF manualMap: Function is optimized
STATUS OF manualMap2: Function is optimized
STATUS OF utilToArray: Function is optimized
STATUS OF utilToArray2: Function is optimized
STATUS OF utilToArray3: Function is optimized
STATUS OF utilToArray4: Function is optimized

original x 930,629 ops/sec ±4.61% (90 runs sampled)
originalBind x 42,216 ops/sec ±3.26% (88 runs sampled)
originalApply x 602,979 ops/sec ±3.59% (90 runs sampled)
sliceArr x 214,171 ops/sec ±0.97% (96 runs sampled)
arrayApply x 194,816 ops/sec ±2.49% (89 runs sampled)
manualMap x 132,307 ops/sec ±3.35% (88 runs sampled)
manualMap2 x 1,179,469 ops/sec ±2.05% (93 runs sampled)
utilToArray x 133,083 ops/sec ±3.77% (87 runs sampled)
utilToArray2 x 1,076,526 ops/sec ±2.10% (93 runs sampled)
utilToArray3 x 183,454 ops/sec ±21.06% (74 runs sampled)
utilToArray4 x 1,051,558 ops/sec ±0.88% (93 runs sampled)

Fastest is manualMap2

[dashed]

For reference, my implementation of toArray4.

It creates an array copy of arguments but with a number of empty element spaces to the right of the array. This number of empty element spaces is provided via the this context, which is by default 0 if it's not a Number. The callee is supposed to fill the empty spaces of the returned array.

function toArray4() {

  var n = !(this instanceof Number) ? 0 : +(this);

  var m = arguments.length + n;
  var args = new Array(m);
  while (m-- > n)  {
    args[m] = arguments[m - n];
  }

  return args;
}

function utilToArray4() {

  preEmit.apply(null, arguments);
  if (shouldEmit.apply(null, arguments)) {
    var args = toArray4.apply(1, arguments);
    args[0] = 'action';
    bus.emit.apply(bus, args);
  }

};

[spoike]

I'm busy this weekend so I'm unable test this out and comment in a coherent matter but --> 👍 nice work y'guys, I appreciate it.

[spoike]

@bripkens @dashed Feel free to create a pull request for this issue.

[dashed]

I don't think this is really a problem anymore after looking at master.