From 1c90992ffc2c6d12b9a130c1b7c93ff869bb5b7f Mon Sep 17 00:00:00 2001
From: Seth Brenith <seth.brenith@microsoft.com>
Date: Wed, 16 Nov 2022 10:18:33 -0800
Subject: [PATCH] Merge with cached Script after streaming compilation

Currently, if a script is compiled on the main thread or deserialized on
any thread, and a matching Script object is found in the Isolate
compilation cache, the new content is merged into the existing Script.
This CL implements the same merging for scripts which were compiled by a
background task. I expect speed changes to be minimal, because merging
is only needed in a small minority of compilations. When needed, it
usually takes about 10% as long as the deserialization of the script,
which in turn is faster than compilation from source text.

This CL also removes some code which I added in preparation for merging
on a background thread in this case. Upon further discussion, we've
determined that the extra round trip to a background thread when the
main thread is likely just waiting for completion would do more harm
than good, and performing the compilation cache lookup from the
background thread would be quite cumbersome.

Bug: v8:12808
Change-Id: Ia7a14a739779ab658b505572d19df4ec489a078e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4023904
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84402}
---
 src/codegen/compiler.cc | 70 ++++++++++-------------------------------
 src/codegen/compiler.h  | 26 ++-------------
 test/cctest/test-api.cc |  6 ----
 3 files changed, 18 insertions(+), 84 deletions(-)

