Generate a custom OSR entrypoint for OSR compiles on all platforms, and transition to optimized cod…

Generate a custom OSR entrypoint for OSR compiles on all platforms, and transition to optimized code using the special entrypoint, instead of through the deoptimizer. Do not install the OSR compiled code as _the_ optimized code for a function.
Remove OSR-related stuff from deoptimizer.
BUG=
R=mstarzinger@chromium.org

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

[titzer, 2013-07-31 09:15:39]

On 2013/07/31 08:46:02, titzer wrote:

A little more background for review:

PRIOR: When the profiler decide it's time to do an OSR compile for a function on
the stack, it patches the backedges in its code to call into the builtin created
by Builtins::Generate_OnStackReplacement. This builtin calls the runtime
function Runtime_CompileForOnStackReplacement with an AST id that indicates
which loop should be the special OSR loop during the compile. The runtime
compiles the function for OSR, and if successful, installs the code into the
function and returns the same AST id to the stub. This indicates to the stub
that the compile was successful and that it should call the Deoptimizer to
translate the unoptimized from into an optimized frame. The Deoptimizer would
find the optimized code hanging off the function, find the right translation
given the AST id, and then translate the frame.

NOW: OSR is triggered in the same way by the profiler, but what happens after
Runtime_CompileForOnStackReplacement has changed. First, the OSR compiled code
is _not_ installed as the optimized code for the function (which is what
necessitated several prior CLs in this direction). Second, the
Runtime_CompileForOnStackReplacement function returns the actual Code* object to
the builtin stub. If this code object is not NULL, then the stub will calculate
its custom OSR entry point and jump directly there, with no involvement from the
deoptimizer.

CUSTOM ENTRYPOINT: We now generate a custom entrypoint in the OSR-compiled
optimized code that is entered directly from the unoptimized code. When this
custom entrypoint is entered, the stack frame is exactly the same as the
unoptimized frame. We are careful to assign the UnknownOsrValues in the
optimized code to be exactly the right slots in the frame, so that their
locations match up. Then we make sure to make the optimized frame big enough to
subsume the unoptimized frame. When the entrypoint is entered, we simply adjust
the size of the frame, and like magic, the optimized code can run and get both
the parameters and UnknownOsrValues from exactly the right spots in its own
frame. The rest of the unoptimized frame (i.e. non-OSR values) is just dead.

[Michael Starzinger, 2013-07-31 14:55:50]

Nice change. A bunch of comments. The one major comment is in runtime.cc, where
we look for activations of the optimized code after a deopt.

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

https://codereview.chromium.org/21340002/diff/1/src/compiler.cc#newcode923
src/compiler.cc:923: if (install && InstallCodeFromOptimizedCodeMap(info))
return true;
Both InsertCodeIntoOptimizedCodeMap() and InstallCodeFromOptimizedCodeMap()
already have a check for "info->osr_ast_id().IsNone()" in them.

