Let script compilation cache keys outlive their values

This is a partial reland of https://crrev.com/c/3597106

With this change, an old entry in the script compilation cache is not
completely removed by CompilationCacheScript::Age(). Instead, its value
is replaced with undefined. In that way, the Script is still accessible
from the table until the garbage collector destroys it and clears the
weak pointer.

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

src/codegen/compilation-cache.cc

@@ -77,7 +77,8 @@ void CompilationCacheScript::Age() {
       SharedFunctionInfo info = SharedFunctionInfo::cast(value);
       if (!info.HasBytecodeArray() ||
           info.GetBytecodeArray(isolate()).IsOld()) {
-        table.RemoveEntry(entry);
+        table.SetPrimaryValueAt(entry,
+                                ReadOnlyRoots(isolate()).undefined_value());
       }
     }
   }

src/objects/compilation-cache-table-inl.h

@@ -50,12 +50,10 @@ void CompilationCacheTable::SetEvalFeedbackValueAt(InternalIndex entry,
 // the Script. The corresponding value can be either the root SharedFunctionInfo
 // or undefined. The purpose of storing the root SharedFunctionInfo as the value
 // is to keep it alive, not to save a lookup on the Script. A newly added entry
-// always contains the root SharedFunctionInfo.
-//
-// TODO(v8:12808): After the root SharedFunctionInfo has aged sufficiently, it
-// should be replaced with undefined. In this way, all strong references to
-// large objects are dropped, but there is still a way to get the Script if it
-// happens to still be alive.
+// always contains the root SharedFunctionInfo. After the root
+// SharedFunctionInfo has aged sufficiently, it is replaced with undefined. In
+// this way, all strong references to large objects are dropped, but there is
+// still a way to get the Script if it happens to still be alive.
 class ScriptCacheKey : public HashTableKey {
  public:
   enum Index {

src/objects/compilation-cache-table.cc

@@ -398,6 +398,29 @@ Handle<Object> CompilationCacheTable::LookupRegExp(Handle<String> src,
   return Handle<Object>(PrimaryValueAt(entry), isolate);
 }
 
+Handle<CompilationCacheTable> CompilationCacheTable::EnsureScriptTableCapacity(
+    Isolate* isolate, Handle<CompilationCacheTable> cache) {
+  if (cache->HasSufficientCapacityToAdd(1)) return cache;
+
+  // Before resizing, delete are any entries whose keys contain cleared weak
+  // pointers.
+  {
+    DisallowGarbageCollection no_gc;
+    for (InternalIndex entry : cache->IterateEntries()) {
+      Object key;
+      if (!cache->ToKey(isolate, entry, &key)) continue;
+      if (WeakFixedArray::cast(key)
+              .Get(ScriptCacheKey::kWeakScript)
+              .IsCleared()) {
+        DCHECK(cache->PrimaryValueAt(entry).IsUndefined());
+        cache->RemoveEntry(entry);
+      }
+    }
+  }
+
+  return EnsureCapacity(isolate, cache);
+}
+
 Handle<CompilationCacheTable> CompilationCacheTable::PutScript(
     Handle<CompilationCacheTable> cache, Handle<String> src,
     Handle<SharedFunctionInfo> value, Isolate* isolate) {
@@ -413,11 +436,24 @@ Handle<CompilationCacheTable> CompilationCacheTable::PutScript(
                      script->column_offset(), script->origin_options(),
                      host_defined_options, isolate);
   Handle<Object> k = key.AsHandle(isolate, value);
-  cache = EnsureCapacity(isolate, cache);
-  InternalIndex entry = cache->FindInsertionEntry(isolate, key.Hash());
+
+  // Check whether there is already a matching entry. If so, we must overwrite
+  // it. This allows an entry whose value is undefined to upgrade to contain a
+  // SharedFunctionInfo.
+  InternalIndex entry = cache->FindEntry(isolate, &key);
+  bool found_existing = entry.is_found();
+  if (!found_existing) {
+    cache = EnsureScriptTableCapacity(isolate, cache);
+    entry = cache->FindInsertionEntry(isolate, key.Hash());
+  }
+  // TODO(v8:12808): Once all code paths are updated to reuse a Script if
+  // available, we could DCHECK here that the Script in the existing entry
+  // matches the Script in the new key. For now, there is no such guarantee.
   cache->SetKeyAt(entry, *k);
   cache->SetPrimaryValueAt(entry, *value);
-  cache->ElementAdded();
+  if (!found_existing) {
+    cache->ElementAdded();
+  }
   return cache;
 }
 
@@ -488,6 +524,10 @@ void CompilationCacheTable::RemoveEntry(InternalIndex entry) {
     NoWriteBarrierSet(*this, entry_index + i, the_hole_value);
   }
   ElementRemoved();
+
+  // This table does not shrink upon deletion. The script cache depends on that
+  // fact, because EnsureScriptTableCapacity calls RemoveEntry at a time when
+  // shrinking the table would be counterproductive.
 }
 
 }  // namespace internal

src/objects/compilation-cache-table.h

@@ -136,6 +136,9 @@ class CompilationCacheTable
   DECL_CAST(CompilationCacheTable)
 
  private:
+  static Handle<CompilationCacheTable> EnsureScriptTableCapacity(
+      Isolate* isolate, Handle<CompilationCacheTable> cache);
+
   OBJECT_CONSTRUCTORS(CompilationCacheTable,
                       HashTable<CompilationCacheTable, CompilationCacheShape>);
 };