Introduce an OptimizingCompiler class, responsible for maintaining the state needed to run Cranksha…

src/compiler.cc

Index: src/compiler.cc
diff --git a/src/compiler.cc b/src/compiler.cc
index 22118ee4d05edae1b104769dbc6f7adf9a9ece31..e24df7255aef57aa6b9da7f761ef6a38ec420b00 100644
--- a/src/compiler.cc
+++ b/src/compiler.cc
@@ -165,64 +165,36 @@ static bool AlwaysFullCompiler(Isolate* isolate) {
 }
 
 
-static void FinishOptimization(Handle<JSFunction> function, int64_t start) {
-  int opt_count = function->shared()->opt_count();
-  function->shared()->set_opt_count(opt_count + 1);
-  double ms = static_cast<double>(OS::Ticks() - start) / 1000;
-  if (FLAG_trace_opt) {
-    PrintF("[optimizing: ");
-    function->PrintName();
-    PrintF(" / %" V8PRIxPTR, reinterpret_cast<intptr_t>(*function));
-    PrintF(" - took %0.3f ms]\n", ms);
-  }
-  if (FLAG_trace_opt_stats) {
-    static double compilation_time = 0.0;
-    static int compiled_functions = 0;
-    static int code_size = 0;
-
-    compilation_time += ms;
-    compiled_functions++;
-    code_size += function->shared()->SourceSize();
-    PrintF("Compiled: %d functions with %d byte source size in %fms.\n",
-           compiled_functions,
-           code_size,
-           compilation_time);
-  }
-}
-
-
-static bool MakeCrankshaftCode(CompilationInfo* info) {
+OptimizingCompiler::Status OptimizingCompiler::InnerCreateHGraph() {
   ASSERT(V8::UseCrankshaft());
-  ASSERT(info->IsOptimizing());
-  ASSERT(!info->IsCompilingForDebugging());
+  ASSERT(info()->IsOptimizing());
+  ASSERT(!info()->IsCompilingForDebugging());
 
-  // We should never arrive here if there is not code object on the
+  // We should never arrive here if there is no code object on the
   // shared function object.
-  Handle<Code> code(info->shared_info()->code());
+  Handle<Code> code(info()->shared_info()->code());
   ASSERT(code->kind() == Code::FUNCTION);
 
   // We should never arrive here if optimization has been disabled on the
   // shared function info.
-  ASSERT(!info->shared_info()->optimization_disabled());
+  ASSERT(!info()->shared_info()->optimization_disabled());
 
   // Fall back to using the full code generator if it's not possible
   // to use the Hydrogen-based optimizing compiler. We already have
   // generated code for this from the shared function object.
-  if (AlwaysFullCompiler(info->isolate())) {
-    info->SetCode(code);
-    return true;
+  if (AlwaysFullCompiler(info()->isolate())) {
+    info()->SetCode(code);
+    return BAILED_OUT;
   }
 
   // Limit the number of times we re-compile a functions with
   // the optimizing compiler.
   const int kMaxOptCount =
       FLAG_deopt_every_n_times == 0 ? Compiler::kDefaultMaxOptCount : 1000;
-  if (info->shared_info()->opt_count() > kMaxOptCount) {
-    info->AbortOptimization();
-    info->shared_info()->DisableOptimization();
-    // True indicates the compilation pipeline is still going, not
-    // necessarily that we optimized the code.
-    return true;
+  if (info()->shared_info()->opt_count() > kMaxOptCount) {
+    info()->AbortOptimization();
+    info()->shared_info()->DisableOptimization();
+    return BAILED_OUT;
   }
 
   // Due to an encoding limit on LUnallocated operands in the Lithium
@@ -234,47 +206,45 @@ static bool MakeCrankshaftCode(CompilationInfo* info) {
   // the negative indices and locals the non-negative ones.
   const int parameter_limit = -LUnallocated::kMinFixedIndex;
   const int locals_limit = LUnallocated::kMaxFixedIndex;
-  Scope* scope = info->scope();
+  Scope* scope = info()->scope();
   if ((scope->num_parameters() + 1) > parameter_limit ||
-      (info->osr_ast_id() != AstNode::kNoNumber &&
+      (info()->osr_ast_id() != AstNode::kNoNumber &&
        scope->num_parameters() + 1 + scope->num_stack_slots() > locals_limit)) {
-    info->AbortOptimization();
-    info->shared_info()->DisableOptimization();
-    // True indicates the compilation pipeline is still going, not
-    // necessarily that we optimized the code.
-    return true;
+    info()->AbortOptimization();
+    info()->shared_info()->DisableOptimization();
+    return BAILED_OUT;
   }
 
   // Take --hydrogen-filter into account.
-  Handle<String> name = info->function()->debug_name();
+  Handle<String> name = info()->function()->debug_name();
   if (*FLAG_hydrogen_filter != '\0') {
     Vector<const char> filter = CStrVector(FLAG_hydrogen_filter);
     if ((filter[0] == '-'
          && name->IsEqualTo(filter.SubVector(1, filter.length())))
         || (filter[0] != '-' && !name->IsEqualTo(filter))) {
-      info->SetCode(code);
-      return true;
+      info()->SetCode(code);
+      return BAILED_OUT;
     }
   }
 
+  Timer t(this);

[danno, 2012/07/16 12:19:09]

nit: Move this down after the comment so diff is clearer.

[sanjoy, 2012/07/16 13:46:46]

Done.

   // Recompile the unoptimized version of the code if the current version
   // doesn't have deoptimization support. Alternatively, we may decide to
   // run the full code generator to get a baseline for the compile-time
   // performance of the hydrogen-based compiler.
-  int64_t start = OS::Ticks();
-  bool should_recompile = !info->shared_info()->has_deoptimization_support();
+  bool should_recompile = !info()->shared_info()->has_deoptimization_support();
   if (should_recompile || FLAG_hydrogen_stats) {
     HPhase phase(HPhase::kFullCodeGen);
-    CompilationInfoWithZone unoptimized(info->shared_info());
+    CompilationInfoWithZone unoptimized(info()->shared_info());
     // Note that we use the same AST that we will use for generating the
     // optimized code.
-    unoptimized.SetFunction(info->function());
-    unoptimized.SetScope(info->scope());
+    unoptimized.SetFunction(info()->function());
+    unoptimized.SetScope(info()->scope());
     if (should_recompile) unoptimized.EnableDeoptimizationSupport();
     bool succeeded = FullCodeGenerator::MakeCode(&unoptimized);
     if (should_recompile) {
-      if (!succeeded) return false;
-      Handle<SharedFunctionInfo> shared = info->shared_info();
+      if (!succeeded) return FAILED;
+      Handle<SharedFunctionInfo> shared = info()->shared_info();
       shared->EnableDeoptimizationSupport(*unoptimized.code());
       // The existing unoptimized code was replaced with the new one.
       Compiler::RecordFunctionCompilation(
@@ -288,52 +258,111 @@ static bool MakeCrankshaftCode(CompilationInfo* info) {
   // is safe as long as the unoptimized code has deoptimization
   // support.
   ASSERT(FLAG_always_opt || code->optimizable());
-  ASSERT(info->shared_info()->has_deoptimization_support());
+  ASSERT(info()->shared_info()->has_deoptimization_support());
 
   if (FLAG_trace_hydrogen) {
     PrintF("-----------------------------------------------------------\n");
     PrintF("Compiling method %s using hydrogen\n", *name->ToCString());
-    HTracer::Instance()->TraceCompilation(info->function());
+    HTracer::Instance()->TraceCompilation(info()->function());
   }
-
-  Handle<Context> global_context(info->closure()->context()->global_context());
-  TypeFeedbackOracle oracle(code, global_context, info->isolate(),
-                            info->zone());
-  HGraphBuilder builder(info, &oracle);
+  Handle<Context> global_context(

[danno, 2012/07/16 12:19:09]

This fits on one line like in the original, no?

[sanjoy, 2012/07/16 13:46:46]

No, because I had to replace `info' with `info()'.

+      info()->closure()->context()->global_context());
+  oracle_ = new(info()->zone()) TypeFeedbackOracle(
+      code, global_context, info()->isolate(), info()->zone());
+  graph_builder_ = new(info()->zone()) HGraphBuilder(info(), oracle_);
   HPhase phase(HPhase::kTotal);
-  HGraph* graph = builder.CreateGraph();
-  if (info->isolate()->has_pending_exception()) {
-    info->SetCode(Handle<Code>::null());
-    return false;
+  graph_ = graph_builder_->CreateGraph();
+  is_inline_bailout_ = graph_builder_->inline_bailout();
+
+  if (info()->isolate()->has_pending_exception()) {
+    info()->SetCode(Handle<Code>::null());
+    return FAILED;
   }
 

[danno, 2012/07/16 12:19:09]

Don't add a line here, please keep original white space as much as possible to
help make diff clearer.

[sanjoy, 2012/07/16 13:46:46]

Done.

[sanjoy, 2012/07/16 13:46:46]

Done.

-  if (graph != NULL) {
-    SmartArrayPointer<char> bailout_reason;
-    if (!graph->Optimize(&bailout_reason)) {
-      if (!bailout_reason.is_empty()) builder.Bailout(*bailout_reason);
-    } else {
-      LChunk* chunk = LChunk::NewChunk(graph);
-      if (chunk != NULL) {
-        Handle<Code> optimized_code = chunk->Codegen();
-        if (!optimized_code.is_null()) {
-          info->SetCode(optimized_code);
-          FinishOptimization(info->closure(), start);
-          return true;
-        }
-      }
-    }
+  if (graph_ == NULL) {
+    AbortCrankshaft();
+    return BAILED_OUT;
+  }
+
+  return SUCCEEDED;
+}
+
+OptimizingCompiler::Status OptimizingCompiler::InnerOptimizeHGraph() {
+  Timer t(this);
+  ASSERT(graph_ != NULL);
+  SmartArrayPointer<char> bailout_reason;
+  if (!graph_->Optimize(&bailout_reason)) {
+    if (!bailout_reason.is_empty()) graph_builder_->Bailout(*bailout_reason);
+    AbortCrankshaft();
+    return BAILED_OUT;

[danno, 2012/07/16 12:19:09]

How about AbortCrankshaft just returns a OptimizingCompiler::Status so you can
write return AbortCrankshaft();

+  } else {
+    chunk_ = LChunk::NewChunk(graph_);
+  }
+  if (chunk_ == NULL) {

[danno, 2012/07/16 12:19:09]

Why will chunk ever be null? It seems like the only path is in the else above,
why not put the test there and always return SUCCEEDED at the end of this
funciton?

[sanjoy, 2012/07/16 13:46:46]

Done.

+    AbortCrankshaft();
+    return BAILED_OUT;
+  } else {
+    return SUCCEEDED;
+  }
+}
+
+
+OptimizingCompiler::Status OptimizingCompiler::InnerGenerateAndInstallCode() {
+  Timer timer(this);
+  ASSERT(chunk_ != NULL);
+  ASSERT(graph_ != NULL);
+  Handle<Code> optimized_code = chunk_->Codegen();
+  if (optimized_code.is_null()) {
+    AbortCrankshaft();
+    return BAILED_OUT;
+  }
+  info()->SetCode(optimized_code);
+  return SUCCEEDED;
+}
+
+
+void OptimizingCompiler::RecordOptimizationStats() {

[danno, 2012/07/16 12:19:09]

Can you please move this back up to where FinishOptimization used to be? Makes
the diffs much simpler.

[sanjoy, 2012/07/16 13:46:46]

Done.

+  Handle<JSFunction> function = info()->closure();
+  int opt_count = function->shared()->opt_count();
+  function->shared()->set_opt_count(opt_count + 1);
+  double ms = static_cast<double>(total_time_taken_) / 1000;
+  if (FLAG_trace_opt) {
+    PrintF("[optimizing: ");
+    function->PrintName();
+    PrintF(" / %" V8PRIxPTR, reinterpret_cast<intptr_t>(*function));
+    PrintF(" - took %0.3f ms]\n", ms);
+  }
+  if (FLAG_trace_opt_stats) {
+    static double compilation_time = 0.0;
+    static int compiled_functions = 0;
+    static int code_size = 0;
+
+    compilation_time += ms;
+    compiled_functions++;
+    code_size += function->shared()->SourceSize();
+    PrintF("Compiled: %d functions with %d byte source size in %fms.\n",
+           compiled_functions,
+           code_size,
+           compilation_time);
+  }
+}
+
+// A return value of true indicates the compilation pipeline is still
+// going, not necessarily that we optimized the code.
+static bool MakeCrankshaftCode(CompilationInfo* info) {

[danno, 2012/07/16 12:19:09]

Can you please move this up where MakeCrankshaftCode used to be? Again, to
minimize the diffs if possible.

[sanjoy, 2012/07/16 13:46:46]

Done.

+  OptimizingCompiler compiler(info);
+  OptimizingCompiler::Status status = compiler.CreateHGraph();
+
+  if (status == OptimizingCompiler::SUCCEEDED) {
+    status = compiler.OptimizeHGraph();
+  } else {
+    return status != OptimizingCompiler::FAILED;
   }
 
-  // Keep using the shared code.
-  info->AbortOptimization();
-  if (!builder.inline_bailout()) {
-    // Mark the shared code as unoptimizable unless it was an inlined
-    // function that bailed out.
-    info->shared_info()->DisableOptimization();
+  if (status == OptimizingCompiler::SUCCEEDED) {
+    status = compiler.GenerateAndInstallCode();
   }
-  // True indicates the compilation pipeline is still going, not necessarily
-  // that we optimized the code.
-  return true;
+  return status != OptimizingCompiler::FAILED;
 }