Introduce concurrent on-stack replacement.

Introduce concurrent on-stack replacement.

Currently disabled behind --concurrent-osr.

R=titzer@chromium.org
BUG=

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

[titzer, 2013-09-02 17:03:34]

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

https://codereview.chromium.org/23710014/diff/1/src/compiler.cc#newcode968
src/compiler.cc:968: ASSERT(FLAG_concurrent_recompilation);
Don't assert flags; they should only be used to turn on functionality.

https://codereview.chromium.org/23710014/diff/1/src/compiler.cc#newcode993
src/compiler.cc:993: FullCodeGenerator::BackEdgeTableIterator
back_edges(shared->code(), &no_gc);
It seems weird to make a BackEdgeTableIterator and then use it to search. I
think that probably means you want a SearchBackEdgeTable method on Code.

https://codereview.chromium.org/23710014/diff/1/src/compiler.cc#newcode1116
src/compiler.cc:1116: static uint32_t CurrentPcOffset(Isolate* isolate,
Why are you doing a search for the PC of the top frame? This is should already
be on the stack when calling the builtin, and thus available to be passed in.

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

https://codereview.chromium.org/23710014/diff/1/src/compiler.h#newcode505
src/compiler.h:505: osr_pc_offset_(osr_pc_offset),
This is the PC offset of the backedge. It seems weird that it's going be part of
the compiler. Seems like it would fit better in the CompilationInfo.

https://codereview.chromium.org/23710014/diff/1/src/runtime.cc
File src/runtime.cc (left):

https://codereview.chromium.org/23710014/diff/1/src/runtime.cc#oldcode8601
src/runtime.cc:8601: 
Ah, now I see where this code came from. Why is this better in the compiler? I
think the runtime should do the stack munging and the compiler should stay out
of that business.

[Yang, 2013-09-03 08:50:09]

Addressed comments.

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

https://codereview.chromium.org/23710014/diff/1/src/compiler.cc#newcode993
src/compiler.cc:993: FullCodeGenerator::BackEdgeTableIterator
back_edges(shared->code(), &no_gc);
On 2013/09/02 17:03:35, titzer wrote:
> It seems weird to make a BackEdgeTableIterator and then use it to search. I
> think that probably means you want a SearchBackEdgeTable method on Code.

Done.

https://codereview.chromium.org/23710014/diff/1/src/compiler.cc#newcode1116
src/compiler.cc:1116: static uint32_t CurrentPcOffset(Isolate* isolate,
On 2013/09/02 17:03:35, titzer wrote:
> Why are you doing a search for the PC of the top frame? This is should already
> be on the stack when calling the builtin, and thus available to be passed in.

Already thought of that. Let me work on that on a separate CL.

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

https://codereview.chromium.org/23710014/diff/1/src/compiler.h#newcode505
src/compiler.h:505: osr_pc_offset_(osr_pc_offset),
On 2013/09/02 17:03:35, titzer wrote:
> This is the PC offset of the backedge. It seems weird that it's going be part
of
> the compiler. Seems like it would fit better in the CompilationInfo.

Done.

https://codereview.chromium.org/23710014/diff/1/src/runtime.cc
File src/runtime.cc (left):

https://codereview.chromium.org/23710014/diff/1/src/runtime.cc#oldcode8601
src/runtime.cc:8601: 
On 2013/09/02 17:03:35, titzer wrote:
> Ah, now I see where this code came from. Why is this better in the compiler? I
> think the runtime should do the stack munging and the compiler should stay out
> of that business.

I'll have the builtin fetch the pc from stack, subtract from the instruction
start of the code object and return the pc offset as smi. In a separate CL.

[titzer, 2013-09-03 14:05:36]

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode972
src/compiler.cc:972: ASSERT(osr_pc_offset != 0 ||
closure->IsMarkedForConcurrentRecompilation());
Not sure what you are trying to assert here.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode994
src/compiler.cc:994: BailoutId osr_ast_id =
shared->code()->TranslatePcOffsetToAstId(
Are you calling this method in another place? Otherwise, you're passing in a
pointer to a loop_depth variable just to get it out for printing. That seems
weird. Pointers to local variables should be avoided.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1121
src/compiler.cc:1121: Handle<Code> unoptimized) {
Please put the mechanism for finding the osr_pc_offset back into the runtime.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1154
src/compiler.cc:1154: BailoutId* ast_id) {
This should take the osr_pc_offset as a parameter.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1174
src/compiler.cc:1174: *ast_id = unoptimized->TranslatePcOffsetToAstId(pc_offset,
&loop_depth);
If you move this code back to the runtime, you don't need to pass pointers to a
BailoutId around.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1245
src/compiler.cc:1245: if (!function->IsOptimized()) {
You shouldn't check function->IsOptimized, but look at the result of the code,
which should be in the compilation info. Soon, OSR compilations won't install
the code into the function.

https://codereview.chromium.org/23710014/diff/7001/test/mjsunit/compiler/opti...
File test/mjsunit/compiler/optimized-for-in.js (right):

https://codereview.chromium.org/23710014/diff/7001/test/mjsunit/compiler/opti...
test/mjsunit/compiler/optimized-for-in.js:29: // Flags:
--no-speculative-concurrent-osr
What's wrong here?

[Yang, 2013-09-03 14:49:44]

addressed comments

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode972
src/compiler.cc:972: ASSERT(osr_pc_offset != 0 ||
closure->IsMarkedForConcurrentRecompilation());
On 2013/09/03 14:05:36, titzer wrote:
> Not sure what you are trying to assert here.

Asserting that if we are not recompiling for OSR, we get here via the
ConcurrentRecompile builtin. It's not absolutely necessary, so I can remove it
if it bothers you.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode994
src/compiler.cc:994: BailoutId osr_ast_id =
shared->code()->TranslatePcOffsetToAstId(
On 2013/09/03 14:05:36, titzer wrote:
> Are you calling this method in another place? Otherwise, you're passing in a
> pointer to a loop_depth variable just to get it out for printing. That seems
> weird. Pointers to local variables should be avoided.

Done.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1121
src/compiler.cc:1121: Handle<Code> unoptimized) {
On 2013/09/03 14:05:36, titzer wrote:
> Please put the mechanism for finding the osr_pc_offset back into the runtime.

I got a CL in queue that removes this completely. Can we leave this here for
now?

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1154
src/compiler.cc:1154: BailoutId* ast_id) {
On 2013/09/03 14:05:36, titzer wrote:
> This should take the osr_pc_offset as a parameter.

Ditto. CL queued up doing exactly that.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1174
src/compiler.cc:1174: *ast_id = unoptimized->TranslatePcOffsetToAstId(pc_offset,
&loop_depth);
On 2013/09/03 14:05:36, titzer wrote:
> If you move this code back to the runtime, you don't need to pass pointers to
a
> BailoutId around.

I changed this so that we return the AST id directly. There is no need for a
bool return value.

https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1245
src/compiler.cc:1245: if (!function->IsOptimized()) {
On 2013/09/03 14:05:36, titzer wrote:
> You shouldn't check function->IsOptimized, but look at the result of the code,
> which should be in the compilation info. Soon, OSR compilations won't install
> the code into the function.

Done.

https://codereview.chromium.org/23710014/diff/7001/test/mjsunit/compiler/opti...
File test/mjsunit/compiler/optimized-for-in.js (right):

https://codereview.chromium.org/23710014/diff/7001/test/mjsunit/compiler/opti...
test/mjsunit/compiler/optimized-for-in.js:29: // Flags:
--no-speculative-concurrent-osr
On 2013/09/03 14:05:36, titzer wrote:
> What's wrong here?

This is the only test where we use %OptimizeFunctionOnNextCall(fun, "osr") to
force OSR. But the loops are too short for the concurrent thread to complete
compilation and install, so with speculative concurrent osr, we are not testing
osr entry at all.

[titzer, 2013-09-04 09:23:23]

LGTM with these last few comments.

https://codereview.chromium.org/23710014/diff/18001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/23710014/diff/18001/src/compiler.cc#newcode973
src/compiler.cc:973: ASSERT(compiling_for_osr ||
closure->IsMarkedForConcurrentRecompilation());
On second thought, yes, please take the assert out. It doesn't make sense for
the compiler to dependent on the current state of a function. It should just
generate code.

https://codereview.chromium.org/23710014/diff/18001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/23710014/diff/18001/src/flag-definitions.h#ne...
src/flag-definitions.h:334: DEFINE_bool(speculative_concurrent_osr, false,
s/speculative_//g

It'd only be speculative if the compiler tried OSRing code before it's executed.

https://codereview.chromium.org/23710014/diff/18001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/23710014/diff/18001/src/objects.cc#newcode10328
src/objects.cc:10328: BailoutId Code::TranslatePcOffsetToAstId(uint32_t
pc_offset) {
ASSERT(kind() == FUNCTION)

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
File src/optimizing-compiler-thread.cc (right):

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
src/optimizing-compiler-thread.cc:112: ASSERT(FLAG_speculative_concurrent_osr);
Don't assert flags.

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
src/optimizing-compiler-thread.cc:157: ASSERT(FLAG_speculative_concurrent_osr);
Here and several places below.

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
File src/optimizing-compiler-thread.h (right):

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
src/optimizing-compiler-thread.h:56: osr_candidates_(2),
Why not kReadyForOSRLimit?

[Yang, 2013-09-04 12:55:12]

https://codereview.chromium.org/23710014/diff/18001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/23710014/diff/18001/src/compiler.cc#newcode973
src/compiler.cc:973: ASSERT(compiling_for_osr ||
closure->IsMarkedForConcurrentRecompilation());
On 2013/09/04 09:23:23, titzer wrote:
> On second thought, yes, please take the assert out. It doesn't make sense for
> the compiler to dependent on the current state of a function. It should just
> generate code.

Done.

https://codereview.chromium.org/23710014/diff/18001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/23710014/diff/18001/src/flag-definitions.h#ne...
src/flag-definitions.h:334: DEFINE_bool(speculative_concurrent_osr, false,
On 2013/09/04 09:23:23, titzer wrote:
> s/speculative_//g
> 
> It'd only be speculative if the compiler tried OSRing code before it's
executed.

Done.

https://codereview.chromium.org/23710014/diff/18001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/23710014/diff/18001/src/objects.cc#newcode10328
src/objects.cc:10328: BailoutId Code::TranslatePcOffsetToAstId(uint32_t
pc_offset) {
On 2013/09/04 09:23:23, titzer wrote:
> ASSERT(kind() == FUNCTION)

Done. The BackEdgeTableIterator asserts the same, but I guess it doesn't hurt.

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
File src/optimizing-compiler-thread.cc (right):

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
src/optimizing-compiler-thread.cc:112: ASSERT(FLAG_speculative_concurrent_osr);
On 2013/09/04 09:23:23, titzer wrote:
> Don't assert flags.

Done.

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
src/optimizing-compiler-thread.cc:157: ASSERT(FLAG_speculative_concurrent_osr);
On 2013/09/04 09:23:23, titzer wrote:
> Here and several places below.

Done.

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
File src/optimizing-compiler-thread.h (right):

https://codereview.chromium.org/23710014/diff/18001/src/optimizing-compiler-t...
src/optimizing-compiler-thread.h:56: osr_candidates_(2),
On 2013/09/04 09:23:23, titzer wrote:
> Why not kReadyForOSRLimit?

Done.

[Yang, 2013-09-04 12:56:18]

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