[Interpreter] Support compiling for baseline on return from interpreted function.

[Interpreter] Support compiling for baseline on return from interpreted function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn, which leaves the
interpreted frame and recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/f241a61a3409cef1f8d631d5c3ee992e182f294e
Cr-Commit-Position: refs/heads/master@{#36360}

[rmcilroy, 2016-05-11 11:45:21]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[rmcilroy, 2016-05-11 11:45:22]

Michi, this is what we discussed yesterday with Arm and x64 ports. I'll add the
other ports if you are happy with the approach.

[Yang, 2016-05-11 11:52:42]

On 2016/05/11 11:45:22, rmcilroy wrote:
> Michi, this is what we discussed yesterday with Arm and x64 ports. I'll add
the
> other ports if you are happy with the approach.

src/snapshot lgtm.

[Michael Starzinger, 2016-05-11 13:04:27]

Yep, approach is fine with me.

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc#newcode895
src/compiler.cc:895: os << "[marking " << Brief(*function) << " for baseline
recompilation "
nit: Let's print s/*function/function->shared()/ here instead, because we are
not marking this particular closures, but all of them essentially.

https://codereview.chromium.org/1965343002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1965343002/diff/1/src/objects-inl.h#newcode4791
src/objects-inl.h:4791: bool Code::IsJavaScriptCode() {
This predicate is no longer used in the codebase. I wanted to remove it twice
already. Can we remove it?

[rmcilroy, 2016-05-12 13:10:31]

Patchset #2 (id:20001) has been deleted

[rmcilroy, 2016-05-12 13:11:47]

Updated based on https://codereview.chromium.org/1969423002. I'm much happier
with this approach - PTAL.

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc#newcode895
src/compiler.cc:895: os << "[marking " << Brief(*function) << " for baseline
recompilation "
On 2016/05/11 13:04:27, Michael Starzinger wrote:
> nit: Let's print s/*function/function->shared()/ here instead, because we are
> not marking this particular closures, but all of them essentially.

Done. This brings up the point of whether we should be somehow marking the
shared function instead of the closure for Baseline in the normal case, since we
don't depend on the closure like CS does?

https://codereview.chromium.org/1965343002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1965343002/diff/1/src/objects-inl.h#newcode4791
src/objects-inl.h:4791: bool Code::IsJavaScriptCode() {
On 2016/05/11 13:04:27, Michael Starzinger wrote:
> This predicate is no longer used in the codebase. I wanted to remove it twice
> already. Can we remove it?

Done.

[rmcilroy, 2016-05-12 13:12:30]

Description was changed from

==========
[Interpreter] Support compiling for baseline on return from interpreted
function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn. The Interpreter never actually uses
this return address, it always jumps to the
InterpreterExitTrampoline. The InterpreterExitTrampoline is modified to
check for this value and, if it is present, recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Support compiling for baseline on return from interpreted
function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn, which leaves the 
interpreted frame and recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N
==========

[Michael Starzinger, 2016-05-13 09:01:40]

LGTM with comments. Yes, I agree, new approach is definitely cleaner. I like it.

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc#newcode895
src/compiler.cc:895: os << "[marking " << Brief(*function) << " for baseline
recompilation "
On 2016/05/12 13:11:47, rmcilroy wrote:
> On 2016/05/11 13:04:27, Michael Starzinger wrote:
> > nit: Let's print s/*function/function->shared()/ here instead, because we
are
> > not marking this particular closures, but all of them essentially.
> 
> Done. This brings up the point of whether we should be somehow marking the
> shared function instead of the closure for Baseline in the normal case, since
we
> don't depend on the closure like CS does?

Hmm. Interesting idea. I suppose it would be possible to install the
CompileBaseline builtin on the SharedFunctionInfo itself (as opposed to only the
specific closure). I think the self-healing mechanism we added to the
interpreter entry trampoline would require adapting, because there might still
be many closures on the heap that have the interpreter entry trampoline
installed and just switching SharedFunctionInfo::code won't have an effect on
them. If we tackle this, then lets do it in a separate CL though.

https://codereview.chromium.org/1965343002/diff/40001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1965343002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:613: // activation on the stack.
nit: Can we keep the generic "Leave the frame (also dropping the register
file)." comment here?

https://codereview.chromium.org/1965343002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:616: // Drop receiver + arguments and return.
nit: The "and return" part of the comment no longer applies.

[rmcilroy, 2016-05-17 11:28:31]

Done ports and added a cctest too.

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1965343002/diff/1/src/compiler.cc#newcode895
src/compiler.cc:895: os << "[marking " << Brief(*function) << " for baseline
recompilation "
On 2016/05/13 09:01:39, Michael Starzinger wrote:
> On 2016/05/12 13:11:47, rmcilroy wrote:
> > On 2016/05/11 13:04:27, Michael Starzinger wrote:
> > > nit: Let's print s/*function/function->shared()/ here instead, because we
> are
> > > not marking this particular closures, but all of them essentially.
> > 
> > Done. This brings up the point of whether we should be somehow marking the
> > shared function instead of the closure for Baseline in the normal case,
since
> we
> > don't depend on the closure like CS does?
> 
> Hmm. Interesting idea. I suppose it would be possible to install the
> CompileBaseline builtin on the SharedFunctionInfo itself (as opposed to only
the
> specific closure). I think the self-healing mechanism we added to the
> interpreter entry trampoline would require adapting, because there might still
> be many closures on the heap that have the interpreter entry trampoline
> installed and just switching SharedFunctionInfo::code won't have an effect on
> them. If we tackle this, then lets do it in a separate CL though.

Yes absolutely, only in a separate CL and if it's useful.

https://codereview.chromium.org/1965343002/diff/40001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1965343002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:613: // activation on the stack.
On 2016/05/13 09:01:40, Michael Starzinger wrote:
> nit: Can we keep the generic "Leave the frame (also dropping the register
> file)." comment here?

Done.

https://codereview.chromium.org/1965343002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:616: // Drop receiver + arguments and return.
On 2016/05/13 09:01:40, Michael Starzinger wrote:
> nit: The "and return" part of the comment no longer applies.

Done.

[rmcilroy, 2016-05-17 11:28:36]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 11:28:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965343002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965343002/60001

[commit-bot: I haz the power, 2016-05-17 11:48:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 11:48:25]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[rmcilroy, 2016-05-18 11:33:16]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-18 11:33:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965343002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965343002/80001

[commit-bot: I haz the power, 2016-05-18 12:15:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-18 12:15:34]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[rmcilroy, 2016-05-18 22:32:27]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-18 22:32:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965343002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965343002/100001

[commit-bot: I haz the power, 2016-05-18 23:23:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-18 23:23:58]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[rmcilroy, 2016-05-19 11:46:06]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 11:46:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965343002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965343002/120001

[commit-bot: I haz the power, 2016-05-19 12:18:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 12:18:52]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-05-19 12:25:13]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-05-19 12:25:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1965343002/#ps120001
(title: "Fix test for nosnap build")

