[parser] Pass LocalIsolate to ParseOnBackground

Unify parse post-processing between main-thread and background-thread
parsing, now that we have LocalIsolate and can Internalize on background
threads.

As part of this, simplify the LocalIsolate parking pattern to explicitly
park during ParseOnBackground, rather than being implicitly parked when
ParseOnBackground is called. This reduces the amound of scoping needed
in the BackgroundCompileTask::Run method.

Change-Id: Ifdb128b763129bda78bd1bae89dac1c62f872350
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3277876
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77872}

src/codegen/compiler.cc

@@ -1494,12 +1494,12 @@ void BackgroundCompileTask::Run() {
   bool toplevel_script_compilation = flags_.is_toplevel();
 
   LocalIsolate isolate(isolate_for_local_isolate_, ThreadKind::kBackground);
+  UnparkedScope unparked_scope(&isolate);
+  LocalHandleScope handle_scope(&isolate);
 
   if (toplevel_script_compilation) {
     DCHECK_NULL(persistent_handles_);
     DCHECK(input_shared_info_.is_null());
-    UnparkedScope unparked_scope(&isolate);
-    LocalHandleScope handle_scope(&isolate);
 
     // We don't have the script source, origin, or details yet, so use default
     // values for them. These will be fixed up during the main-thread merge.
@@ -1510,12 +1510,11 @@ void BackgroundCompileTask::Run() {
   } else {
     DCHECK_NOT_NULL(persistent_handles_);
     isolate.heap()->AttachPersistentHandles(std::move(persistent_handles_));
-    UnparkedScope unparked_scope(&isolate);
-
     Handle<SharedFunctionInfo> shared_info =
         input_shared_info_.ToHandleChecked();
     script_ = isolate.heap()->NewPersistentHandle(
         Script::cast(shared_info->script()));
+    info_->CheckFlagsForFunctionFromScript(*script_);
 
     SharedStringAccessGuardIfNeeded access_guard(&isolate);
     info_->set_function_name(info_->GetOrCreateAstValueFactory()->GetString(
@@ -1531,65 +1530,50 @@ void BackgroundCompileTask::Run() {
   Parser parser(&isolate, info_.get(), script_);
   parser.InitializeEmptyScopeChain(info_.get());
 
-  parser.ParseOnBackground(info_.get(), start_position_, end_position_,
-                           function_literal_id_);
+  parser.ParseOnBackground(&isolate, info_.get(), start_position_,
+                           end_position_, function_literal_id_);
+  parser.UpdateStatistics(script_, use_counts_, &total_preparse_skipped_);
 
   // Save the language mode.
   language_mode_ = info_->language_mode();
 
-  {
-    TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
-                 "V8.CompileCodeBackground");
-    RCS_SCOPE(info_->runtime_call_stats(),
-              RuntimeCallCounterIdForCompileBackground(info_.get()));
-
-    UnparkedScope unparked_scope(&isolate);
-    LocalHandleScope handle_scope(&isolate);
-
-    info_->ast_value_factory()->Internalize(&isolate);
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
+               "V8.CompileCodeBackground");
+  RCS_SCOPE(info_->runtime_call_stats(),
+            RuntimeCallCounterIdForCompileBackground(info_.get()));
 
+  MaybeHandle<SharedFunctionInfo> maybe_result;
+  if (info_->literal() != nullptr) {
+    Handle<SharedFunctionInfo> shared_info;
     if (toplevel_script_compilation) {
-      // The script is new, so we can mutate it here.
-      parser.HandleSourceURLComments(&isolate, script_);
+      shared_info =
+          CreateTopLevelSharedFunctionInfo(info_.get(), script_, &isolate);
     } else {
-      info_->CheckFlagsForFunctionFromScript(*script_);
-    }
-
-    MaybeHandle<SharedFunctionInfo> maybe_result;
-    if (info_->literal() != nullptr) {
-      Handle<SharedFunctionInfo> shared_info;
-      if (toplevel_script_compilation) {
-        shared_info =
-            CreateTopLevelSharedFunctionInfo(info_.get(), script_, &isolate);
-      } else {
-        // Clone into a placeholder SFI for storing the results.
-        shared_info = isolate.factory()->CloneSharedFunctionInfo(
-            input_shared_info_.ToHandleChecked());
-      }
-
-      if (IterativelyExecuteAndFinalizeUnoptimizedCompilationJobs(
-              &isolate, shared_info, script_, info_.get(),
-              compile_state_.allocator(), &is_compiled_scope_,
-              &finalize_unoptimized_compilation_data_,
-              &jobs_to_retry_finalization_on_main_thread_)) {
-        maybe_result = shared_info;
-      }
+      // Clone into a placeholder SFI for storing the results.
+      shared_info = isolate.factory()->CloneSharedFunctionInfo(
+          input_shared_info_.ToHandleChecked());
     }
 
-    if (maybe_result.is_null()) {
-      PreparePendingException(&isolate, info_.get());
+    if (IterativelyExecuteAndFinalizeUnoptimizedCompilationJobs(
+            &isolate, shared_info, script_, info_.get(),
+            compile_state_.allocator(), &is_compiled_scope_,
+            &finalize_unoptimized_compilation_data_,
+            &jobs_to_retry_finalization_on_main_thread_)) {
+      maybe_result = shared_info;
     }
+  }
 
-    outer_function_sfi_ =
-        isolate.heap()->NewPersistentMaybeHandle(maybe_result);
-    script_ = isolate.heap()->NewPersistentHandle(script_);
-
-    parser.UpdateStatistics(script_, use_counts_, &total_preparse_skipped_);
+  if (maybe_result.is_null()) {
+    PreparePendingException(&isolate, info_.get());
   }
 
+  outer_function_sfi_ = isolate.heap()->NewPersistentMaybeHandle(maybe_result);
+  DCHECK(isolate.heap()->ContainsPersistentHandle(script_.location()));
+  persistent_handles_ = isolate.heap()->DetachPersistentHandles();
+
+  // Make sure the language mode didn't change.
   DCHECK_EQ(language_mode_, info_->language_mode());
   info_.reset();
-  persistent_handles_ = isolate.heap()->DetachPersistentHandles();
 }
 
 MaybeHandle<SharedFunctionInfo> BackgroundCompileTask::FinalizeScript(

src/parsing/parser.cc

@@ -18,6 +18,7 @@
 #include "src/common/globals.h"
 #include "src/common/message-template.h"
 #include "src/compiler-dispatcher/lazy-compile-dispatcher.h"
+#include "src/heap/parked-scope.h"
 #include "src/logging/counters.h"
 #include "src/logging/log.h"
 #include "src/logging/runtime-call-stats-scope.h"
@@ -689,7 +690,8 @@ FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
   return result;
 }
 
-void Parser::PostProcessParseResult(Isolate* isolate, ParseInfo* info,
+template <typename IsolateT>
+void Parser::PostProcessParseResult(IsolateT* isolate, ParseInfo* info,
                                     FunctionLiteral* literal) {
   if (literal == nullptr) return;
 
@@ -699,10 +701,7 @@ void Parser::PostProcessParseResult(Isolate* isolate, ParseInfo* info,
     info->set_allow_eval_cache(allow_eval_cache());
   }
 
-  // We cannot internalize on a background thread; a foreground task will take
-  // care of calling AstValueFactory::Internalize just before compilation.
-  DCHECK_EQ(isolate != nullptr, parsing_on_main_thread_);
-  if (isolate) info->ast_value_factory()->Internalize(isolate);
+  info->ast_value_factory()->Internalize(isolate);
 
   {
     RCS_SCOPE(info->runtime_call_stats(), RuntimeCallCounterId::kCompileAnalyse,
@@ -715,6 +714,12 @@ void Parser::PostProcessParseResult(Isolate* isolate, ParseInfo* info,
   }
 }
 
+template void Parser::PostProcessParseResult(Isolate* isolate, ParseInfo* info,
+                                             FunctionLiteral* literal);
+template void Parser::PostProcessParseResult(LocalIsolate* isolate,
+                                             ParseInfo* info,
+                                             FunctionLiteral* literal);
+
 ZonePtrList<const AstRawString>* Parser::PrepareWrappedArguments(
     Isolate* isolate, ParseInfo* info, Zone* zone) {
   DCHECK(parsing_on_main_thread_);
@@ -3292,37 +3297,48 @@ void Parser::UpdateStatistics(Handle<Script> script, int* use_counts,
   *preparse_skipped = total_preparse_skipped_;
 }
 
-void Parser::ParseOnBackground(ParseInfo* info, int start_position,
-                               int end_position, int function_literal_id) {
+void Parser::ParseOnBackground(LocalIsolate* isolate, ParseInfo* info,
+                               int start_position, int end_position,
+                               int function_literal_id) {
   RCS_SCOPE(runtime_call_stats_, RuntimeCallCounterId::kParseBackgroundProgram);
   parsing_on_main_thread_ = false;
 
   DCHECK_NULL(info->literal());
   FunctionLiteral* result = nullptr;
-
-  scanner_.Initialize();
-
-  DCHECK(original_scope_);
-
-  // When streaming, we don't know the length of the source until we have parsed
-  // it. The raw data can be UTF-8, so we wouldn't know the source length until
-  // we have decoded it anyway even if we knew the raw data length (which we
-  // don't). We work around this by storing all the scopes which need their end
-  // position set at the end of the script (the top scope and possible eval
-  // scopes) and set their end position after we know the script length.
+  {
+    // We can park the isolate while parsing, it doesn't need to allocate or
+    // access the main thread.
+    ParkedScope parked_scope(isolate);
+
+    scanner_.Initialize();
+
+    DCHECK(original_scope_);
+
+    // When streaming, we don't know the length of the source until we have
+    // parsed it. The raw data can be UTF-8, so we wouldn't know the source
+    // length until we have decoded it anyway even if we knew the raw data
+    // length (which we don't). We work around this by storing all the scopes
+    // which need their end position set at the end of the script (the top scope
+    // and possible eval scopes) and set their end position after we know the
+    // script length.
+    if (flags().is_toplevel()) {
+      DCHECK_EQ(start_position, 0);
+      DCHECK_EQ(end_position, 0);
+      DCHECK_EQ(function_literal_id, kFunctionLiteralIdTopLevel);
+      result = DoParseProgram(/* isolate = */ nullptr, info);
+    } else {
+      result = DoParseFunction(/* isolate = */ nullptr, info, start_position,
+                               end_position, function_literal_id,
+                               info->function_name());
+    }
+    MaybeResetCharacterStream(info, result);
+    MaybeProcessSourceRanges(info, result, stack_limit_);
+  }
+  // We need to unpark by now though, to be able to internalize.
+  PostProcessParseResult(isolate, info, result);
   if (flags().is_toplevel()) {
-    DCHECK_EQ(start_position, 0);
-    DCHECK_EQ(end_position, 0);
-    DCHECK_EQ(function_literal_id, kFunctionLiteralIdTopLevel);
-    result = DoParseProgram(/* isolate = */ nullptr, info);
-  } else {
-    result = DoParseFunction(/* isolate = */ nullptr, info, start_position,
-                             end_position, function_literal_id,
-                             info->function_name());
+    HandleSourceURLComments(isolate, script_);
   }
-  MaybeResetCharacterStream(info, result);
-  MaybeProcessSourceRanges(info, result, stack_limit_);
-  PostProcessParseResult(/* isolate = */ nullptr, info, result);
 }
 
 Parser::TemplateLiteralState Parser::OpenTemplateLiteral(int pos) {

src/parsing/parser.h

@@ -139,7 +139,8 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   static bool IsPreParser() { return false; }
 
   // Sets the literal on |info| if parsing succeeded.
-  void ParseOnBackground(ParseInfo* info, int start_position, int end_position,
+  void ParseOnBackground(LocalIsolate* isolate, ParseInfo* info,
+                         int start_position, int end_position,
                          int function_literal_id);
 
   // Initializes an empty scope chain for top-level scripts, or scopes which
@@ -223,7 +224,8 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   void ParseFunction(Isolate* isolate, ParseInfo* info,
                      Handle<SharedFunctionInfo> shared_info);
 
-  void PostProcessParseResult(Isolate* isolate, ParseInfo* info,
+  template <typename IsolateT>
+  void PostProcessParseResult(IsolateT* isolate, ParseInfo* info,
                               FunctionLiteral* literal);
 
   FunctionLiteral* DoParseFunction(Isolate* isolate, ParseInfo* info,

src/parsing/parsing.cc

@@ -23,9 +23,9 @@ namespace parsing {
 
 namespace {
 
-void MaybeReportErrorsAndStatistics(ParseInfo* info, Handle<Script> script,
-                                    Isolate* isolate, Parser* parser,
-                                    ReportStatisticsMode mode) {
+void MaybeReportStatistics(ParseInfo* info, Handle<Script> script,
+                           Isolate* isolate, Parser* parser,
+                           ReportStatisticsMode mode) {
   switch (mode) {
     case ReportStatisticsMode::kYes:
       parser->UpdateStatistics(isolate, script);
@@ -57,7 +57,7 @@ bool ParseProgram(ParseInfo* info, Handle<Script> script,
   // Ok to use Isolate here; this function is only called in the main thread.
   DCHECK(parser.parsing_on_main_thread_);
   parser.ParseProgram(isolate, script, info, maybe_outer_scope_info);
-  MaybeReportErrorsAndStatistics(info, script, isolate, &parser, mode);
+  MaybeReportStatistics(info, script, isolate, &parser, mode);
   return info->literal() != nullptr;
 }
 
@@ -88,7 +88,7 @@ bool ParseFunction(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
   // Ok to use Isolate here; this function is only called in the main thread.
   DCHECK(parser.parsing_on_main_thread_);
   parser.ParseFunction(isolate, info, shared_info);
-  MaybeReportErrorsAndStatistics(info, script, isolate, &parser, mode);
+  MaybeReportStatistics(info, script, isolate, &parser, mode);
   return info->literal() != nullptr;
 }
 

test/unittests/heap/local-factory-unittest.cc

@@ -80,17 +80,11 @@ class LocalFactoryTest : public TestWithIsolateAndZone {
     parse_info_.set_character_stream(
         ScannerStream::ForTesting(utf16_source.data(), utf16_source.size()));
 
-    {
-      DisallowGarbageCollection no_gc;
-      DisallowHeapAccess no_heap_access;
+    Parser parser(local_isolate(), parse_info(), script_);
+    parser.InitializeEmptyScopeChain(parse_info());
+    parser.ParseOnBackground(local_isolate(), parse_info(), 0, 0,
+                             kFunctionLiteralIdTopLevel);
 
-      Parser parser(isolate()->main_thread_local_isolate(), parse_info(),
-                    script_);
-      parser.InitializeEmptyScopeChain(parse_info());
-      parser.ParseOnBackground(parse_info(), 0, 0, kFunctionLiteralIdTopLevel);
-    }
-
-    parse_info()->ast_value_factory()->Internalize(local_isolate());
     DeclarationScope::AllocateScopeInfos(parse_info(), local_isolate());
 
     // Create the SFI list on the script so that SFI SetScript works.