[codegen, heap] Improve TimeToSafepoint for concurrent compilation

TimeToSafepoint is the time needed for all background threads to enter
a safepoint after the GC was started on the main thread. This CL
improves that metric during concurrent compilation to bytecode by doing:

1) Park the LocalIsolate during
   InterpreterCompilationJob::ExecuteJobImpl. There are no concurrent
   heap accesses happening while generating bytecode for now. So instead
   of manually placing Safepoint() invocations in the code, simply park
   the local isolate.
2) Destroy the LocalIsolate before the ReleaseParser operation. I've
   seen this take around 2ms, which regressed TimeToSafepoint a lot.
3) Add explicit safepoints to concurrent allocations. This covers the
   rest of the code and from what I've seen so far this is good enough
   to keep TimeToSafepoint around a few microseconds.

I've still seen TimeToSafepoint events with 20-80 microseconds but those
were quite rare and always seemed to be related to Turbofan.

AsLocalIsolate() is necessary in generic code to convert both Isolate
and LocalIsolate to LocalIsolate.

Bug: v8:10315
Change-Id: Idaf9f04ffdf850d0ab0081ec372cc384a9fe7ef9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2663159Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72618}

src/codegen/compiler.cc

@@ -30,6 +30,7 @@
 #include "src/execution/frames-inl.h"
 #include "src/execution/isolate-inl.h"
 #include "src/execution/isolate.h"
+#include "src/execution/local-isolate.h"
 #include "src/execution/runtime-profiler.h"
 #include "src/execution/vm-state-inl.h"
 #include "src/handles/maybe-handles.h"