https://codereview.chromium.org/21340002/diff/1/src/compiler.cc#newcode948
src/compiler.cc:948: if (install) {
Instead of passing the "install" boolean flag we could just check that
"osr_ast_id()" is not set. Simplifies the interface and is in line with current
semantics of that field.

https://codereview.chromium.org/21340002/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/21340002/diff/1/src/compiler.h#newcode593
src/compiler.h:593: static bool CompileLazy(CompilationInfo* info, bool install
= true);
The boolean flag is obsolete, see commment in compiler.cc about that.

https://codereview.chromium.org/21340002/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/21340002/diff/1/src/deoptimizer.cc#newcode2328
src/deoptimizer.cc:2328: PrintF("Deopt input frame size=%d (fixed_size=%d +
fp_sp_delta=%d - %d)\n",
nit: Can we remove this left-over debug code again?

https://codereview.chromium.org/21340002/diff/1/src/deoptimizer.cc#newcode2337
src/deoptimizer.cc:2337: PrintF("Deopt input frame size=%d (fixed_size=%d +"
Likewise.

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

https://codereview.chromium.org/21340002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5137: : environment_(environment),
It seems dangerous to me to preserve the full HEnvironment from graph building
all the way down to the Lithium level. AFAICT it's only used to figure out
whether the HUnknownOSRValue comes from a parameter slot and to calculate the
spill index. Can we instead use the replayed HEnvironment in the LChunkBuilder
and/or extract the information here during graph construction?

https://codereview.chromium.org/21340002/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5151: HEnvironment *environment() { return
environment_; }
nit: The star character sticks to the left.

https://codereview.chromium.org/21340002/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/21340002/diff/1/src/ia32/builtins-ia32.cc#new...
src/ia32/builtins-ia32.cc:1352: __ mov(ebx, Operand(ebx, FixedArray::kHeaderSize
+
nit: Use the following instead, easier to read ...

__ mov(ebx, FieldOperand(ebx, FixedArray::OffsetOfElementAt(index)));

https://codereview.chromium.org/21340002/diff/1/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):

https://codereview.chromium.org/21340002/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.h:228: void GenerateOsrPrologue();
nit: Let's add a short one-liner comment about this method (also saying that it
sets the OSR-entry offset).

https://codereview.chromium.org/21340002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/21340002/diff/1/src/objects.cc#newcode9294
src/objects.cc:9294: bool install) {
The boolean flag is obsolete, see comments in "compiler.cc" about that.

https://codereview.chromium.org/21340002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21340002/diff/1/src/runtime.cc#newcode8293
src/runtime.cc:8293: bool has_other_activations = false;
We should only look for activations of optimized code on the stack. If
function->code() is unoptimized then lookup for other activations of
function->code() is not correct.

We need some way to get the _optimized_ code from which we deoptimized, even if
it's not installed in the function. AFAICT, the Deoptimizer::compiled_code field
should be accurate at the very top of this method. The below loop should look
for activations of that code object instead.

https://codereview.chromium.org/21340002/diff/1/src/runtime.cc#newcode8561
src/runtime.cc:8561: return osr_code.is_null() ? NULL: *osr_code;
nit: Missing white-space in front of the colon.

[vincent.belliard.fr, 2013-07-31 15:07:44]

https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc#newco...
src/arm/builtins-arm.cc:999: __ add(r0, r0, r1);
You can do the SmiUntag and the add in one instruction:
add(r0, r0, Operand::SmiUntag(r1));

https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc#newco...
src/arm/builtins-arm.cc:1000: __ add(r0, r0, Operand(Code::kHeaderSize -
kHeapObjectTag));
the result of add can be directly stored in lr. It avoids the mov.

[Michael Starzinger, 2013-07-31 16:29:33]

https://codereview.chromium.org/21340002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21340002/diff/1/src/runtime.cc#newcode8293
src/runtime.cc:8293: bool has_other_activations = false;
On 2013/07/31 14:55:50, Michael Starzinger wrote:
> We should only look for activations of optimized code on the stack. If
> function->code() is unoptimized then lookup for other activations of
> function->code() is not correct.
> 
> We need some way to get the _optimized_ code from which we deoptimized, even
if
> it's not installed in the function. AFAICT, the Deoptimizer::compiled_code
field
> should be accurate at the very top of this method. The below loop should look
> for activations of that code object instead.

As discussed offline: Yes, your are right, if function->code() is not optimized,
then there should be no further activations on the stack because the same OSR
code cannot be reused. So we should just do an early return before looking for
activations if "!function->IsOptimized()". Agreed.

[titzer, 2013-08-01 07:39:43]

On 2013/07/31 15:07:44, vincent.belliard.fr wrote:
> https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc
> File src/arm/builtins-arm.cc (right):
> 
>
https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc#newco...
> src/arm/builtins-arm.cc:999: __ add(r0, r0, r1);
> You can do the SmiUntag and the add in one instruction:
> add(r0, r0, Operand::SmiUntag(r1));

This is not a SmiUntag, but a heap access (i.e. subtract by 1, not shift by 1).

> 
>
https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc#newco...
> src/arm/builtins-arm.cc:1000: __ add(r0, r0, Operand(Code::kHeaderSize -
> kHeapObjectTag));
> the result of add can be directly stored in lr. It avoids the mov.

Done. I forgot how nice it is to have a 3-address architecture.

[titzer, 2013-08-01 07:43:30]

On 2013/08/01 07:39:43, titzer wrote:
> On 2013/07/31 15:07:44, vincent.belliard.fr wrote:
> > https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc
> > File src/arm/builtins-arm.cc (right):
> > 
> >
>
https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc#newco...
> > src/arm/builtins-arm.cc:999: __ add(r0, r0, r1);
> > You can do the SmiUntag and the add in one instruction:
> > add(r0, r0, Operand::SmiUntag(r1));
> 
> This is not a SmiUntag, but a heap access (i.e. subtract by 1, not shift by
1).

Sorry, wrong line. SmiUntag it is.


> > 
> >
>
https://codereview.chromium.org/21340002/diff/1/src/arm/builtins-arm.cc#newco...
> > src/arm/builtins-arm.cc:1000: __ add(r0, r0, Operand(Code::kHeaderSize -
> > kHeapObjectTag));
> > the result of add can be directly stored in lr. It avoids the mov.
> 
> Done. I forgot how nice it is to have a 3-address architecture.

[titzer, 2013-08-01 09:08:01]

On 2013/07/31 14:55:50, Michael Starzinger wrote:
> Nice change. A bunch of comments. The one major comment is in runtime.cc,
where
> we look for activations of the optimized code after a deopt.
> 
> https://codereview.chromium.org/21340002/diff/1/src/compiler.cc
> File src/compiler.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/1/src/compiler.cc#newcode923
> src/compiler.cc:923: if (install && InstallCodeFromOptimizedCodeMap(info))
> return true;
> Both InsertCodeIntoOptimizedCodeMap() and InstallCodeFromOptimizedCodeMap()
> already have a check for "info->osr_ast_id().IsNone()" in them.

Done; I've added is_osr() to CompilationInfo to make this more explicit.

> 
> https://codereview.chromium.org/21340002/diff/1/src/compiler.cc#newcode948
> src/compiler.cc:948: if (install) {
> Instead of passing the "install" boolean flag we could just check that
> "osr_ast_id()" is not set. Simplifies the interface and is in line with
current
> semantics of that field.
> 

Done.

> https://codereview.chromium.org/21340002/diff/1/src/compiler.h
> File src/compiler.h (right):
> 
> https://codereview.chromium.org/21340002/diff/1/src/compiler.h#newcode593
> src/compiler.h:593: static bool CompileLazy(CompilationInfo* info, bool
install
> = true);
> The boolean flag is obsolete, see commment in compiler.cc about that.
> 
> https://codereview.chromium.org/21340002/diff/1/src/deoptimizer.cc
> File src/deoptimizer.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/1/src/deoptimizer.cc#newcode2328
> src/deoptimizer.cc:2328: PrintF("Deopt input frame size=%d (fixed_size=%d +
> fp_sp_delta=%d - %d)\n",
> nit: Can we remove this left-over debug code again?
> 

Done.

> https://codereview.chromium.org/21340002/diff/1/src/deoptimizer.cc#newcode2337
> src/deoptimizer.cc:2337: PrintF("Deopt input frame size=%d (fixed_size=%d +"
> Likewise.
> 

Done.

> https://codereview.chromium.org/21340002/diff/1/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/21340002/diff/1/src/hydrogen-instructions.h#n...
> src/hydrogen-instructions.h:5137: : environment_(environment),
> It seems dangerous to me to preserve the full HEnvironment from graph building
> all the way down to the Lithium level. AFAICT it's only used to figure out
> whether the HUnknownOSRValue comes from a parameter slot and to calculate the
> spill index. Can we instead use the replayed HEnvironment in the LChunkBuilder
> and/or extract the information here during graph construction?
> 

Didn't do this yet. I don't like keeping the entire environment around either; I
think maybe we just want to keep enough information for each HUnknownOsrValue so
that we can compute the correct spill index; it might be good enough to store
whether it's a parameter or a local.

>
https://codereview.chromium.org/21340002/diff/1/src/hydrogen-instructions.h#n...
> src/hydrogen-instructions.h:5151: HEnvironment *environment() { return
> environment_; }
> nit: The star character sticks to the left.
> 

Done.

> https://codereview.chromium.org/21340002/diff/1/src/ia32/builtins-ia32.cc
> File src/ia32/builtins-ia32.cc (right):
> 
>
https://codereview.chromium.org/21340002/diff/1/src/ia32/builtins-ia32.cc#new...
> src/ia32/builtins-ia32.cc:1352: __ mov(ebx, Operand(ebx,
FixedArray::kHeaderSize
> +
> nit: Use the following instead, easier to read ...
> 
> __ mov(ebx, FieldOperand(ebx, FixedArray::OffsetOfElementAt(index)));
> 

Done.

>
https://codereview.chromium.org/21340002/diff/1/src/ia32/lithium-codegen-ia32.h
> File src/ia32/lithium-codegen-ia32.h (right):
> 
>
https://codereview.chromium.org/21340002/diff/1/src/ia32/lithium-codegen-ia32...
> src/ia32/lithium-codegen-ia32.h:228: void GenerateOsrPrologue();
> nit: Let's add a short one-liner comment about this method (also saying that
it
> sets the OSR-entry offset).
> 

Done.

> https://codereview.chromium.org/21340002/diff/1/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/1/src/objects.cc#newcode9294
> src/objects.cc:9294: bool install) {
> The boolean flag is obsolete, see comments in "compiler.cc" about that.
> 

Done.

> https://codereview.chromium.org/21340002/diff/1/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/1/src/runtime.cc#newcode8293
> src/runtime.cc:8293: bool has_other_activations = false;
> We should only look for activations of optimized code on the stack. If
> function->code() is unoptimized then lookup for other activations of
> function->code() is not correct.

PTAL here. I modified this code to get the initial function and code from the
deoptimizer, and assert that at least the function matches up; it then searches
for functions on the stack that shared that same optimized code (which should be
none in the case of an OSR).

> 
> We need some way to get the _optimized_ code from which we deoptimized, even
if
> it's not installed in the function. AFAICT, the Deoptimizer::compiled_code
field
> should be accurate at the very top of this method. The below loop should look
> for activations of that code object instead.
> 
> https://codereview.chromium.org/21340002/diff/1/src/runtime.cc#newcode8561
> src/runtime.cc:8561: return osr_code.is_null() ? NULL: *osr_code;
> nit: Missing white-space in front of the colon.

Done.

[Michael Starzinger, 2013-08-01 09:25:29]

https://codereview.chromium.org/21340002/diff/14001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21340002/diff/14001/src/runtime.cc#newcode8247
src/runtime.cc:8247: if (it.frame()->function()->code() == *optimized_code) {
Yep, this method is looking good to me now.

[titzer, 2013-09-09 07:35:58]

On 2013/08/01 09:25:29, Michael Starzinger wrote:
> https://codereview.chromium.org/21340002/diff/14001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/14001/src/runtime.cc#newcode8247
> src/runtime.cc:8247: if (it.frame()->function()->code() == *optimized_code) {
> Yep, this method is looking good to me now.

PTAL, ready to land this.

[Michael Starzinger, 2013-09-09 15:34:43]

LGTM with final round of comments.

https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc#newcode1219
src/compiler.cc:1219: return result;  // return null.
nit: Can we just write down "Handle<Code>::null()" here? Much easier to read and
probably same or even better code.

https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc#newcode1244
src/compiler.cc:1244: return result;
Likewise.

https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1495: // TODO(titzer): Implement me!
Hmm, can we, implement this? It was implemented before, is there a reason you
removed it?

https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5001: HEnvironment *environment() { return
environment_; }
I still don't like the fact that we preserve the full environment here. At least
we are making a copy when we create the OSR-graph now, so I am fine with landing
as it is.

https://codereview.chromium.org/21340002/diff/21001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/21340002/diff/21001/src/objects.cc#newcode9327
src/objects.cc:9327: ASSERT(result !=
Isolate::Current()->has_pending_exception());
Please keep using info->isolate() here, Isolate::Current() is on death row.

https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc#newcode8417
src/runtime.cc:8417: ASSERT(frame->function() == *function);
nit: Move up one line.

https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc#newcode8595
src/runtime.cc:8595: // which the unoptimized code can jump into.
nit: Drop one of the "which".

[titzer, 2013-09-09 16:30:08]

On 2013/09/09 15:34:43, Michael Starzinger wrote:
> LGTM with final round of comments.
> 
> https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc#newcode1219
> src/compiler.cc:1219: return result;  // return null.
> nit: Can we just write down "Handle<Code>::null()" here? Much easier to read
and
> probably same or even better code.
> 

Done.

>
https://codereview.chromium.org/21340002/diff/21001/src/compiler.cc#newcode1244
> src/compiler.cc:1244: return result;
> Likewise.
> 

Done.

>
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions...
> src/hydrogen-instructions.cc:1495: // TODO(titzer): Implement me!
> Hmm, can we, implement this? It was implemented before, is there a reason you
> removed it?
> 

I think I removed it several weeks back on accident when I removed all the
informative definition crap. It's back now!

>
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/21340002/diff/21001/src/hydrogen-instructions...
> src/hydrogen-instructions.h:5001: HEnvironment *environment() { return
> environment_; }
> I still don't like the fact that we preserve the full environment here. At
least
> we are making a copy when we create the OSR-graph now, so I am fine with
landing
> as it is.
> 

Ack.

> https://codereview.chromium.org/21340002/diff/21001/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/21001/src/objects.cc#newcode9327
> src/objects.cc:9327: ASSERT(result !=
> Isolate::Current()->has_pending_exception());
> Please keep using info->isolate() here, Isolate::Current() is on death row.
> 

Done.

> https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc#newcode8417
> src/runtime.cc:8417: ASSERT(frame->function() == *function);
> nit: Move up one line.
>

Done.
 
> https://codereview.chromium.org/21340002/diff/21001/src/runtime.cc#newcode8595
> src/runtime.cc:8595: // which the unoptimized code can jump into.
> nit: Drop one of the "which".

Done.

[titzer, 2013-09-09 16:35:16]

Committed patchset #5 manually as r16599 (presubmit successful).