[compiler] Collect eager inner functions for compilation during renumbering.

src/compiler.cc

Index: src/compiler.cc
diff --git a/src/compiler.cc b/src/compiler.cc
index ee89ba8fe869c582cc808a61050052a3277fdb26..6f6f9bf9d2d96ec6400cff2acd5ad702f23666a3 100644
--- a/src/compiler.cc
+++ b/src/compiler.cc
@@ -38,8 +38,6 @@
 namespace v8 {
 namespace internal {
 
-
-
 // A wrapper around a CompilationInfo that detaches the Handles from
 // the underlying DeferredHandleScope and stores them in info_ on
 // destruction.
@@ -377,8 +375,7 @@ bool ShouldUseIgnition(CompilationInfo* info) {
   return shared->PassesFilter(FLAG_ignition_filter);
 }
 
-CompilationJob* GetUnoptimizedCompilationJob(CompilationInfo* info,
-                                             LazyCompilationMode mode) {
+CompilationJob* GetUnoptimizedCompilationJob(CompilationInfo* info) {
   // Function should have been parsed and analyzed before creating a compilation
   // job.
   DCHECK_NOT_NULL(info->literal());
@@ -386,9 +383,9 @@ CompilationJob* GetUnoptimizedCompilationJob(CompilationInfo* info,
 
   EnsureFeedbackMetadata(info);
   if (ShouldUseIgnition(info)) {
-    return interpreter::Interpreter::NewCompilationJob(info, mode);
+    return interpreter::Interpreter::NewCompilationJob(info);
   } else {
-    return FullCodeGenerator::NewCompilationJob(info, mode);
+    return FullCodeGenerator::NewCompilationJob(info);
   }
 }
 
@@ -437,6 +434,26 @@ CompilationJob::Status FinalizeUnoptimizedCompilationJob(CompilationJob* job) {
   return status;
 }
 
+bool Renumber(ParseInfo* parse_info) {
+  if (!AstNumbering::Renumber(parse_info->isolate(), parse_info->zone(),
+                              parse_info->literal(),
+                              parse_info->eager_inner_function_literals())) {
+    return false;
+  }
+  Handle<SharedFunctionInfo> shared_info = parse_info->shared_info();
+  if (!shared_info.is_null()) {
+    FunctionLiteral* lit = parse_info->literal();
+    shared_info->set_ast_node_count(lit->ast_node_count());
+    if (lit->dont_optimize_reason() != kNoReason) {
+      shared_info->DisableOptimization(lit->dont_optimize_reason());
+    }
+    if (lit->flags() & AstProperties::kMustUseIgnitionTurbo) {
+      shared_info->set_must_use_ignition_turbo(true);
+    }
+  }
+  return true;
+}
+
 bool GenerateUnoptimizedCode(CompilationInfo* info) {
   if (FLAG_validate_asm && info->scope()->asm_module() &&
       !info->shared_info()->is_asm_wasm_broken() && !info->is_debug()) {
@@ -451,8 +468,7 @@ bool GenerateUnoptimizedCode(CompilationInfo* info) {
     }
   }
 
-  std::unique_ptr<CompilationJob> job(
-      GetUnoptimizedCompilationJob(info, LazyCompilationMode::kIfRequested));
+  std::unique_ptr<CompilationJob> job(GetUnoptimizedCompilationJob(info));
   if (job->PrepareJob() != CompilationJob::SUCCEEDED) return false;
   if (job->ExecuteJob() != CompilationJob::SUCCEEDED) return false;
   if (FinalizeUnoptimizedCompilationJob(job.get()) !=
@@ -462,14 +478,72 @@ bool GenerateUnoptimizedCode(CompilationInfo* info) {
   return true;
 }
 
+bool CompileUnoptimizedInnerFunctionsRecursively(
+    ZoneVector<FunctionLiteral*>* literals, CompilationInfo* outer_info) {
+  Isolate* isolate = outer_info->isolate();
+  Handle<Script> script = outer_info->script();
+
+  for (auto it = literals->begin(); it != literals->end(); it++) {
+    FunctionLiteral* literal = *it;
+
+    // Find any previously allocated shared function info for the given literal.
+    Handle<SharedFunctionInfo> shared;
+    MaybeHandle<SharedFunctionInfo> maybe_existing =
+        script->FindSharedFunctionInfo(isolate, literal);
+    if (maybe_existing.ToHandle(&shared)) {
+      DCHECK(!shared->is_toplevel());
+      // If we found an existing shared function info with compiled code,
+      // we are done.
+      if (shared->is_compiled()) continue;
+    } else {
+      shared =
+          isolate->factory()->NewSharedFunctionInfoForLiteral(literal, script);
+      shared->set_is_toplevel(false);
+    }
+
+    Zone zone(isolate->allocator(), ZONE_NAME);
+    ParseInfo parse_info(&zone, script);

[marja, 2017/01/05 19:25:22]

It's weird | surprising that we need to create a new ParseInfo here even if we
don't parse... it's because the renumbering needs one as input, right?

[rmcilroy, 2017/01/06 17:08:14]

Yes I agree this is a bit weird (it is a copy of the code which used to be in
GetSharedFunctionInfo and did the same thing). Yes it's because Renumber needs a
parse_info, and also because the compiler also gets some stuff from parse_info
indirectly via accessors on compile_info (e.g., the literal, script, scope,
shared_info, etc.). 

It would be really nice to disentangle parse_info and compilation_info so that
they don't refer to each other then we could maybe get away without this.

+    parse_info.set_literal(literal);
+    parse_info.set_shared_info(shared);
+    parse_info.set_function_literal_id(shared->function_literal_id());
+    parse_info.set_language_mode(literal->scope()->language_mode());
+    parse_info.set_ast_value_factory(
+        outer_info->parse_info()->ast_value_factory());
+    parse_info.set_ast_value_factory_owned(false);
+
+    CompilationInfo info(&parse_info, Handle<JSFunction>::null());
+    if (outer_info->will_serialize()) info.PrepareForSerializing();
+    if (outer_info->is_debug()) info.MarkAsDebug();
+
+    if (!Renumber(&parse_info) ||
+        !CompileUnoptimizedInnerFunctionsRecursively(
+            parse_info.eager_inner_function_literals(), &info) ||
+        !GenerateUnoptimizedCode(&info)) {
+      if (!isolate->has_pending_exception()) isolate->StackOverflow();
+      return false;
+    }
+
+    DCHECK(!info.code().is_null());
+    RecordFunctionCompilation(CodeEventListener::FUNCTION_TAG, &info);
+    if (literal->should_be_used_once_hint()) {
+      info.code()->MarkToBeExecutedOnce(isolate);
+    }
+  }
+  return true;
+}
+
 bool CompileUnoptimizedCode(CompilationInfo* info) {
-  DCHECK(AllowCompilation::IsAllowed(info->isolate()));
+  Isolate* isolate = info->isolate();
+  DCHECK(AllowCompilation::IsAllowed(isolate));
+
   if (!Compiler::Analyze(info->parse_info()) ||
+      !CompileUnoptimizedInnerFunctionsRecursively(
+          info->parse_info()->eager_inner_function_literals(), info) ||
       !GenerateUnoptimizedCode(info)) {
-    Isolate* isolate = info->isolate();
     if (!isolate->has_pending_exception()) isolate->StackOverflow();
     return false;
   }
+
   return true;
 }
 
@@ -552,25 +626,6 @@ void InsertCodeIntoOptimizedCodeMap(CompilationInfo* info) {
                                             literals, info->osr_ast_id());
 }
 
-bool Renumber(ParseInfo* parse_info) {
-  if (!AstNumbering::Renumber(parse_info->isolate(), parse_info->zone(),
-                              parse_info->literal())) {
-    return false;
-  }
-  Handle<SharedFunctionInfo> shared_info = parse_info->shared_info();
-  if (!shared_info.is_null()) {
-    FunctionLiteral* lit = parse_info->literal();
-    shared_info->set_ast_node_count(lit->ast_node_count());
-    if (lit->dont_optimize_reason() != kNoReason) {
-      shared_info->DisableOptimization(lit->dont_optimize_reason());
-    }
-    if (lit->flags() & AstProperties::kMustUseIgnitionTurbo) {
-      shared_info->set_must_use_ignition_turbo(true);
-    }
-  }
-  return true;
-}
-
 bool GetOptimizedCodeNow(CompilationJob* job) {
   CompilationInfo* info = job->info();
   Isolate* isolate = info->isolate();
@@ -1008,8 +1063,6 @@ Handle<SharedFunctionInfo> CompileToplevel(CompilationInfo* info) {
 
     EnsureSharedFunctionInfosArrayOnScript(parse_info);
 
-    FunctionLiteral* lit = parse_info->literal();
-
     // Measure how long it takes to do the compilation; only take the
     // rest of the function into account to avoid overlap with the
     // parsing statistics.
@@ -1024,6 +1077,7 @@ Handle<SharedFunctionInfo> CompileToplevel(CompilationInfo* info) {
                  parse_info->is_eval() ? "V8.CompileEval" : "V8.Compile");
 
     // Allocate a shared function info object.
+    FunctionLiteral* lit = parse_info->literal();
     DCHECK_EQ(kNoSourcePosition, lit->function_token_position());
     result = isolate->factory()->NewSharedFunctionInfoForLiteral(lit, script);
     result->set_is_toplevel(true);
@@ -1587,7 +1641,7 @@ Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfoForStreamedScript(
 
 Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfo(
     FunctionLiteral* literal, Handle<Script> script,
-    CompilationInfo* outer_info, LazyCompilationMode mode) {
+    CompilationInfo* outer_info) {
   // Precondition: code has been parsed and scopes have been analyzed.
   Isolate* isolate = outer_info->isolate();
   MaybeHandle<SharedFunctionInfo> maybe_existing;
@@ -1595,80 +1649,21 @@ Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfo(
   // Find any previously allocated shared function info for the given literal.
   maybe_existing = script->FindSharedFunctionInfo(isolate, literal);
 
-  // We found an existing shared function info. If it has any sort of code
-  // attached, don't worry about compiling and simply return it. Otherwise,
-  // continue to decide whether to eagerly compile.
-  // Note that we also carry on if we are compiling eager to obtain code for
-  // debugging, unless we already have code with debug break slots.
+  // If we found an existing shared function info, return it.
   Handle<SharedFunctionInfo> existing;
   if (maybe_existing.ToHandle(&existing)) {
     DCHECK(!existing->is_toplevel());
-    if (existing->HasBaselineCode() || existing->HasBytecodeArray()) {
-      if (!outer_info->is_debug() || existing->HasDebugCode()) {
-        return existing;
-      }
-    }
-  }
-
-  // Allocate a shared function info object.
-  Handle<SharedFunctionInfo> result;
-  if (!maybe_existing.ToHandle(&result)) {
-    result =
-        isolate->factory()->NewSharedFunctionInfoForLiteral(literal, script);
-    result->set_is_toplevel(false);
-  }
-
-  Zone zone(isolate->allocator(), ZONE_NAME);
-  ParseInfo parse_info(&zone, script);
-  CompilationInfo info(&parse_info, Handle<JSFunction>::null());
-  parse_info.set_literal(literal);
-  parse_info.set_shared_info(result);
-  parse_info.set_function_literal_id(result->function_literal_id());
-  parse_info.set_language_mode(literal->scope()->language_mode());
-  parse_info.set_ast_value_factory(
-      outer_info->parse_info()->ast_value_factory());
-  parse_info.set_ast_value_factory_owned(false);
-
-  if (outer_info->will_serialize()) info.PrepareForSerializing();
-  if (outer_info->is_debug()) info.MarkAsDebug();
-
-  // If this inner function is already compiled, we don't need to compile
-  // again. When compiling for debug, we are not interested in having debug
-  // break slots in inner functions, neither for setting break points nor
-  // for revealing inner functions.
-  // This is especially important for generators. We must not replace the
-  // code for generators, as there may be suspended generator objects.
-  if (!result->is_compiled()) {
-    if (mode == LazyCompilationMode::kAlways ||
-        !literal->ShouldEagerCompile()) {
-      info.SetCode(isolate->builtins()->CompileLazy());
-      Scope* outer_scope = literal->scope()->GetOuterScopeWithContext();
-      if (outer_scope) {
-        result->set_outer_scope_info(*outer_scope->scope_info());
-      }
-    } else {
-      // Generate code
-      TimerEventScope<TimerEventCompileCode> timer(isolate);
-      RuntimeCallTimerScope runtimeTimer(isolate,
-                                         &RuntimeCallStats::CompileCode);
-      TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"), "V8.CompileCode");
-      if (Renumber(info.parse_info()) && GenerateUnoptimizedCode(&info)) {
-        // Code generation will ensure that the feedback vector is present and
-        // appropriately sized.
-        DCHECK(!info.code().is_null());
-        if (literal->should_be_used_once_hint()) {
-          info.code()->MarkToBeExecutedOnce(isolate);
-        }
-      } else {
-        return Handle<SharedFunctionInfo>::null();
-      }
-    }
+    return existing;

[jochen (gone - plz use gerrit), 2017/01/05 15:53:12]

this might be wrong if we want debug code, but the existing SFI has only
baseline code

[rmcilroy, 2017/01/05 16:02:42]

The debug code logic didn't do anything any longer - there is a comment below in
the old code saying "When compiling for debug, we are not interested in having
debug break slots in inner functions" and the compilation logic was wrapped in a
if (!result->is_compiled()) so even though we fell through here, we were never
actually recompiling the inner function for debug code anyway.

   }
 
-  if (maybe_existing.is_null()) {
-    RecordFunctionCompilation(CodeEventListener::FUNCTION_TAG, &info);
+  // Allocate a shared function info object which will be compiled lazily.
+  Handle<SharedFunctionInfo> result =
+      isolate->factory()->NewSharedFunctionInfoForLiteral(literal, script);

[jochen (gone - plz use gerrit), 2017/01/05 15:53:12]

why don't you need to put the compile lazy builtin here?

[rmcilroy, 2017/01/05 16:02:42]

I was confused by this too, but NewSharedFunctionInfoForLiteral already does it
(the old code only added the lazy builtin to the ComilationInfo (not the new
SFI), presumably so that RecordFunctionCompilation "worked").

+  result->set_is_toplevel(false);
+  Scope* outer_scope = literal->scope()->GetOuterScopeWithContext();
+  if (outer_scope) {
+    result->set_outer_scope_info(*outer_scope->scope_info());
   }
-
   return result;
 }
 
@@ -1713,9 +1708,9 @@ MaybeHandle<Code> Compiler::GetOptimizedCodeForOSR(Handle<JSFunction> function,
 }
 
 CompilationJob* Compiler::PrepareUnoptimizedCompilationJob(
-    CompilationInfo* info, LazyCompilationMode mode) {
+    CompilationInfo* info) {
   VMState<COMPILER> state(info->isolate());
-  std::unique_ptr<CompilationJob> job(GetUnoptimizedCompilationJob(info, mode));
+  std::unique_ptr<CompilationJob> job(GetUnoptimizedCompilationJob(info));
   if (job->PrepareJob() != CompilationJob::SUCCEEDED) {
     return nullptr;
   }