diff --git a/src/codegen/compiler.cc b/src/codegen/compiler.cc
index 88496b8fd0..b87ebc2ba1 100644
--- a/src/codegen/compiler.cc
+++ b/src/codegen/compiler.cc
@@ -2148,7 +2148,8 @@ Handle<SharedFunctionInfo> BackgroundMergeTask::CompleteMergeInForeground(
 
 MaybeHandle<SharedFunctionInfo> BackgroundCompileTask::FinalizeScript(
     Isolate* isolate, Handle<String> source,
-    const ScriptDetails& script_details) {
+    const ScriptDetails& script_details,
+    MaybeHandle<Script> maybe_cached_script) {
   ScriptOriginOptions origin_options = script_details.origin_options;
 
   DCHECK(flags_.is_toplevel());
@@ -2166,11 +2167,15 @@ MaybeHandle<SharedFunctionInfo> BackgroundCompileTask::FinalizeScript(
     maybe_result = outer_function_sfi_;
   }
 
-  if (!maybe_result.is_null() &&
-      background_merge_task_.HasPendingForegroundWork()) {
-    DCHECK(flags().is_toplevel());
+  if (Handle<Script> cached_script;
+      maybe_cached_script.ToHandle(&cached_script) && !maybe_result.is_null()) {
+    BackgroundMergeTask merge;
+    merge.SetUpOnMainThread(isolate, cached_script);
+    CHECK(merge.HasPendingBackgroundWork());
+    merge.BeginMergeInBackground(isolate->AsLocalIsolate(), script);
+    CHECK(merge.HasPendingForegroundWork());
     Handle<SharedFunctionInfo> result =
-        background_merge_task_.CompleteMergeInForeground(isolate, script);
+        merge.CompleteMergeInForeground(isolate, script);
     maybe_result = result;
     script = handle(Script::cast(result->script()), isolate);
     DCHECK(script->source().StrictEquals(*source));
@@ -2213,7 +2218,6 @@ MaybeHandle<SharedFunctionInfo> BackgroundCompileTask::FinalizeScript(
 bool BackgroundCompileTask::FinalizeFunction(
     Isolate* isolate, Compiler::ClearExceptionFlag flag) {
   DCHECK(!flags_.is_toplevel());
-  DCHECK(!background_merge_task_.HasPendingForegroundWork());
 
   MaybeHandle<SharedFunctionInfo> maybe_result;
   Handle<SharedFunctionInfo> input_shared_info =
@@ -2306,33 +2310,12 @@ void BackgroundDeserializeTask::SourceTextAvailable(
                                            language_mode);
 }
 
-void BackgroundCompileTask::SourceTextAvailable(
-    Isolate* isolate, Handle<String> source_text,
-    const ScriptDetails& script_details) {
-  DCHECK_EQ(isolate, isolate_for_local_isolate_);
-
-  // Non-toplevel compilations already refer to an existing Script; there is no
-  // need to look anything up in the compilation cache.
-  if (!flags().is_toplevel()) return;
-
-  LanguageMode language_mode = flags().outer_language_mode();
-  background_merge_task_.SetUpOnMainThread(isolate, source_text, script_details,
-                                           language_mode);
-}
-
 bool BackgroundDeserializeTask::ShouldMergeWithExistingScript() const {
   DCHECK(v8_flags.merge_background_deserialized_script_with_compilation_cache);
   return background_merge_task_.HasPendingBackgroundWork() &&
          off_thread_data_.HasResult();
 }
 
-bool BackgroundCompileTask::ShouldMergeWithExistingScript() const {
-  DCHECK(v8_flags.stress_background_compile);
-  DCHECK(!script_.is_null());
-  return background_merge_task_.HasPendingBackgroundWork() &&
-         jobs_to_retry_finalization_on_main_thread_.empty();
-}
-
 void BackgroundDeserializeTask::MergeWithExistingScript() {
   DCHECK(ShouldMergeWithExistingScript());
 
@@ -2344,21 +2327,6 @@ void BackgroundDeserializeTask::MergeWithExistingScript() {
       &isolate, off_thread_data_.GetOnlyScript(isolate.heap()));
 }
 
-void BackgroundCompileTask::MergeWithExistingScript() {
-  DCHECK(ShouldMergeWithExistingScript());
-
-  LocalIsolate isolate(isolate_for_local_isolate_, ThreadKind::kBackground);
-  UnparkedScope unparked_scope(&isolate);
-  LocalHandleScope handle_scope(isolate.heap());
-
-  // Get a non-persistent handle to the newly compiled script.
-  isolate.heap()->AttachPersistentHandles(std::move(persistent_handles_));
-  Handle<Script> script = handle(*script_, &isolate);
-  persistent_handles_ = isolate.heap()->DetachPersistentHandles();
-
-  background_merge_task_.BeginMergeInBackground(&isolate, script);
-}
-
 MaybeHandle<SharedFunctionInfo> BackgroundDeserializeTask::Finish(
     Isolate* isolate, Handle<String> source,
     ScriptOriginOptions origin_options) {
@@ -3306,15 +3274,9 @@ class StressBackgroundCompileThread : public base::Thread {
     data()->task = std::make_unique<i::BackgroundCompileTask>(
         data(), isolate, type,
         ScriptCompiler::CompileOptions::kNoCompileOptions);
-    data()->task->SourceTextAvailable(isolate, source, script_details);
   }
 
-  void Run() override {
-    data()->task->Run();
-    if (data()->task->ShouldMergeWithExistingScript()) {
-      data()->task->MergeWithExistingScript();
-    }
-  }
+  void Run() override { data()->task->Run(); }
 
   ScriptStreamingData* data() { return streamed_source_.impl(); }
 
@@ -3748,6 +3710,7 @@ Compiler::GetSharedFunctionInfoForStreamedScript(
   BackgroundCompileTask* task = streaming_data->task.get();
 
   MaybeHandle<SharedFunctionInfo> maybe_result;
+  MaybeHandle<Script> maybe_cached_script;
   // Check if compile cache already holds the SFI, if so no need to finalize
   // the code compiled on the background thread.
   CompilationCache* compilation_cache = isolate->compilation_cache();
@@ -3758,16 +3721,14 @@ Compiler::GetSharedFunctionInfoForStreamedScript(
         compilation_cache->LookupScript(source, script_details,
                                         task->flags().outer_language_mode());
 
-    // TODO(v8:12808): Determine what to do if we finish streaming and find that
-    // another copy of the Script already exists but has no root
-    // SharedFunctionInfo or has an uncompiled SharedFunctionInfo. For now, we
-    // just ignore it and create a new Script.
     if (!lookup_result.toplevel_sfi().is_null()) {
       maybe_result = lookup_result.toplevel_sfi();
     }
 
     if (!maybe_result.is_null()) {
       compile_timer.set_hit_isolate_cache();
+    } else {
+      maybe_cached_script = lookup_result.script();
     }
   }
 
@@ -3779,7 +3740,8 @@ Compiler::GetSharedFunctionInfoForStreamedScript(
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.compile"),
                  "V8.OffThreadFinalization.Publish");
 
-    maybe_result = task->FinalizeScript(isolate, source, script_details);
+    maybe_result = task->FinalizeScript(isolate, source, script_details,
+                                        maybe_cached_script);
 
     Handle<SharedFunctionInfo> result;
     if (maybe_result.ToHandle(&result)) {
diff --git a/src/codegen/compiler.h b/src/codegen/compiler.h
index 7a0d18a452..ce77192393 100644
--- a/src/codegen/compiler.h
+++ b/src/codegen/compiler.h
@@ -534,28 +534,10 @@ class V8_EXPORT_PRIVATE BackgroundCompileTask {
   void Run(LocalIsolate* isolate,
            ReusableUnoptimizedCompileState* reusable_state);
 
-  // Checks the Isolate compilation cache to see whether it will be necessary to
-  // merge the newly compiled objects into an existing Script. This can change
-  // the value of ShouldMergeWithExistingScript, and embedders should check the
-  // latter after calling this. May only be called on a thread where the Isolate
-  // is currently entered.
-  void SourceTextAvailable(Isolate* isolate, Handle<String> source_text,
-                           const ScriptDetails& script_details);
-
-  // Returns whether the embedder should call MergeWithExistingScript. This
-  // function may be called from any thread, any number of times, but its return
-  // value is only meaningful after SourceTextAvailable has completed.
-  bool ShouldMergeWithExistingScript() const;
-
-  // Partially merges newly compiled objects into an existing Script with the
-  // same source, and generates a list of follow-up work for the main thread.
-  // May be called from any thread, only once, and only if
-  // ShouldMergeWithExistingScript returned true.
-  void MergeWithExistingScript();
-
   MaybeHandle<SharedFunctionInfo> FinalizeScript(
       Isolate* isolate, Handle<String> source,
-      const ScriptDetails& script_details);
+      const ScriptDetails& script_details,
+      MaybeHandle<Script> maybe_cached_script);
 
   bool FinalizeFunction(Isolate* isolate, Compiler::ClearExceptionFlag flag);
 
@@ -593,10 +575,6 @@ class V8_EXPORT_PRIVATE BackgroundCompileTask {
   int start_position_;
   int end_position_;
   int function_literal_id_;
-
-  // Task that merges newly compiled content into an existing Script from the
-  // Isolate compilation cache, if necessary.
-  BackgroundMergeTask background_merge_task_;
 };
 
 // Contains all data which needs to be transmitted between threads for
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index d1fe4973dd..8d2ab7f012 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -23792,12 +23792,6 @@ TEST(StreamingWithIsolateScriptCache) {
 // Variant of the above test which evicts the root SharedFunctionInfo from the
 // Isolate script cache but still reuses the same Script.
 TEST(StreamingWithIsolateScriptCacheClearingRootSFI) {
-  // TODO(v8:12808): Remove this check once background compilation is capable of
-  // reusing an existing Script.
-  if (i::v8_flags.stress_background_compile) {
-    return;
-  }
-
   StreamingWithIsolateScriptCache(true);
 }