[compiler] Multiple fixes for reuse_scope_infos

verwaest · V8 LUCI CQ · commit 67dd629e34bb · 2024-07-22T13:32:54.000Z
- Also reuse the scope_info of the function itself. It might be in RO_SPACE in case the script is in the snapshot, and it would be a shame to recreate it. - Make sure we don't drop (or forget to pick up) scope infos for newly compiled sfis that already existed in the script - Make sure to reattach scope info chains wherever an outer scope info exists first. Due to code caches with spotty coverage we might see unexpected SFI/scope info combinations. This also adds a flag to run verification on scope info reuse after merging. Bug: 352673356 Change-Id: Iad702d2ab775cdcb72bdc472942601c340f4f4d2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5725954 Reviewed-by: Leszek Swirski <leszeks@chromium.org> Commit-Queue: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/main@{#95154}
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
@@ -2611,7 +2611,8 @@ void ModuleScope::AllocateModuleVariables() {
   }
 }
 
-// Needs to be kept in sync with ScopeInfo::UniqueIdInScript.
+// Needs to be kept in sync with ScopeInfo::UniqueIdInScript and
+// SharedFunctionInfo::UniqueIdInScript.
 int Scope::UniqueIdInScript() const {
   // Script scopes start "before" the script to avoid clashing with a scope that
   // starts on character 0.
@@ -2805,20 +2806,25 @@ void DeclarationScope::AllocateScopeInfos(ParseInfo* info,
     Tagged<ScopeInfo> outer = sfi->HasOuterScopeInfo()
                                   ? sfi->GetOuterScopeInfo()
                                   : Tagged<ScopeInfo>();
-    // Look at all the existing inner functions (they are numbered id+1 until
-    // max_id+1) to reattach their outer scope infos to corresponding scopes.
-    for (int i = info->literal()->function_literal_id() + 1;
+    // Look at all inner functions whether they have scope infos that we should
+    // reuse. Also look at the compiled function itself, and reuse its function
+    // scope info if it exists.
+    for (int i = info->literal()->function_literal_id();
          i < info->max_info_id() + 1; ++i) {
       Tagged<MaybeObject> maybe_info = infos->get(i);
       if (maybe_info.IsWeak()) {
         Tagged<Object> info = maybe_info.GetHeapObjectAssumeWeak();
         Tagged<ScopeInfo> scope_info;
         if (Is<SharedFunctionInfo>(info)) {
           Tagged<SharedFunctionInfo> sfi = Cast<SharedFunctionInfo>(info);
-          // Reuse outer scope infos. Don't look at sfi->scope_info() because
-          // that might be empty if the sfi isn't compiled yet.
-          if (!sfi->HasOuterScopeInfo()) continue;
-          scope_info = sfi->GetOuterScopeInfo();
+          if (!sfi->scope_info()->IsEmpty() &&
+              sfi->scope_info()->HasContext()) {
+            scope_info = sfi->scope_info();
+          } else if (sfi->HasOuterScopeInfo()) {
+            scope_info = sfi->GetOuterScopeInfo();
+          } else {
+            continue;
+          }
         } else {
           scope_info = Cast<ScopeInfo>(info);
         }
diff --git a/src/codegen/compiler.cc b/src/codegen/compiler.cc
@@ -2022,18 +2022,39 @@ class ConstantPoolPointerForwarder {
   }
 
   void RecordScopeInfos(Tagged<MaybeObject> maybe_old_info) {
+    RecordScopeInfos(maybe_old_info.GetHeapObjectAssumeWeak());
+  }
+
+  // Record all scope infos relevant for a shared function info or scope info
+  // (recorded for eval).
+  void RecordScopeInfos(Tagged<HeapObject> info) {
     Tagged<ScopeInfo> scope_info;
-    Tagged<HeapObject> info = maybe_old_info.GetHeapObjectAssumeWeak();
     if (Is<SharedFunctionInfo>(info)) {
       Tagged<SharedFunctionInfo> old_sfi = Cast<SharedFunctionInfo>(info);
-      if (!old_sfi->HasOuterScopeInfo()) return;
-      scope_info = old_sfi->GetOuterScopeInfo();
+      // Also record context-having own scope infos for SFIs.
+      if (!old_sfi->scope_info()->IsEmpty() &&
+          old_sfi->scope_info()->HasContext()) {
+        scope_info = old_sfi->scope_info();
+      } else if (old_sfi->HasOuterScopeInfo()) {
+        scope_info = old_sfi->GetOuterScopeInfo();
+      } else {
+        return;
+      }
     } else {
       scope_info = Cast<ScopeInfo>(info);
     }
+
     while (true) {
-      if (scope_infos_to_update_.find(scope_info->UniqueIdInScript()) !=
-          scope_infos_to_update_.end()) {
+      auto it = scope_infos_to_update_.find(scope_info->UniqueIdInScript());
+      if (it != scope_infos_to_update_.end()) {
+        // Once we find an already recorded scope info, it need to match the one
+        // on the chain.
+        if (V8_UNLIKELY(*it->second != scope_info)) {
+          info->Print();
+          (*it->second)->Print();
+          scope_info->Print();
+          UNREACHABLE();
+        }
         return;
       }
       scope_infos_to_update_[scope_info->UniqueIdInScript()] =
@@ -2062,17 +2083,51 @@ class ConstantPoolPointerForwarder {
            !scope_infos_to_update_.empty();
   }
 
-  void UpdateOuterScopeInfo(Tagged<SharedFunctionInfo> sfi) {
+  // Find an own scope info for the sfi based on the UniqueIdInScript that the
+  // own scope info would have. This works even if the SFI doesn't yet have a
+  // scope info attached by computing UniqueIdInScript from the SFI position.
+  //
+  // This should only directly be used for SFIs that already existed on the
+  // script. Their outer scope info will already be correct.
+  bool InstallOwnScopeInfo(Tagged<SharedFunctionInfo> sfi) {
+    auto it = scope_infos_to_update_.find(sfi->UniqueIdInScript());
+    if (it == scope_infos_to_update_.end()) return false;
+    sfi->SetScopeInfo(*it->second);
+    return true;
+  }
+
+  // Either replace the own scope info of the sfi, or the first outer scope info
+  // that was recorded.
+  //
+  // This has to be used for all newly created SFIs since their outer scope info
+  // also may need to be reattached.
+  void UpdateScopeInfo(Tagged<SharedFunctionInfo> sfi) {
+    // This should not be called on already existing SFIs. Their scope infos are
+    // already correct.
+    DCHECK_NE(MakeWeak(sfi),
+              old_script_->infos()->get(sfi->function_literal_id()));
+    if (InstallOwnScopeInfo(sfi)) return;
     if (!sfi->HasOuterScopeInfo()) return;
+
+    Tagged<ScopeInfo> parent =
+        sfi->scope_info()->IsEmpty() ? Tagged<ScopeInfo>() : sfi->scope_info();
     Tagged<ScopeInfo> outer_info = sfi->GetOuterScopeInfo();
+
     auto it = scope_infos_to_update_.find(outer_info->UniqueIdInScript());
-    if (it == scope_infos_to_update_.end()) return;
+    while (it == scope_infos_to_update_.end()) {
+      if (!outer_info->HasOuterScopeInfo()) return;
+      parent = outer_info;
+      outer_info = outer_info->OuterScopeInfo();
+      it = scope_infos_to_update_.find(outer_info->UniqueIdInScript());
+    }
     if (outer_info == *it->second) return;
+
     VerifyScopeInfo(outer_info, *it->second);
-    if (sfi->is_compiled()) {
-      sfi->scope_info()->set_outer_scope_info(*it->second);
-    } else {
+
+    if (parent.is_null()) {
       sfi->set_raw_outer_scope_info_or_feedback_metadata(*it->second);
+    } else {
+      parent->set_outer_scope_info(*it->second);
     }
   }
 
@@ -2254,6 +2309,16 @@ void BackgroundMergeTask::BeginMergeInBackground(
           new_compiled_data_for_cached_sfis_.push_back(
               {local_heap->NewPersistentHandle(old_sfi),
                local_heap->NewPersistentHandle(new_sfi)});
+          // Pick up existing scope infos from the old sfi. The new sfi will be
+          // copied over the old sfi later. This will ensure that we'll keep
+          // using the old sfis. This will also allow us check later whether new
+          // scope infos have appeared that need to be reused.
+          if (!old_sfi->scope_info()->IsEmpty()) {
+            new_sfi->SetScopeInfo(old_sfi->scope_info());
+          } else if (old_sfi->HasOuterScopeInfo()) {
+            new_sfi->scope_info()->set_outer_scope_info(
+                old_sfi->GetOuterScopeInfo());
+          }
           forwarder.AddBytecodeArray(new_sfi->GetBytecodeArray(isolate));
         }
       } else {
@@ -2278,13 +2343,23 @@ void BackgroundMergeTask::BeginMergeInBackground(
 
   if (forwarder.HasAnythingToForward()) {
     for (DirectHandle<SharedFunctionInfo> new_sfi : used_new_sfis_) {
-      forwarder.UpdateOuterScopeInfo(*new_sfi);
+      forwarder.UpdateScopeInfo(*new_sfi);
+    }
+    for (const auto& new_compiled_data : new_compiled_data_for_cached_sfis_) {
+      // It's possible that new_compiled_data.cached_sfi had
+      // scope_info()->IsEmpty() while an inner function has scope info if the
+      // cached_sfi was recreated when an outer function was recompiled. If so,
+      // new_compiled_data.new_sfi does not have a reused scope info yet, and
+      // we'll have found it when we visited the inner function. Try to pick it
+      // up here.
+      forwarder.InstallOwnScopeInfo(*new_compiled_data.new_sfi);
     }
     forwarder.IterateAndForwardPointers();
   }
   persistent_handles_ = local_heap->DetachPersistentHandles();
   state_ = kPendingForegroundWork;
 }
+
 Handle<SharedFunctionInfo> BackgroundMergeTask::CompleteMergeInForeground(
     Isolate* isolate, DirectHandle<Script> new_script) {
   DCHECK_EQ(state_, kPendingForegroundWork);
@@ -2295,8 +2370,8 @@ Handle<SharedFunctionInfo> BackgroundMergeTask::CompleteMergeInForeground(
       isolate, isolate->main_thread_local_heap(), old_script);
 
   for (const auto& new_compiled_data : new_compiled_data_for_cached_sfis_) {
-    if (!new_compiled_data.cached_sfi->is_compiled() &&
-        new_compiled_data.new_sfi->is_compiled()) {
+    Tagged<SharedFunctionInfo> sfi = *new_compiled_data.cached_sfi;
+    if (!sfi->is_compiled() && new_compiled_data.new_sfi->is_compiled()) {
       // Updating existing DebugInfos is not supported, but we don't expect
       // uncompiled SharedFunctionInfos to contain DebugInfos.
       DCHECK(!new_compiled_data.cached_sfi->HasDebugInfo(isolate));
@@ -2306,8 +2381,7 @@ Handle<SharedFunctionInfo> BackgroundMergeTask::CompleteMergeInForeground(
       // cached_sfi to new_sfi, and then copy every field using CopyFrom.
       new_compiled_data.new_sfi->set_script(
           new_compiled_data.cached_sfi->script(kAcquireLoad), kReleaseStore);
-      new_compiled_data.cached_sfi->CopyFrom(*new_compiled_data.new_sfi,
-                                             isolate);
+      sfi->CopyFrom(*new_compiled_data.new_sfi, isolate);
     }
   }
 
@@ -2336,12 +2410,17 @@ Handle<SharedFunctionInfo> BackgroundMergeTask::CompleteMergeInForeground(
   // pools is required.
   if (forwarder.HasAnythingToForward()) {
     for (DirectHandle<SharedFunctionInfo> new_sfi : used_new_sfis_) {
-      forwarder.UpdateOuterScopeInfo(*new_sfi);
+      forwarder.UpdateScopeInfo(*new_sfi);
       if (new_sfi->HasBytecodeArray(isolate)) {
         forwarder.AddBytecodeArray(new_sfi->GetBytecodeArray(isolate));
       }
     }
     for (const auto& new_compiled_data : new_compiled_data_for_cached_sfis_) {
+      // It's possible that cached_sfi wasn't compiled, but an inner function
+      // existed that didn't exist when be background merged. In that case, pick
+      // up the relevant scope infos.
+      Tagged<SharedFunctionInfo> sfi = *new_compiled_data.cached_sfi;
+      forwarder.InstallOwnScopeInfo(sfi);
       if (new_compiled_data.cached_sfi->HasBytecodeArray(isolate)) {
         forwarder.AddBytecodeArray(
             new_compiled_data.cached_sfi->GetBytecodeArray(isolate));
@@ -2364,6 +2443,45 @@ Handle<SharedFunctionInfo> BackgroundMergeTask::CompleteMergeInForeground(
     SharedFunctionInfo::EnsureSourcePositionsAvailable(isolate, result);
   }
 
+  if (v8_flags.verify_code_merge) {
+    // Check that there aren't any duplicate scope infos. Every scope/context
+    // should correspond to at most one scope info.
+    std::unordered_map<int, Tagged<ScopeInfo>> scope_infos;
+    for (int i = 0; i < old_script->infos()->length(); i++) {
+      Tagged<ScopeInfo> scope_info;
+      if (!old_script->infos()->get(i).IsWeak()) continue;
+      Tagged<HeapObject> info =
+          old_script->infos()->get(i).GetHeapObjectAssumeWeak();
+      if (Is<SharedFunctionInfo>(info)) {
+        Tagged<SharedFunctionInfo> old_sfi = Cast<SharedFunctionInfo>(info);
+        if (!old_sfi->scope_info()->IsEmpty()) {
+          scope_info = old_sfi->scope_info();
+        } else if (old_sfi->HasOuterScopeInfo()) {
+          scope_info = old_sfi->GetOuterScopeInfo();
+        } else {
+          continue;
+        }
+      } else {
+        scope_info = Cast<ScopeInfo>(info);
+      }
+      while (true) {
+        auto it = scope_infos.find(scope_info->UniqueIdInScript());
+        if (it != scope_infos.end()) {
+          if (*it->second != scope_info) {
+            old_script->infos()->get(i).GetHeapObjectAssumeWeak()->Print();
+            (*it->second)->Print();
+            scope_info->Print();
+            UNREACHABLE();
+          }
+          break;
+        }
+        scope_infos[scope_info->UniqueIdInScript()] = scope_info;
+        if (!scope_info->HasOuterScopeInfo()) break;
+        scope_info = scope_info->OuterScopeInfo();
+      }
+    }
+  }
+
   return handle_scope.CloseAndEscape(result);
 }
 
diff --git a/src/flags/flag-definitions.h b/src/flags/flag-definitions.h
@@ -2217,6 +2217,7 @@ DEFINE_BOOL(
     merge_background_deserialized_script_with_compilation_cache, true,
     "After deserializing code cache data on a background thread, merge it into "
     "an existing Script if one is found in the Isolate compilation cache")
+DEFINE_BOOL(verify_code_merge, false, "Verify scope infos after merge")
 DEFINE_BOOL(
     embedder_instance_types, false,
     "enable type checks based on instance types provided by the embedder")
diff --git a/src/objects/scope-info.cc b/src/objects/scope-info.cc
@@ -726,7 +726,8 @@ int ScopeInfo::ContextLength() const {
          (function_name_context_slot ? 1 : 0);
 }
 
-// Needs to be kept in sync with Scope::UniqueIdInScript.
+// Needs to be kept in sync with Scope::UniqueIdInScript and
+// SharedFunctionInfo::UniqueIdInScript.
 int ScopeInfo::UniqueIdInScript() const {
   // Script scopes start "before" the script to avoid clashing with a scope that
   // starts on character 0.
diff --git a/src/objects/shared-function-info-inl.h b/src/objects/shared-function-info-inl.h
@@ -613,11 +613,11 @@ DEF_GETTER(SharedFunctionInfo, outer_scope_info, Tagged<HeapObject>) {
 
 bool SharedFunctionInfo::HasOuterScopeInfo() const {
   Tagged<ScopeInfo> outer_info;
-  if (!is_compiled()) {
+  Tagged<ScopeInfo> info = scope_info(kAcquireLoad);
+  if (info->IsEmpty()) {
     if (!IsScopeInfo(outer_scope_info())) return false;
     outer_info = Cast<ScopeInfo>(outer_scope_info());
   } else {
-    Tagged<ScopeInfo> info = scope_info(kAcquireLoad);
     if (!info->HasOuterScopeInfo()) return false;
     outer_info = info->OuterScopeInfo();
   }
@@ -626,15 +626,17 @@ bool SharedFunctionInfo::HasOuterScopeInfo() const {
 
 Tagged<ScopeInfo> SharedFunctionInfo::GetOuterScopeInfo() const {
   DCHECK(HasOuterScopeInfo());
-  if (!is_compiled()) return Cast<ScopeInfo>(outer_scope_info());
-  return scope_info(kAcquireLoad)->OuterScopeInfo();
+  Tagged<ScopeInfo> info = scope_info(kAcquireLoad);
+  if (info->IsEmpty()) return Cast<ScopeInfo>(outer_scope_info());
+  return info->OuterScopeInfo();
 }
 
 void SharedFunctionInfo::set_outer_scope_info(Tagged<HeapObject> value,
                                               WriteBarrierMode mode) {
   DCHECK(!is_compiled());
   DCHECK(IsTheHole(raw_outer_scope_info_or_feedback_metadata()));
   DCHECK(IsScopeInfo(value) || IsTheHole(value));
+  DCHECK(scope_info()->IsEmpty());
   set_raw_outer_scope_info_or_feedback_metadata(value, mode);
 }
 
diff --git a/src/objects/shared-function-info.cc b/src/objects/shared-function-info.cc
@@ -265,6 +265,17 @@ bool SharedFunctionInfo::HasDebugInfo(Isolate* isolate) const {
   return isolate->debug()->HasDebugInfo(*this);
 }
 
+// Needs to be kept in sync with Scope::UniqueIdInScript and
+// ScopeInfo::UniqueIdInScript.
+int SharedFunctionInfo::UniqueIdInScript() const {
+  // Script scopes start "before" the script to avoid clashing with a scope that
+  // starts on character 0.
+  if (function_literal_id() == kFunctionLiteralIdTopLevel) return -1;
+  // Default constructors have the same start position as their parent class
+  // scope. Use the next char position to distinguish this scope.
+  return StartPosition() + IsDefaultConstructor(kind());
+}
+
 Tagged<DebugInfo> SharedFunctionInfo::GetDebugInfo(Isolate* isolate) const {
   return isolate->debug()->TryGetDebugInfo(*this).value();
 }
diff --git a/src/objects/shared-function-info.h b/src/objects/shared-function-info.h
@@ -597,6 +597,8 @@ class SharedFunctionInfo
 
   inline FunctionKind kind() const;
 
+  int UniqueIdInScript() const;
+
   // Defines the index in a native context of closure's map instantiated using
   // this shared function info.
   DECL_INT_ACCESSORS(function_map_index)

Original file line number	Diff line number	Diff line change
`@@ -726,7 +726,8 @@ int ScopeInfo::ContextLength() const {`
`726`	`726`	`(function_name_context_slot ? 1 : 0);`
`727`	`727`	`}`
`728`	`728`
`729`		`-// Needs to be kept in sync with Scope::UniqueIdInScript.`
	`729`	`+// Needs to be kept in sync with Scope::UniqueIdInScript and`
	`730`	`+// SharedFunctionInfo::UniqueIdInScript.`
`730`	`731`	`int ScopeInfo::UniqueIdInScript() const {`
`731`	`732`	`// Script scopes start "before" the script to avoid clashing with a scope that`
`732`	`733`	`// starts on character 0.`