Refactor the compiling pipeline.

Refactor the compiling pipeline.

Goals:
 - easier to read, more suitable identifiers.
 - better distinction between compiling optimized/unoptimized code
 - compiler does not install code on the function.
 - easier to add features (e.g. caching optimized code for osr).
 - remove unnecessary code.

R=titzer@chromium.org

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

[Yang, 2013-12-09 12:34:44]

I realize that this is a huge CL. Feel free to suggest another reviewer for
this.

[titzer, 2013-12-09 14:49:27]

https://codereview.chromium.org/110203002/diff/20001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/accessors.cc#newcode653
src/accessors.cc:653: if (!Compiler::EnsureCompiled(function_handle,
KEEP_EXCEPTION)) {
Not sure why you negated this condition, can you switch it back?

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

https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:292: static void CallRuntimePassFunctionAndTailCall(
Maybe split this into a CallRuntimeWithFunction and then another method,
TailCall, since we also tail call in CallCompileOptimizedAndTailCall below...

https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:780: { FrameScope scope(masm, StackFrame::INTERNAL);
This is an almost exact copy of CallRuntimePassFunctionAndTailCall, but with a
known function id and an extra parameter...

https://codereview.chromium.org/110203002/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1515: script_name, 0, 0,
formatting changes on this line.

https://codereview.chromium.org/110203002/diff/20001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/builtins.h#newcode103
src/builtins.h:103: V(LazyCompile,                    BUILTIN, UNINITIALIZED,   
         \
s/LazyCompile/Compile/ 
?

https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode349
src/compiler.cc:349: if (!info()->closure()->PassesFilter(FLAG_hydrogen_filter))
{
Do we also want to set a bailout reason and disable optimization here?

I think we probably always want to pass the bailout reason to
Abort*Optimization()

https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode1215
src/compiler.cc:1215: ASSERT(info->shared_info()->scope_info() !=
ScopeInfo::Empty(isolate));
Don't know why this assert is here.

https://codereview.chromium.org/110203002/diff/20001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/compiler.h#newcode506
src/compiler.h:506: class RecompileJob: public ZoneObject {
Probably want to rename this class as well. Maybe OptimizedCompileJob?

https://codereview.chromium.org/110203002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/factory.cc#newcode765
src/factory.cc:765: result->MarkForCompileOptimized();
MarkForCompileOptimized reads weird...why not mark for optimization? 

Btw, why do we check result->is_compiled() here?

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc#newc...
src/full-codegen.cc:1715: for (uint32_t i = 0; i < back_edges.length(); i++) {
I'm not so sure about the assumption that the AST ids have a unique entry in the
backedge table, which appears to be already assumed in the previous code.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc#newc...
src/full-codegen.cc:1722: ASSERT(Verify(isolate, code,
code->allow_osr_at_loop_nesting_level()));
Probably not going to work if you have a recursive function, directly or
indirectly, that is OSR'd.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newco...
src/full-codegen.h:931: static void AddStackCheck(Code* code, uint32_t
pc_offset);
Why not a Handle<Code>?

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newco...
src/full-codegen.h:934: static void RemoveStackCheck(CompilationInfo* info);
And why does this one take a CompilationInfo? Does it remove all checks, or just
one?

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

https://codereview.chromium.org/110203002/diff/20001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:77: static void CallRuntimePassFunctionAndTailCall(
Similar comments to the arm port on naming here.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9474
src/objects.cc:9474: ASSERT(is_compiled() ||
GetIsolate()->DebuggerHasBreakPoints());
I don't know why we assert is_compiled()

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9486
src/objects.cc:9486: ASSERT(is_compiled() ||
GetIsolate()->DebuggerHasBreakPoints());
Same.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9487
src/objects.cc:9487: ASSERT(!IsOptimized());
Or this.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9489
src/objects.cc:9489: ASSERT(!shared()->is_generator());
Or that.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9594
src/objects.cc:9594: ASSERT(index > kEntriesStart);
>=?

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8307
src/runtime.cc:8307: PrintF("[lazy: ");
s/lazy/unoptimized/

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8314
src/runtime.cc:8314: ASSERT(!function->is_compiled());
Why?

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8334
src/runtime.cc:8334: if (!function->shared()->is_compiled()) {
I don't think we want this check. IIRC that forces all functions to be first
compiled unoptimized and then recompiled.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8356
src/runtime.cc:8356: if (code.is_null()) code =
isolate->builtins()->InOptimizationQueue();
I totally don't understand this. What if the optimized compile just fails
(without recompilation on)? What is it supposed to return?

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8610
src/runtime.cc:8610: Handle<Code> current_code(function->shared()->code());
Let's call this caller_code?

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode12686
src/runtime.cc:12686: static const ParseRestriction restriction =
NO_PARSE_RESTRICTION;
Why the extra local here?

https://codereview.chromium.org/110203002/diff/20001/test/cctest/test-compile...
File test/cctest/test-compiler.cc (right):

https://codereview.chromium.org/110203002/diff/20001/test/cctest/test-compile...
test/cctest/test-compiler.cc:108: Handle<String>(), 0, 0,
formatting changes

[Yang, 2013-12-10 11:22:03]

Addressed comments.

Regarding the is_compiled() asserts: we currently assume that we first compile
unoptimized code before optimizing, and guard that in several places. I'd like
to keep it this way for this CL. We may reconsider this later.

https://codereview.chromium.org/110203002/diff/20001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/accessors.cc#newcode653
src/accessors.cc:653: if (!Compiler::EnsureCompiled(function_handle,
KEEP_EXCEPTION)) {
On 2013/12/09 14:49:28, titzer wrote:
> Not sure why you negated this condition, can you switch it back?

Done.

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

https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:292: static void CallRuntimePassFunctionAndTailCall(
On 2013/12/09 14:49:28, titzer wrote:
> Maybe split this into a CallRuntimeWithFunction and then another method,
> TailCall, since we also tail call in CallCompileOptimizedAndTailCall below...

Done.

https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#...
src/arm/builtins-arm.cc:780: { FrameScope scope(masm, StackFrame::INTERNAL);
On 2013/12/09 14:49:28, titzer wrote:
> This is an almost exact copy of CallRuntimePassFunctionAndTailCall, but with a
> known function id and an extra parameter...

I thought it would make things unnecessarily complicated by making one function
that handles both. I did refactor out the tail call.

https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode349
src/compiler.cc:349: if (!info()->closure()->PassesFilter(FLAG_hydrogen_filter))
{
On 2013/12/09 14:49:28, titzer wrote:
> Do we also want to set a bailout reason and disable optimization here?
> 
> I think we probably always want to pass the bailout reason to
> Abort*Optimization()

Done.

https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode1215
src/compiler.cc:1215: ASSERT(info->shared_info()->scope_info() !=
ScopeInfo::Empty(isolate));
On 2013/12/09 14:49:28, titzer wrote:
> Don't know why this assert is here.

Removed.

https://codereview.chromium.org/110203002/diff/20001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/compiler.h#newcode506
src/compiler.h:506: class RecompileJob: public ZoneObject {
On 2013/12/09 14:49:28, titzer wrote:
> Probably want to rename this class as well. Maybe OptimizedCompileJob?

Done.

https://codereview.chromium.org/110203002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/factory.cc#newcode765
src/factory.cc:765: result->MarkForCompileOptimized();
On 2013/12/09 14:49:28, titzer wrote:
> MarkForCompileOptimized reads weird...why not mark for optimization? 
> 
> Btw, why do we check result->is_compiled() here?

Done. We only optimize if we have unoptimized code.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc#newc...
src/full-codegen.cc:1715: for (uint32_t i = 0; i < back_edges.length(); i++) {
On 2013/12/09 14:49:28, titzer wrote:
> I'm not so sure about the assumption that the AST ids have a unique entry in
the
> backedge table, which appears to be already assumed in the previous code.

AST ids in the back edge table are taken from IterationStatement::OsrEntryId().
It would be weird to have the same OSR entry assigned to two different iteration
statements offsets. The osr_entry_id_ value of the IterationStatement is set in
the constructor via GetNextId().

I can add some assertions to ensure this assumption, if it bothers you.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc#newc...
src/full-codegen.cc:1722: ASSERT(Verify(isolate, code,
code->allow_osr_at_loop_nesting_level()));
On 2013/12/09 14:49:28, titzer wrote:
> Probably not going to work if you have a recursive function, directly or
> indirectly, that is OSR'd.

Verify only tests that the back edge states are consistent with the
allow_osr_at_loop_nesting_level value in that more deeply nested back edges are
not patched and vice versa. RemoveStackCheck does not change this state: both
OSR_AFTER_STACK_CHECK and ON_STACK_REPLACEMENT are considered patched in Verify.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newco...
src/full-codegen.h:934: static void RemoveStackCheck(CompilationInfo* info);
On 2013/12/09 14:49:28, titzer wrote:
> And why does this one take a CompilationInfo? Does it remove all checks, or
just
> one?

This removes only the one added in AddStackCheck. All the necessary information
are in the compilation info, so I though passing that would be cleaner.

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

https://codereview.chromium.org/110203002/diff/20001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:77: static void CallRuntimePassFunctionAndTailCall(
On 2013/12/09 14:49:28, titzer wrote:
> Similar comments to the arm port on naming here.

Done.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9474
src/objects.cc:9474: ASSERT(is_compiled() ||
GetIsolate()->DebuggerHasBreakPoints());
On 2013/12/09 14:49:28, titzer wrote:
> I don't know why we assert is_compiled()

The assumption is that we compiled unoptimized code prior to optimized code,
unless the debugger threw away all code in between.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9487
src/objects.cc:9487: ASSERT(!IsOptimized());
On 2013/12/09 14:49:28, titzer wrote:
> Or this.

To guard that we don't optimize something we already have optimized code for
(that hasn't deopted yet), in case this happens accidentally when we change
something.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9489
src/objects.cc:9489: ASSERT(!shared()->is_generator());
On 2013/12/09 14:49:28, titzer wrote:
> Or that.

Generators are not optimizable, and we should have caught that earlier.

https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9594
src/objects.cc:9594: ASSERT(index > kEntriesStart);
On 2013/12/09 14:49:28, titzer wrote:
> >=?

Each entry of the optimized code map consists of native context, code object,
and literals array, in exactly that order. The first entry of an optimized code
map, at kEntriesStart, is therefore a native context.

The index argument is found via SearchOptimizedCodeMap, which returns the index
for the code object in the entry, which is at least kEntries+1.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8307
src/runtime.cc:8307: PrintF("[lazy: ");
On 2013/12/09 14:49:28, titzer wrote:
> s/lazy/unoptimized/

Done.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8314
src/runtime.cc:8314: ASSERT(!function->is_compiled());
On 2013/12/09 14:49:28, titzer wrote:
> Why?

I guess we would guard that this is only called from the builtin. I think we can
remove this.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8334
src/runtime.cc:8334: if (!function->shared()->is_compiled()) {
On 2013/12/09 14:49:28, titzer wrote:
> I don't think we want this check. IIRC that forces all functions to be first
> compiled unoptimized and then recompiled.

I think that is intentional. I merely inlined AllowOptimization here. I would
like to keep this refactoring from changing the behavior to avoid running into
possible performance regressions. We could experiment with this later.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8356
src/runtime.cc:8356: if (code.is_null()) code =
isolate->builtins()->InOptimizationQueue();
On 2013/12/09 14:49:28, titzer wrote:
> I totally don't understand this. What if the optimized compile just fails
> (without recompilation on)? What is it supposed to return?

You are right. Currently GetOptimizedCode would return the emtpy handle for
successfully queued optimization and the unoptimized shared code on failure (so
that it could be installed, as fallback).

I changed it so that it returns the empty handle on failure and the builtin on
successful queuing.

Also found a bug in case of concurrent OSR here.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8610
src/runtime.cc:8610: Handle<Code> current_code(function->shared()->code());
On 2013/12/09 14:49:28, titzer wrote:
> Let's call this caller_code?

Done.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode12686
src/runtime.cc:12686: static const ParseRestriction restriction =
NO_PARSE_RESTRICTION;
On 2013/12/09 14:49:28, titzer wrote:
> Why the extra local here?

Removed.

https://codereview.chromium.org/110203002/diff/20001/test/cctest/test-compile...
File test/cctest/test-compiler.cc (right):

https://codereview.chromium.org/110203002/diff/20001/test/cctest/test-compile...
test/cctest/test-compiler.cc:108: Handle<String>(), 0, 0,
On 2013/12/09 14:49:28, titzer wrote:
> formatting changes

Done.

[Yang, 2013-12-17 09:54:29]

Precautionary ping. Would be nice to get this in shape before the holidays.

[titzer, 2013-12-17 15:27:54]

lgtm.

There is still something weird about the debugger interaction with the pipeline,
but willing to take it as is, which is a big improvement.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newco...
src/full-codegen.h:934: static void RemoveStackCheck(CompilationInfo* info);
On 2013/12/10 11:22:04, Yang wrote:
> On 2013/12/09 14:49:28, titzer wrote:
> > And why does this one take a CompilationInfo? Does it remove all checks, or
> just
> > one?
> 
> This removes only the one added in AddStackCheck. All the necessary
information
> are in the compilation info, so I though passing that would be cleaner.

I think Add and Remove should be symmetric, and neither should take a
CompilationInfo, which is an indirection to the stuff needed for the operation.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8314
src/runtime.cc:8314: ASSERT(!function->is_compiled());
On 2013/12/10 11:22:04, Yang wrote:
> On 2013/12/09 14:49:28, titzer wrote:
> > Why?
> 
> I guess we would guard that this is only called from the builtin. I think we
can
> remove this.

In general, I think neither CompileOptimized nor CompileUnoptimized should
depend on the prior state of the function. They only establish these invariants
upon exit:

CompileUnoptimized:
  function has code.
  function has unoptimized code if no code existed before.

CompileOptimized:
  function has code.
  function has optimized code if it was already available / generated
successfully.
  if concurrent compilation is on, then there might be a new concurrent
compilation job running in the background now.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8334
src/runtime.cc:8334: if (!function->shared()->is_compiled()) {
On 2013/12/10 11:22:04, Yang wrote:
> On 2013/12/09 14:49:28, titzer wrote:
> > I don't think we want this check. IIRC that forces all functions to be first
> > compiled unoptimized and then recompiled.
> 
> I think that is intentional. I merely inlined AllowOptimization here. I would
> like to keep this refactoring from changing the behavior to avoid running into
> possible performance regressions. We could experiment with this later.

Add a TODO. I still think it's an unnecessary restriction, but it is OK for now.

[Yang, 2013-12-17 16:31:32]

Addressed comments in a new patch set.

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newco...
src/full-codegen.h:934: static void RemoveStackCheck(CompilationInfo* info);
On 2013/12/17 15:27:55, titzer wrote:
> On 2013/12/10 11:22:04, Yang wrote:
> > On 2013/12/09 14:49:28, titzer wrote:
> > > And why does this one take a CompilationInfo? Does it remove all checks,
or
> > just
> > > one?
> > 
> > This removes only the one added in AddStackCheck. All the necessary
> information
> > are in the compilation info, so I though passing that would be cleaner.
> 
> I think Add and Remove should be symmetric, and neither should take a
> CompilationInfo, which is an indirection to the stuff needed for the
operation.

Made the signatures symmetrical.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8314
src/runtime.cc:8314: ASSERT(!function->is_compiled());
On 2013/12/17 15:27:55, titzer wrote:
> On 2013/12/10 11:22:04, Yang wrote:
> > On 2013/12/09 14:49:28, titzer wrote:
> > > Why?
> > 
> > I guess we would guard that this is only called from the builtin. I think we
> can
> > remove this.
> 
> In general, I think neither CompileOptimized nor CompileUnoptimized should
> depend on the prior state of the function. They only establish these
invariants
> upon exit:
> 
> CompileUnoptimized:
>   function has code.
>   function has unoptimized code if no code existed before.
> 
> CompileOptimized:
>   function has code.
>   function has optimized code if it was already available / generated
> successfully.
>   if concurrent compilation is on, then there might be a new concurrent
> compilation job running in the background now.
> 

Added asserts at the end guarding those exit invariants.

https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8334
src/runtime.cc:8334: if (!function->shared()->is_compiled()) {
On 2013/12/17 15:27:55, titzer wrote:
> On 2013/12/10 11:22:04, Yang wrote:
> > On 2013/12/09 14:49:28, titzer wrote:
> > > I don't think we want this check. IIRC that forces all functions to be
first
> > > compiled unoptimized and then recompiled.
> > 
> > I think that is intentional. I merely inlined AllowOptimization here. I
would
> > like to keep this refactoring from changing the behavior to avoid running
into
> > possible performance regressions. We could experiment with this later.
> 
> Add a TODO. I still think it's an unnecessary restriction, but it is OK for
now.

Done.

[Yang, 2013-12-23 14:30:58]

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