Do not bailout from optimizing functions that use f(x, arguments)

Do not bailout from optimizing functions that use f(x, arguments)
if there is not enough type-feedback to detect that f is Function.prototype.apply.

BUG=v8:3709
LOG=N
TEST=mjsunit/regress/regress-3709

Committed: https://chromium.googlesource.com/v8/v8/+/64a9cd63a7d395d082fea00d43503a43a671ad9e

[ulan, 2014-11-19 16:32:37]

ulan@chromium.org changed reviewers:
+ jkummerow@chromium.org

[ulan, 2014-11-19 16:32:37]

PTAL

[Jakob Kummerow, 2014-11-19 20:46:16]

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc#newcode9282
src/hydrogen.cc:9282: if (CanBeFunctionApplyArguments(expr, false)) {
I'm not too happy with this condition. It lacks a check for not having type
feedback. As it is, *any* call of the form "some_function(something, arguments)"
will emit the deopt, as long as "some_function" isn't an HConstant. That's safe
in terms of functionality, but could hurt performance when it's part of a big,
expensive-to-optimize function where we'd try 10 times before giving up, rather
than disabling optimization immediately.

I don't see a good alternative right now ("expr->IsUninitialized()" doesn't
appear to be useful here); maybe we can find something tomorrow.

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc#newcode9284
src/hydrogen.cc:9284: Deoptimizer::EAGER);
If we find an appropriate check for "no type feedback yet", this should be
Deoptimizer::SOFT; otherwise it deserves a comment why it's EAGER.

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.h#newcode2265
src/hydrogen.h:2265: HValue* Lookup(Variable* var) {
Why do you need this variant? The "!value->IsArgumentsObject()" check in
IsEligibleForEnvironmentLivenessAnalysis() should do the trick, no?

https://codereview.chromium.org/736043002/diff/1/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-3709.js (right):

https://codereview.chromium.org/736043002/diff/1/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-3709.js:25: if (%GetOptimizationStatus(foo) != 4) {
Just use assertOptimized(foo) / assertUnoptimized(foo), it'll do all the
required magic.

[Jakob Kummerow, 2014-11-20 09:59:15]

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc#newcode9282
src/hydrogen.cc:9282: if (CanBeFunctionApplyArguments(expr, false)) {
> I don't see a good alternative right now ("expr->IsUninitialized()" doesn't
> appear to be useful here); maybe we can find something tomorrow.

I've talked to mvstanton@, he suggested to implement something like:

bool TypeFeedbackOracle::CallIsUninitialized(FeedbackVectorICSlot slot) {
  return GetInfo(slot)->IsUndefined();
}

which can be queried with the slot returned by Call's CallFeedbackSlot().

[ulan, 2014-11-20 15:23:30]

Thanks for the comments! I uploaded new patch set.

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc#newcode9282
src/hydrogen.cc:9282: if (CanBeFunctionApplyArguments(expr, false)) {
On 2014/11/20 09:59:15, Jakob wrote:
> > I don't see a good alternative right now ("expr->IsUninitialized()" doesn't
> > appear to be useful here); maybe we can find something tomorrow.
> 
> I've talked to mvstanton@, he suggested to implement something like:
> 
> bool TypeFeedbackOracle::CallIsUninitialized(FeedbackVectorICSlot slot) {
>   return GetInfo(slot)->IsUndefined();
> }
> 
> which can be queried with the slot returned by Call's CallFeedbackSlot().

Done.

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc#newcode9282
src/hydrogen.cc:9282: if (CanBeFunctionApplyArguments(expr, false)) {
On 2014/11/19 20:46:16, Jakob wrote:
> I'm not too happy with this condition. It lacks a check for not having type
> feedback. As it is, *any* call of the form "some_function(something,
arguments)"
> will emit the deopt, as long as "some_function" isn't an HConstant. That's
safe
> in terms of functionality, but could hurt performance when it's part of a big,
> expensive-to-optimize function where we'd try 10 times before giving up,
rather
> than disabling optimization immediately.
> 
> I don't see a good alternative right now ("expr->IsUninitialized()" doesn't
> appear to be useful here); maybe we can find something tomorrow.

Acknowledged.

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.cc#newcode9284
src/hydrogen.cc:9284: Deoptimizer::EAGER);
On 2014/11/19 20:46:16, Jakob wrote:
> If we find an appropriate check for "no type feedback yet", this should be
> Deoptimizer::SOFT; otherwise it deserves a comment why it's EAGER.

Done.

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/736043002/diff/1/src/hydrogen.h#newcode2265
src/hydrogen.h:2265: HValue* Lookup(Variable* var) {
On 2014/11/19 20:46:16, Jakob wrote:
> Why do you need this variant? The "!value->IsArgumentsObject()" check in
> IsEligibleForEnvironmentLivenessAnalysis() should do the trick, no?

Indeed, node.

https://codereview.chromium.org/736043002/diff/1/test/mjsunit/regress/regress...
File test/mjsunit/regress/regress-3709.js (right):

https://codereview.chromium.org/736043002/diff/1/test/mjsunit/regress/regress...
test/mjsunit/regress/regress-3709.js:25: if (%GetOptimizationStatus(foo) != 4) {
On 2014/11/19 20:46:16, Jakob wrote:
> Just use assertOptimized(foo) / assertUnoptimized(foo), it'll do all the
> required magic.

Done.

[Jakob Kummerow, 2014-11-20 15:57:10]

LGTM.

I'm a bit sad that we have to increase Call nodes' sizes, but since calls
probably aren't all that frequent, that should be fine.

[ulan, 2014-11-20 16:04:15]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2014-11-20 16:05:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/736043002/40001

[commit-bot: I haz the power, 2014-11-20 17:07:49]

Committed patchset #3 (id:40001)

[Sven Panne, 2014-11-21 07:34:26]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2014-11-21 07:34:27]

Coming a bit late to the party, but anyway... :-)

https://codereview.chromium.org/736043002/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/736043002/diff/40001/src/ast.h#newcode1930
src/ast.h:1930: class IsUninitializedField : public BitField8<bool, 0, 1> {};
This is not so good for 2 reasons: Even on 32bit, we have 2 bytes internal
padding "for free" at the beginning of this class, so small new stuff should go
there. Furthermore, using BitField complicates things, a plain old bool would be
more efficient and we have the room anyway.