Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Array.prototype.slice for arguments for optimization #263

Closed
wants to merge 1 commit into from

Conversation

arusakov
Copy link

Hi, thanks for great websocket library!

There are some good topics about optimizations, for example: Optimization-killers. Functions with Array.prototype.slice.call(arguments) cannot be optimized. So I replaced them like this commit in Node.js from one of V8 developer.

@arusakov
Copy link
Author

@brycekahle I do not understand what is wrong with the tests :(

@brycekahle
Copy link
Contributor

The tests were failing before, so likely not because of your code changes.

var args = Array.prototype.slice.call(arguments, 1);
// We need to copy arguments to local array for using it in another function call.
// The only way js engine (like V8) will be able to optimize it.
var args = [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing args = [] it would be better to just allocate an array with the size of the arguments, and adding adding the arguments to the correct index.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Array(arguments.length) is the best solution for browsers based on latest version of V8 (Chrome, Opera, etc), but for some browsers (old IE8, IE9, old V8 based browsers, maybe Firefox - I'm not sure now) args = [] and args.push() is faster. So I think, args = [] - is good, is middle way - newest browsers are fast, older browsers - work as quickly as possible

@arusakov
Copy link
Author

@3rd-Eden I write some test, put it in project root folder and run with node (I use v4.1.1):

var Emitter = require('./lib/event/emitter').EventEmitter;

function test() {
  var emitter = new Emitter();
  var COUNT = 1e5;

  var forTest = 0;

  // add two listeners
  emitter.on('message', function (data) {
    forTest += data.val;
  });
  emitter.on('message', function (data) {
    forTest -= data.val;
  });

  // random value
  var rndSeed = Date.now() % 10;

  var t1 = Date.now();
  for (var i = 0; i < COUNT; ++i, ++rndSeed) {
    emitter.emit('message', {
      val: rndSeed
    });
  }
  t1 = Date.now() - t1;
  console.log(t1, forTest);
}

test();

I got the following results (less time - better):

Version time (ms)
slice 130
[] and push 25

@richotaylor
Copy link

Just ran into this as well. Would love to see it be merged.

@lpinca
Copy link
Contributor

lpinca commented Oct 30, 2015

The solution proposed by @3rd-Eden is indeed faster even in IE8. You can test it here http://jsperf.com/arguments-push-vs-nopushcall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants