Reland "Merge with cached Script after synchronous deserialization"

Changes since original:
- Updated to use the returned value from CompleteMergeInForeground as
  the compilation result, which is important for correctness.
- Added a test to verify the above.
- Moved the merge code into code-serializer.cc so that it can run before
  FinalizeDeserialization, which makes it more consistent with
  background deserialization.

Original change's description:
> Merge with cached Script after synchronous deserialization
>
> Currently, if a script is deserialized on a background 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 the much simpler case of deserializing on the main
> thread. 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 corresponding deserialization.
>
> Bug: v8:12808
> Change-Id: Ie7a92bcb3111edf4cdab0eddeb7567979b35f437
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4010100
> Reviewed-by: Leszek Swirski <leszeks@chromium.org>
> Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#84123}

Bug: v8:12808
Change-Id: I0628a381644e79888cb3ebdd97bda270814d0e9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4014644
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84255}

src/codegen/background-merge-task.h

@@ -33,6 +33,11 @@ class V8_EXPORT_PRIVATE BackgroundMergeTask {
                          const ScriptDetails& script_details,
                          LanguageMode language_mode);
 
+  // Alternative step 1: on the main thread, if the caller has already looked up
+  // the script in the Isolate compilation cache, set up the necessary
+  // persistent data for the background merge.
+  void SetUpOnMainThread(Isolate* isolate, Handle<Script> cached_script);
+
   // Step 2: on the background thread, update pointers in the new Script's
   // object graph to point to corresponding objects from the cached Script where
   // appropriate. May only be called if HasPendingBackgroundWork returned true.

src/codegen/compiler.cc

@@ -1972,9 +1972,6 @@ void BackgroundMergeTask::SetUpOnMainThread(Isolate* isolate,
     return;
   }
 
-  // Any data sent to the background thread will need to be a persistent handle.
-  persistent_handles_ = std::make_unique<PersistentHandles>(isolate);
-
   if (lookup_result.is_compiled_scope().is_compiled()) {
     // There already exists a compiled top-level SFI, so the main thread will
     // discard the background serialization results and use the top-level SFI
@@ -1985,11 +1982,18 @@ void BackgroundMergeTask::SetUpOnMainThread(Isolate* isolate,
   } else {
     DCHECK(lookup_result.toplevel_sfi().is_null());
     // A background merge is required.
-    state_ = kPendingBackgroundWork;
-    cached_script_ = persistent_handles_->NewHandle(*script);
+    SetUpOnMainThread(isolate, script);
   }
 }
 
+void BackgroundMergeTask::SetUpOnMainThread(Isolate* isolate,
+                                            Handle<Script> cached_script) {
+  // Any data sent to the background thread will need to be a persistent handle.
+  persistent_handles_ = std::make_unique<PersistentHandles>(isolate);
+  state_ = kPendingBackgroundWork;
+  cached_script_ = persistent_handles_->NewHandle(*cached_script);
+}
+
 void BackgroundMergeTask::BeginMergeInBackground(LocalIsolate* isolate,
                                                  Handle<Script> new_script) {
   DCHECK_EQ(state_, kPendingBackgroundWork);
@@ -3530,9 +3534,8 @@ MaybeHandle<SharedFunctionInfo> GetSharedFunctionInfoForScriptImpl(
         // would be non-trivial.
       } else {
         maybe_result = CodeSerializer::Deserialize(
-            isolate, cached_data, source, script_details.origin_options);
-        // TODO(v8:12808): Merge the newly deserialized code into a preexisting
-        // Script if one was found in the compilation cache.
+            isolate, cached_data, source, script_details.origin_options,
+            maybe_script);
       }
 
       bool consuming_code_cache_succeeded = false;

src/snapshot/code-serializer.cc

