Optimize Function.prototype.call

Optimize Function.prototype.call

- May inline the function, or call it directly, instead of going through call
- Supports arguments object escaping when it escapes to builtins (preparation for slice.call(arguments, ...) optimization)
- Both .call and .apply now support inlining when calling builtins indirectly

BUG=
R=verwaest@chromium.org, rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21840

[Toon Verwaest, 2014-06-12 21:41:14]

I like this CL much better!

Apart from some structural comments, I'm not sure about the arguments object
handling. Can we defer that part to another CL? That gets the overall .call
inlining already out of the way.

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

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode7978
src/hydrogen.cc:7978: bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
Does it make sense to have both methods? I'm pretty sure that by having both,
people will start calling the other version, which may not be correct?

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode7984
src/hydrogen.cc:7984: ) {
Nit: put ) { back on the previous line

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8005
src/hydrogen.cc:8005: //  expr could be .call or .apply so it's not safe to rely
on its
I don't think we need this comment...

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8570
src/hydrogen.cc:8570: if (TryInlineApply(known_function, expr,
args_count_no_receiver)) {
Seems like TryIndirectCall splits to apply and call handling, which join again
in HandleIndirectCall. It's a bit weird that it here again has special handling
for Apply. Is it really specific to Apply, or should it just be named
TryInlineIndirectCall?

Or perhaps we can even merge it with TryInlineCall since they don't behave that
different anymore.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8575
src/hydrogen.cc:8575: HValue* receiver =
environment()->ExpressionStackAt(args_count_no_receiver);
Can you move this code into TryInlineBuiltinMethodCall?

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8581
src/hydrogen.cc:8581: if (TryInlineBuiltinMethodCall(expr, receiver, map,
known_function,
This was already done in BuildFunctionCallCall. Can we avoid this duplication?
Either pass in a flag for the extra behavior, or move this out to the Apply
variant. Well, I guess we first have to think about the extra behavior tout
court... see the comment down below.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8603
src/hydrogen.cc:8603: !expr->target()->shared()->HasBuiltinFunctionId()) {
What about creating both BuildFunctionCall and BuildFunctionApply, and using a
switch to jump to those? I think you did that in your previous CL... That makes
it a bit more symmetrical.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8686
src/hydrogen.cc:8686: receiver = is_builtin_call ? receiver :
BuildWrapReceiver(receiver, function);
You can't just skip BuildWrapReceiver for builtins. BuildWrapReceiver has the
required logic to figure out whether it can be skipped or not (skipped only when
it's in strict-mode). Slice is in strict-mode, which is what you care about
most, so that should work out I presume?

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8710
src/hydrogen.cc:8710: // be allowed after all.
Iek! So what if I do the following?

a = [];
a.push(arguments);
return a;

[p.antonov, 2014-06-13 08:34:24]

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

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode7978
src/hydrogen.cc:7978: bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> Does it make sense to have both methods? I'm pretty sure that by having both,
> people will start calling the other version, which may not be correct?

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode7984
src/hydrogen.cc:7984: ) {
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> Nit: put ) { back on the previous line

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8005
src/hydrogen.cc:8005: //  expr could be .call or .apply so it's not safe to rely
on its
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> I don't think we need this comment...

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8570
src/hydrogen.cc:8570: if (TryInlineApply(known_function, expr,
args_count_no_receiver)) {
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> Seems like TryIndirectCall splits to apply and call handling, which join again
> in HandleIndirectCall. It's a bit weird that it here again has special
handling
> for Apply. Is it really specific to Apply, or should it just be named
> TryInlineIndirectCall?
> 
> Or perhaps we can even merge it with TryInlineCall since they don't behave
that
> different anymore.

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8575
src/hydrogen.cc:8575: HValue* receiver =
environment()->ExpressionStackAt(args_count_no_receiver);
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> Can you move this code into TryInlineBuiltinMethodCall? 

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8581
src/hydrogen.cc:8581: if (TryInlineBuiltinMethodCall(expr, receiver, map,
known_function,
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> This was already done in BuildFunctionCallCall. Can we avoid this duplication?
> Either pass in a flag for the extra behavior, or move this out to the Apply
> variant. Well, I guess we first have to think about the extra behavior tout
> court... see the comment down below.

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8603
src/hydrogen.cc:8603: !expr->target()->shared()->HasBuiltinFunctionId()) {
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> What about creating both BuildFunctionCall and BuildFunctionApply, and using a
> switch to jump to those? I think you did that in your previous CL... That
makes
> it a bit more symmetrical.

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8686
src/hydrogen.cc:8686: receiver = is_builtin_call ? receiver :
BuildWrapReceiver(receiver, function);
On 2014/06/12 21:41:14, Toon Verwaest wrote:
> You can't just skip BuildWrapReceiver for builtins. BuildWrapReceiver has the
> required logic to figure out whether it can be skipped or not (skipped only
when
> it's in strict-mode). Slice is in strict-mode, which is what you care about
> most, so that should work out I presume?

Done.

https://codereview.chromium.org/335683002/diff/1/src/hydrogen.cc#newcode8710
src/hydrogen.cc:8710: // be allowed after all.
On 2014/06/12 21:41:13, Toon Verwaest wrote:
> Iek! So what if I do the following?
> 
> a = [];
> a.push(arguments);
> return a;

Done.

[p.antonov, 2014-06-13 08:36:22]

For handling arguments, I think we need some way to signal that a builtin can
handle arguments properly, or? Or should all the builtins that cannot handle
arguments check for them (e.g. in the case of push it would check if the first
argument is arguments, it already checks the receiver)?

[Toon Verwaest, 2014-06-13 12:03:41]

Nice, lgtm with some last comments regarding the tests.
If you address those, I'll land.

https://codereview.chromium.org/335683002/diff/20001/test/mjsunit/compiler/in...
File test/mjsunit/compiler/inlined-call.js (right):

https://codereview.chromium.org/335683002/diff/20001/test/mjsunit/compiler/in...
test/mjsunit/compiler/inlined-call.js:94: assertEquals(void 0, funStrictRecv);
What about just using undefined? :)

https://codereview.chromium.org/335683002/diff/20001/test/mjsunit/compiler/in...
test/mjsunit/compiler/inlined-call.js:120: assertTrue(this instanceof Number);
Perhaps also check that it's actually called by doing something like above where
you let the receiver escape? In this case, just write a value into the global
scope?

[p.antonov, 2014-06-13 12:12:35]

https://codereview.chromium.org/335683002/diff/20001/test/mjsunit/compiler/in...
File test/mjsunit/compiler/inlined-call.js (right):

https://codereview.chromium.org/335683002/diff/20001/test/mjsunit/compiler/in...
test/mjsunit/compiler/inlined-call.js:94: assertEquals(void 0, funStrictRecv);
On 2014/06/13 12:03:41, Toon Verwaest wrote:
> What about just using undefined? :)

Done.

https://codereview.chromium.org/335683002/diff/20001/test/mjsunit/compiler/in...
test/mjsunit/compiler/inlined-call.js:120: assertTrue(this instanceof Number);
On 2014/06/13 12:03:41, Toon Verwaest wrote:
> Perhaps also check that it's actually called by doing something like above
where
> you let the receiver escape? In this case, just write a value into the global
> scope?

Done.

[Toon Verwaest, 2014-06-13 12:52:38]

Committed patchset #4 manually as r21840 (presubmit successful).

[Jakob Kummerow, 2014-06-19 13:50:38]

This had to be reverted in r21887.

Firstly, it misses several checks, see
https://codereview.chromium.org/346473002/.

Also, the expression stack is out of sync with full-codegen when the arguments
are visited, which causes deopts to mess up the frame, leading to crashes later
on. We don't have a reduced repro for that yet; the full-blown repro is to:
(1) build Chrome with a V8 version containing this patch. You might want to
build in Debug mode with GYP_DEFINES="component=shared_library
v8_optimized_debug=0".
(2) visit http://video.search.yahoo.com/. You might want to run with flags:
out/Debug/chrome --no-sandbox --renderer-cmd-prefix="xterm -e gdb -ex run -args"
--js-flags="--trace-deopt --print-opt-code --code-comments".

[p.antonov, 2014-06-23 13:59:23]

I can add the checks but I don't know how the expression stack is out of sync or
how it could be synced.

[Jakob Kummerow, 2014-06-23 15:13:32]

On 2014/06/23 13:59:23, p.antonov wrote:
> I don't know how the expression stack is out of sync or
> how it could be synced.

Basically, at any deopt point (i.e. any time a potentially deoptimizing
instruction might be introduced, which in particular includes all recursive
VisitForFoo(...) calls), the layout (i.e. both size and contents) of the
expression stack must be the same in full codegen (e.g. full-codegen-ia32.cc)
and Hydrogen. You can't just call Push()/Pop()/Drop() arbitrarily. If you
carefully trace through the stack's contents at every step in both full codegen
and hydrogen, you'll find the places where this CL currently violates this rule.
If/when you re-upload, please do add tests covering those cases.

[p.antonov, 2014-06-24 08:59:30]

On 2014/06/23 15:13:32, Jakob wrote:
> You can't just call Push()/Pop()/Drop() arbitrarily.

Ok I see what you mean, and actually you simply need something like a property
read deoptimization in the first argument and it will crash:

    // Flags: --allow-natives-syntax --noalways-opt

    function callsFReceiver(o) {
        return [].f.call(o.m, 1, 2, 3);
    }

    Array.prototype.f = function() {
        return this.m;
    };


    var o1 = {m: 1};
    var o2 = {a: 0, m:1};

    var r1 = callsFReceiver(o1);
    callsFReceiver(o1);
    %OptimizeFunctionOnNextCall(callsFReceiver);
    var r2 = callsFReceiver(o1);
    callsFReceiver(o2);
    var r3 = callsFReceiver(o1);

    print(r1 === r2);
    print(r2 === r3);

    callsFReceiver(o1);
    callsFReceiver(o1);
    %OptimizeFunctionOnNextCall(callsFReceiver);
    callsFReceiver(o1);

I was indeed arbitrarily calling the stack manipulation methods.