[commit-bot: I haz the power, 2016-05-19 12:25:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965343002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965343002/120001

[commit-bot: I haz the power, 2016-05-19 12:27:23]

Description was changed from

==========
[Interpreter] Support compiling for baseline on return from interpreted
function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn, which leaves the 
interpreted frame and recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Support compiling for baseline on return from interpreted
function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn, which leaves the 
interpreted frame and recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2016-05-19 12:27:24]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-05-19 12:29:00]

Description was changed from

==========
[Interpreter] Support compiling for baseline on return from interpreted
function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn, which leaves the 
interpreted frame and recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Support compiling for baseline on return from interpreted
function.

We cannot tier up from interpreted to baseline code when there is an
activation of the function on the stack. This significantly regresses
the performance of recursive functions since they are unlikely to get
tiered up.

This CL adds the ability for a function to be marked for baseline
compilation when it returns. To do this we patch the
InterpreterEntryTrampoline return address to point to
InterpreterMarkBaselineOnReturn, which leaves the
interpreted frame and recompile the function for
baseline.

This improves the score of EarlyBoyer by ~8x for Ignition.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/f241a61a3409cef1f8d631d5c3ee992e182f294e
Cr-Commit-Position: refs/heads/master@{#36360}
==========

[commit-bot: I haz the power, 2016-05-19 12:29:01]

Patchset 6 (id:??) landed as
https://crrev.com/f241a61a3409cef1f8d631d5c3ee992e182f294e
Cr-Commit-Position: refs/heads/master@{#36360}