@@ -429,7 +429,8 @@ void BaselineBatchCompileIfSparkplugCompiled(Isolate* isolate, Script script) {
 
 MaybeHandle<SharedFunctionInfo> CodeSerializer::Deserialize(
     Isolate* isolate, AlignedCachedData* cached_data, Handle<String> source,
-    ScriptOriginOptions origin_options) {
+    ScriptOriginOptions origin_options,
+    MaybeHandle<Script> maybe_cached_script) {
   if (v8_flags.stress_background_compile) {
     StressOffThreadDeserializeThread thread(isolate, cached_data);
     CHECK(thread.Start());
@@ -468,6 +469,22 @@ MaybeHandle<SharedFunctionInfo> CodeSerializer::Deserialize(
     if (v8_flags.profile_deserialization) PrintF("[Deserializing failed]\n");
     return MaybeHandle<SharedFunctionInfo>();
   }
+
+  // Check whether the newly deserialized data should be merged into an
+  // existing Script from the Isolate compilation cache. If so, perform
+  // the merge in a single-threaded manner since this deserialization was
+  // single-threaded.
+  if (Handle<Script> cached_script;
+      maybe_cached_script.ToHandle(&cached_script)) {
+    BackgroundMergeTask merge;
+    merge.SetUpOnMainThread(isolate, cached_script);
+    CHECK(merge.HasPendingBackgroundWork());
+    Handle<Script> new_script = handle(Script::cast(result->script()), isolate);
+    merge.BeginMergeInBackground(isolate->AsLocalIsolate(), new_script);
+    CHECK(merge.HasPendingForegroundWork());
+    result = merge.CompleteMergeInForeground(isolate, new_script);
+  }
+
   BaselineBatchCompileIfSparkplugCompiled(isolate,
                                           Script::cast(result->script()));
   if (v8_flags.profile_deserialization) {

src/snapshot/code-serializer.h

@@ -85,7 +85,8 @@ class CodeSerializer : public Serializer {
 
   V8_WARN_UNUSED_RESULT static MaybeHandle<SharedFunctionInfo> Deserialize(
       Isolate* isolate, AlignedCachedData* cached_data, Handle<String> source,
-      ScriptOriginOptions origin_options);
+      ScriptOriginOptions origin_options,
+      MaybeHandle<Script> maybe_cached_script = {});
 
   V8_WARN_UNUSED_RESULT static OffThreadDeserializeData
   StartDeserializeOffThread(LocalIsolate* isolate,

test/cctest/test-serialize.cc

@@ -2867,6 +2867,66 @@ TEST(Regress503552) {
   delete cache_data;
 }
 
+static void CodeSerializerMergeDeserializedScript(bool retain_toplevel_sfi) {
+  v8_flags.stress_background_compile = false;
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  HandleScope outer_scope(isolate);
+  Handle<String> source = isolate->factory()->NewStringFromAsciiChecked(
+      "(function () {return 123;})");
+  AlignedCachedData* cached_data = nullptr;
+  Handle<Script> script;
+  {
+    HandleScope first_compilation_scope(isolate);
+    Handle<SharedFunctionInfo> shared = CompileScriptAndProduceCache(
+        isolate, source, ScriptDetails(), &cached_data,
+        v8::ScriptCompiler::kNoCompileOptions);
+    Handle<BytecodeArray> bytecode =
+        handle(shared->GetBytecodeArray(isolate), isolate);
+    for (int i = 0; i <= v8_flags.bytecode_old_age; ++i) {
+      bytecode->MakeOlder();
+    }
+    Handle<Script> local_script =
+        handle(Script::cast(shared->script()), isolate);
+    script = first_compilation_scope.CloseAndEscape(local_script);
+  }
+
+  Handle<HeapObject> retained_toplevel_sfi;
+  if (retain_toplevel_sfi) {
+    retained_toplevel_sfi =
+        handle(script->shared_function_infos().Get(0).GetHeapObjectAssumeWeak(),
+               isolate);
+  }
+
+  // GC twice in case incremental marking had already marked the bytecode array.
+  // After this, the Isolate compilation cache contains a weak reference to the
+  // Script but not the top-level SharedFunctionInfo.
+  CcTest::CollectAllGarbage();
+  CcTest::CollectAllGarbage();
+
+  Handle<SharedFunctionInfo> copy =
+      CompileScript(isolate, source, ScriptDetails(), cached_data,
+                    v8::ScriptCompiler::kConsumeCodeCache);
+  delete cached_data;
+
+  // The existing Script was reused.
+  CHECK_EQ(*script, copy->script());
+
+  // The existing top-level SharedFunctionInfo was also reused.
+  if (retain_toplevel_sfi) {
+    CHECK_EQ(*retained_toplevel_sfi, *copy);
+  }
+}
+
+TEST(CodeSerializerMergeDeserializedScript) {
+  CodeSerializerMergeDeserializedScript(/*retain_toplevel_sfi=*/false);
+}
+
+TEST(CodeSerializerMergeDeserializedScriptRetainingToplevelSfi) {
+  CodeSerializerMergeDeserializedScript(/*retain_toplevel_sfi=*/true);
+}
+
 UNINITIALIZED_TEST(SnapshotCreatorBlobNotCreated) {
   DisableAlwaysOpt();
   DisableEmbeddedBlobRefcounting();