@@ -619,7 +620,8 @@ std::unique_ptr<UnoptimizedCompilationJob>
 ExecuteSingleUnoptimizedCompilationJob(
     ParseInfo* parse_info, FunctionLiteral* literal,
     AccountingAllocator* allocator,
-    std::vector<FunctionLiteral*>* eager_inner_literals) {
+    std::vector<FunctionLiteral*>* eager_inner_literals,
+    LocalIsolate* local_isolate) {
   if (UseAsmWasm(literal, parse_info->flags().is_asm_wasm_broken())) {
     std::unique_ptr<UnoptimizedCompilationJob> asm_job(
         AsmJs::NewCompilationJob(parse_info, literal, allocator));
@@ -634,7 +636,7 @@ ExecuteSingleUnoptimizedCompilationJob(
   }
   std::unique_ptr<UnoptimizedCompilationJob> job(
       interpreter::Interpreter::NewCompilationJob(
-          parse_info, literal, allocator, eager_inner_literals));
+          parse_info, literal, allocator, eager_inner_literals, local_isolate));
 
   if (job->ExecuteJob() != CompilationJob::SUCCEEDED) {
     // Compilation failed, return null.
@@ -649,9 +651,13 @@ bool RecursivelyExecuteUnoptimizedCompilationJobs(
     AccountingAllocator* allocator,
     UnoptimizedCompilationJobList* function_jobs) {
   std::vector<FunctionLiteral*> eager_inner_literals;
+
+  // We need to pass nullptr here because we are on the background
+  // thread but don't have a LocalIsolate.
+  DCHECK_NULL(LocalHeap::Current());
   std::unique_ptr<UnoptimizedCompilationJob> job =
       ExecuteSingleUnoptimizedCompilationJob(parse_info, literal, allocator,
-                                             &eager_inner_literals);
+                                             &eager_inner_literals, nullptr);
 
   if (!job) return false;
 
@@ -690,7 +696,9 @@ bool IterativelyExecuteAndFinalizeUnoptimizedCompilationJobs(
 
     std::unique_ptr<UnoptimizedCompilationJob> job =
         ExecuteSingleUnoptimizedCompilationJob(parse_info, literal, allocator,
-                                               &functions_to_compile);
+                                               &functions_to_compile,
+                                               isolate->AsLocalIsolate());
+
     if (!job) return false;
 
     UpdateSharedFunctionFlagsAfterCompilation(literal, *shared_info);
@@ -716,8 +724,8 @@ bool IterativelyExecuteAndFinalizeUnoptimizedCompilationJobs(
         DCHECK((!std::is_same<LocalIsolate, Isolate>::value));
         DCHECK_NOT_NULL(jobs_to_retry_finalization_on_main_thread);
 
-        // Clear the literal and ParseInfo to prevent further attempts to access
-        // them.
+        // Clear the literal and ParseInfo to prevent further attempts to
+        // access them.
         job->compilation_info()->ClearLiteral();
         job->ClearParseInfo();
         jobs_to_retry_finalization_on_main_thread->emplace_back(
@@ -1553,37 +1561,39 @@ void BackgroundCompileTask::Run() {
   } else {
     DCHECK(info_->flags().is_toplevel());
 
-    LocalIsolate isolate(isolate_for_local_isolate_, ThreadKind::kBackground);
-    UnparkedScope unparked_scope(isolate.heap());
-    LocalHandleScope handle_scope(&isolate);
-
-    info_->ast_value_factory()->Internalize(&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.
-    Handle<Script> script = info_->CreateScript(
-        &isolate, isolate.factory()->empty_string(), kNullMaybeHandle,
-        ScriptOriginOptions(false, false, false, info_->flags().is_module()));
+    {
+      LocalIsolate isolate(isolate_for_local_isolate_, ThreadKind::kBackground);
+      UnparkedScope unparked_scope(&isolate);
+      LocalHandleScope handle_scope(&isolate);
+
+      info_->ast_value_factory()->Internalize(&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.
+      Handle<Script> script = info_->CreateScript(
+          &isolate, isolate.factory()->empty_string(), kNullMaybeHandle,
+          ScriptOriginOptions(false, false, false, info_->flags().is_module()));
+
+      parser_->HandleSourceURLComments(&isolate, script);
+
+      MaybeHandle<SharedFunctionInfo> maybe_result;
+      if (info_->literal() != nullptr) {
+        maybe_result = CompileAndFinalizeOnBackgroundThread(
+            info_.get(), compile_state_.allocator(), script, &isolate,
+            &finalize_unoptimized_compilation_data_,
+            &jobs_to_retry_finalization_on_main_thread_, &is_compiled_scope_);
+      } else {
+        DCHECK(compile_state_.pending_error_handler()->has_pending_error());
+        PreparePendingException(&isolate, info_.get());
+      }
 
-    parser_->HandleSourceURLComments(&isolate, script);
+      outer_function_sfi_ =
+          isolate.heap()->NewPersistentMaybeHandle(maybe_result);
+      script_ = isolate.heap()->NewPersistentHandle(script);
 
-    MaybeHandle<SharedFunctionInfo> maybe_result;
-    if (info_->literal() != nullptr) {
-      maybe_result = CompileAndFinalizeOnBackgroundThread(
-          info_.get(), compile_state_.allocator(), script, &isolate,
-          &finalize_unoptimized_compilation_data_,
-          &jobs_to_retry_finalization_on_main_thread_, &is_compiled_scope_);
-    } else {
-      DCHECK(compile_state_.pending_error_handler()->has_pending_error());
-      PreparePendingException(&isolate, info_.get());
+      persistent_handles_ = isolate.heap()->DetachPersistentHandles();
     }
 
-    outer_function_sfi_ =
-        isolate.heap()->NewPersistentMaybeHandle(maybe_result);
-    script_ = isolate.heap()->NewPersistentHandle(script);
-
-    persistent_handles_ = isolate.heap()->DetachPersistentHandles();
-
     {
       TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
                    "V8.FinalizeCodeBackground.ReleaseParser");
@@ -1683,7 +1693,8 @@ bool Compiler::CollectSourcePositions(Isolate* isolate,
   std::unique_ptr<UnoptimizedCompilationJob> job;
   {
     job = interpreter::Interpreter::NewSourcePositionCollectionJob(
-        &parse_info, parse_info.literal(), bytecode, isolate->allocator());
+        &parse_info, parse_info.literal(), bytecode, isolate->allocator(),
+        isolate->main_thread_local_isolate());
 
     if (!job || job->ExecuteJob() != CompilationJob::SUCCEEDED ||
         job->FinalizeJob(shared_info, isolate) != CompilationJob::SUCCEEDED) {

src/execution/isolate.h

@@ -1659,6 +1659,8 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
     return main_thread_local_isolate_.get();
   }
 
+  LocalIsolate* AsLocalIsolate() { return main_thread_local_isolate(); }
+
   LocalHeap* main_thread_local_heap();
   LocalHeap* CurrentLocalHeap();
 

src/execution/local-isolate.h

@@ -85,6 +85,8 @@ class V8_EXPORT_PRIVATE LocalIsolate final : private HiddenLocalFactory {
 
   bool is_main_thread() const { return heap_.is_main_thread(); }
 
+  LocalIsolate* AsLocalIsolate() { return this; }
+
  private:
   friend class v8::internal::LocalFactory;
 

src/heap/local-heap-inl.h

@@ -26,6 +26,9 @@ AllocationResult LocalHeap::AllocateRaw(int size_in_bytes, AllocationType type,
   DCHECK(state == Heap::TEAR_DOWN || state == Heap::NOT_IN_GC);
 #endif
 
+  // Each allocation is supposed to be a safepoint.
+  Safepoint();
+
   bool large_object = size_in_bytes > Heap::MaxRegularHeapObjectSize(type);
   CHECK_EQ(type, AllocationType::kOld);
 

src/interpreter/interpreter.cc

@@ -12,6 +12,8 @@
 #include "src/ast/scopes.h"
 #include "src/codegen/compiler.h"
 #include "src/codegen/unoptimized-compilation-info.h"
+#include "src/execution/local-isolate.h"
+#include "src/heap/parked-scope.h"
 #include "src/init/bootstrapper.h"
 #include "src/init/setup-isolate.h"
 #include "src/interpreter/bytecode-generator.h"
@@ -31,10 +33,10 @@ namespace interpreter {
 
 class InterpreterCompilationJob final : public UnoptimizedCompilationJob {
  public:
-  InterpreterCompilationJob(
-      ParseInfo* parse_info, FunctionLiteral* literal,
-      AccountingAllocator* allocator,
-      std::vector<FunctionLiteral*>* eager_inner_literals);
+  InterpreterCompilationJob(ParseInfo* parse_info, FunctionLiteral* literal,
+                            AccountingAllocator* allocator,
+                            std::vector<FunctionLiteral*>* eager_inner_literals,
+                            LocalIsolate* local_isolate);
   InterpreterCompilationJob(const InterpreterCompilationJob&) = delete;
   InterpreterCompilationJob& operator=(const InterpreterCompilationJob&) =
       delete;
@@ -59,6 +61,7 @@ class InterpreterCompilationJob final : public UnoptimizedCompilationJob {
 
   Zone zone_;
   UnoptimizedCompilationInfo compilation_info_;
+  LocalIsolate* local_isolate_;
   BytecodeGenerator generator_;
 };
 
@@ -156,11 +159,13 @@ bool ShouldPrintBytecode(Handle<SharedFunctionInfo> shared) {
 InterpreterCompilationJob::InterpreterCompilationJob(
     ParseInfo* parse_info, FunctionLiteral* literal,
     AccountingAllocator* allocator,
-    std::vector<FunctionLiteral*>* eager_inner_literals)
+    std::vector<FunctionLiteral*>* eager_inner_literals,
+    LocalIsolate* local_isolate)
     : UnoptimizedCompilationJob(parse_info->stack_limit(), parse_info,
                                 &compilation_info_),
       zone_(allocator, ZONE_NAME),
       compilation_info_(&zone_, parse_info, literal),
+      local_isolate_(local_isolate),
       generator_(&zone_, &compilation_info_, parse_info->ast_string_constants(),
                  eager_inner_literals) {}
 
@@ -176,6 +181,9 @@ InterpreterCompilationJob::Status InterpreterCompilationJob::ExecuteJobImpl() {
   // then ASTs from different functions may be intersperse when printed.
   MaybePrintAst(parse_info(), compilation_info());
 
+  base::Optional<ParkedScope> parked_scope;
+  if (local_isolate_) parked_scope.emplace(local_isolate_);
+
   generator()->GenerateBytecode(stack_limit());
 
   if (generator()->HasStackOverflow()) {
@@ -284,17 +292,19 @@ InterpreterCompilationJob::Status InterpreterCompilationJob::DoFinalizeJobImpl(
 std::unique_ptr<UnoptimizedCompilationJob> Interpreter::NewCompilationJob(
     ParseInfo* parse_info, FunctionLiteral* literal,
     AccountingAllocator* allocator,
-    std::vector<FunctionLiteral*>* eager_inner_literals) {
+    std::vector<FunctionLiteral*>* eager_inner_literals,
+    LocalIsolate* local_isolate) {
   return std::make_unique<InterpreterCompilationJob>(
-      parse_info, literal, allocator, eager_inner_literals);
+      parse_info, literal, allocator, eager_inner_literals, local_isolate);
 }
 
 std::unique_ptr<UnoptimizedCompilationJob>
 Interpreter::NewSourcePositionCollectionJob(
     ParseInfo* parse_info, FunctionLiteral* literal,
-    Handle<BytecodeArray> existing_bytecode, AccountingAllocator* allocator) {
-  auto job = std::make_unique<InterpreterCompilationJob>(parse_info, literal,
-                                                         allocator, nullptr);
+    Handle<BytecodeArray> existing_bytecode, AccountingAllocator* allocator,
+    LocalIsolate* local_isolate) {
+  auto job = std::make_unique<InterpreterCompilationJob>(
+      parse_info, literal, allocator, nullptr, local_isolate);
   job->compilation_info()->SetBytecodeArray(existing_bytecode);
   return job;
 }

src/interpreter/interpreter.h

@@ -23,6 +23,7 @@ class Callable;
 class UnoptimizedCompilationJob;
 class FunctionLiteral;
 class Isolate;
+class LocalIsolate;
 class ParseInfo;
 class RootVisitor;
 class SetupIsolateDelegate;
@@ -46,7 +47,8 @@ class Interpreter {
   static std::unique_ptr<UnoptimizedCompilationJob> NewCompilationJob(
       ParseInfo* parse_info, FunctionLiteral* literal,
       AccountingAllocator* allocator,
-      std::vector<FunctionLiteral*>* eager_inner_literals);
+      std::vector<FunctionLiteral*>* eager_inner_literals,
+      LocalIsolate* local_isolate);
 
   // Creates a compilation job which will generate source positions for
   // |literal| and when finalized, store the result into |existing_bytecode|.
@@ -54,7 +56,8 @@ class Interpreter {
   NewSourcePositionCollectionJob(ParseInfo* parse_info,
                                  FunctionLiteral* literal,
                                  Handle<BytecodeArray> existing_bytecode,
-                                 AccountingAllocator* allocator);
+                                 AccountingAllocator* allocator,
+                                 LocalIsolate* local_isolate);
 
   // If the bytecode handler for |bytecode| and |operand_scale| has not yet
   // been loaded, deserialize it. Then return the handler.