[go: nahoru, domu]

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

V8 disables optimization for dispatch #67

Open
Hypercubed opened this issue Jun 4, 2015 · 11 comments
Open

V8 disables optimization for dispatch #67

Hypercubed opened this issue Jun 4, 2015 · 11 comments

Comments

@Hypercubed
Copy link

As described in various places online (for example nodejs/node-v0.x-archive#6631 and bevry/taskgroup#12) v8 disables optimization when the "arguments variable being passed to other functions and disables optimization for any calling method doing so".

I verified using the following test code:

var signals=require('./dist/signals');

var myObject = new MyObject();
myObject.started.add(testDispatch, this);

for(var i=0;i<20000; i++) {
    myObject.started.dispatch(1,2,3,4,5);
}

function MyObject() {
  this.started = new signals.Signal();
}

function testDispatch(a,b,c,d,e) {
    this || a || b;
};

The output includes:

[failed to optimize Signal.dispatch: Bad value context for arguments value]

If I replace #L366 with:

var paramsArr = [];
for (var index=0; index < arguments.length; index++) {
  paramsArr.push(arguments[index]);
}

I would submit a PR but:

  • building and testing is not working for me.
  • I haven't verified that this actually impacts performance.
  • not sure if there are other places deoptimization will occur.
@Hypercubed
Copy link
Author

I forgot to mention you need to run the test using the following command:

node --trace_opt --trace_deopt test.js

@Hypercubed
Copy link
Author

@sompylasar
Copy link
sompylasar commented Jun 5, 2015 via email

@Hypercubed
Copy link
Author

Yep, I've seen this. I don't entirely trust the jsperf myself.... what I do notice though is messages in the browser that signal.dispatch is being skipped by the v8 optimizer. Some of the other github issue I listed above seam to think it can be significant.

@sompylasar
Copy link
sompylasar commented Jun 5, 2015 via email

@mhofman
Copy link
mhofman commented Jun 10, 2015

Looks like Array.prototype.push.apply is not subject deoptimization and is decently fast so the following should work too:

var paramsArr = [];
Array.prototype.push.apply(paramsArr, arguments);

See http://jsperf.com/copy-fn-arguments-test/16

@Hypercubed
Copy link
Author

Still slower than the loop I think... but surprisingly does appear to allow v8 optimization.

Hypercubed added a commit to Hypercubed/js-signals that referenced this issue Jun 10, 2015
@mhofman
Copy link
mhofman commented Jun 10, 2015

Not that surprising. When you think about it, second argument of Function.apply is guaranteed to not be modified, so no reason to deoptimize.
It seems to be about half the speed of pre-allocated copy in Chrome, but strangely in Node it's much closer to the un-allocated copy, which makes much more sense.

Oh Arrays and their dark magic performances...

Copy to pre-allocated x 9,517,368 ops/sec ±0.31% (99 runs sampled)
Copy to un-allocated x 7,222,102 ops/sec ±0.31% (99 runs sampled)
Copy using Slice x 1,219,995 ops/sec ±0.53% (98 runs sampled)
Copy using Slice Indirect x 1,149,504 ops/sec ±0.48% (91 runs sampled)
Copy using Push apply x 7,130,480 ops/sec ±0.36% (96 runs sampled)
Manual Copy x 10,188,893 ops/sec ±0.49% (95 runs sampled)

@Hypercubed
Copy link
Author

Maybe can do even better. Apparently the one time you should can use arguments in a functional call without causing de-optimization is Function#apply (source https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#what-is-safe-arguments-usage). I saw huge boost here: https://github.com/Hypercubed/mini-signals

@mhofman
Copy link
mhofman commented Sep 15, 2015

@Hypercubed, from what I understand your library doesn't allow for a binding to have its own prepended/saved params.

Regardless, the Function#apply approach is the same that I suggested in my previous comment, just that you use it to invoke the binding's function directly, where here it has to be used to make a copy of the params using Array#push. While in Chrome it is faster than slice.call(), it still isn't as fast as a manual copy as you originally suggested, so in that sense it isn't better. However it sure is more readable IMHO.

@Hypercubed
Copy link
Author

You're right. In both cases v8 optimization works because of Function#apply. I can use the binding function directly because I am not adding any saved params. This is something that cannot be done in EventEmiiters because they need to remove the event name.

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

No branches or pull requests

4 participants