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

disabled optimisation issue: "bad value context for arguments value" #12

Closed
pflannery opened this issue Nov 29, 2013 · 9 comments
Closed

Comments

@pflannery
Copy link
Contributor

I've found something interesting that you may already be aware of.
The following functions all get immediately disabled for optimisation by the V8 compiler when arguments are passed to them.

TaskGroup.addTask
TaskGroup.createTask
TaskGroup.itemCompletionCallback
Task
Task.completionCallback
ambi

and others like
b (minification rocks lol)
setImmediate gets kicked as well

(On top of testing with node 0.10.22, v8 3.14.5.9, I've also tested the code with V8 version 3.23.6 using the d8 compiler and it still disables optimisation for this scenario.)

Pretty much everywhere that has args... causes V8 to immediately disable the functions from optimisation. It seems to be something to do with slice and\or passing the arguments variable to another method inside the calling method.

There is a way around this where by making a copy of the arguments manually which lets us get V8 optimization.

Workaround example:

args=[]
args.push arg for arg in arguments

or when their are static items on the stack too i.e. (item1, item2, args...) then

args=[]
args.push arg for arg in [2...arguments.length]

Love to hear your thoughts..

@balupton
Copy link
Member

Didn't know about it. Definitely is a problem. I wonder if we can report it to the coffeescript team and get them to fix it at the source.

@pflannery
Copy link
Contributor Author

I've reported it to joyent.

setImmediate seems to be a lot slower than processTick and it's also quite a bit slower than setTimeout.
Though it appears that v8 also disables optimisation for all node's timeout methods, like setTimeout and setInterval for the same reasons as setImmediate.

I wondered why chrome gets away with it and turns out they run all their timers through c++ instead of js and just exposes the timer methods. Maybe that's something node should do in the long run too.

@pflannery
Copy link
Contributor Author

Node issue

@balupton I think your right maybe something we can convince the coffee team @jashkenas@michaelficarra to change the output code for args...?

I was reluctant to ask at first as it seemed like this would be a V8 only scenario but I've done two benchmarks showing that small array copying is faster (plus we would get the benefit of V8 optimisation)

Arguments Copy Benchmark
Array Copy Benchmark

@michaelficarra
Copy link

Does calling [].slice.call directly (instead of using __slice helper) still cause it to disable optimisations?

@pflannery
Copy link
Contributor Author

thanks for your reply @michaelficarra, unfortunately it does the same..

this is my test app.js

function TestOptimization(a,b,c,d,e) {
    var target = [].slice.call(arguments);
};

// because V8 trace uses a sampler that 
// periodically samples the state of the VM
// I'm calling this method 150 times 
// on my pc to capture the trace message
// Put this higher if you don't see the trace info
for(var i=0;i<150; i++)
    TestOptimization(1,2,3,4,5);

then I'm running this to capture the optimisation trace messages

node --trace_opt --trace_deopt app.js

outputs

[deoptimize context: be6414b1]
[marking TestOptimization 0xf8f853b8 for recompilation, reason: small function, ICs with typeinfo: 2/2 (100%)]
[disabled optimization for TestOptimization, reason: bad value context for arguments value]

@michaelficarra
Copy link

Okay, please open an issue at jashkenas/coffee-script.

@balupton
Copy link
Member

Gonna close this, as this is something that coffeescript or v8 will need to fix.

@balupton
Copy link
Member

Re-opening, as this is unacceptable, even if it is not our responsibility.

@balupton
Copy link
Member

balupton commented Jun 4, 2016

@balupton balupton closed this as completed Jun 4, 2016
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

No branches or pull requests

3 participants