-
Notifications
You must be signed in to change notification settings - Fork 7.3k
V8 disables optimization for setImmediate #6631
Comments
Just to add, setTimeout and setInterval get disabled too. Here is jsperf for small "copy array" benchmarks - http://jsperf.com/copy-array-test , it's interesting that small array copying is faster than slicing. Seeing as js stack arguments aren't really an array I tested the benchmarks in node and the results yield the same. |
Calling Array.prototype.slice(arguments) is a standard practice in javascript. If v8 doesn't recognize that, it's a reason to open an issue against v8. (a few dumb things I said are removed from this message) |
The weird thing is that code branch only executed when argument.length > 1. And usually it is false, so I'd say it doesn't need to be optimized, unless... Guys, I'm losing my mind. Can you please explain me what is going on with Chromium 30.0.1599 here: |
You was measuring speed of function allocation, not the access to arguments or whatever, updated your jsperf: http://jsperf.com/qwxgwxgwxq/2 |
@pflannery I agree with @rlidwka I'd rather try opening the issue in a v8 bug tracker. May be @trevnorris has other opinion on it? |
@indutny , oh thanks, I'm not used to such low level benchmarks. Which leaves me wondering why function is allocated more slowly, but I can live with that. Similar issue here: jashkenas/coffeescript#3274 (and a proof that this is a common practice indeed) |
@rlidwka @indutny Those new perf's don't fall in to this issue as they wouldn't recreate the "bad value context for arguments value" and a work around to test against it. Here is an example of recreating the "bad value context for arguments value" scenario The main point here is that performance critical code is being disabled by V8. The perfs show there is a way around this and also show a code performance improvement too. I agree that an issue should be raised with the V8 team so I have created an issue with V8 here Though I agree that |
I'll generate the IR using both techniques and do a comparison. Tight loop micro benchmarks are pretty useless in these cases. The As far as using // hydrogen.cc
void ValueContext::ReturnValue(HValue* value) {
// The value is tracked in the bailout environment, and communicated
// through the environment as the result of the expression.
if (!arguments_allowed() && value->CheckFlag(HValue::kIsArguments)) {
owner()->Bailout(kBadValueContextForArgumentsValue);
}
owner()->Push(value);
}
// objects.h
#define ERROR_MESSAGES_LIST(V) \
V(kBadValueContextForArgumentsValue, \
"bad value context for arguments value") \ I'm not sure why, but they usually have a reason behind their madness. :) @pflannery Feel free to open a PR with the optimization. |
@trevnorris thanks for your input, that's exactly where the bail out occurs.. I will setup a pull request for js over the next few days. cheers |
@pflannery thanks. when you do, @ mention me and I'll give it a review. |
Prevent v8 disabling optimization for scenario "bad value context for arguments value". Solves #6631 Signed-off-by: Trevor Norris <[email protected]>
This issue appears to be resolved and can be closed. |
The message in the v8 deopt trace is
"bad value context for arguments value"
This happens because v8 doesn't like the
arguments
variable being passed to other functions and disables optimization for any calling method doing so.In
setImmediate
's case its passing thearguments
variable toArray.prototype.slice
method and therefore being disabled. source code is hereI've tested a workaround which is to copy the arguments manually instead, example:
This enables optimization and also gives a 25% performance gain when calling setImmediate.
I can create the pull request if no objection?
The text was updated successfully, but these errors were encountered: