Allow lookup of matching scripts in Isolate compilation cache

Currently, if the same script text is compiled multiple times with
differing details (such as name, line number, or host-defined options),
then multiple copies of that script are added to the Isolate's
compilation cache. However, any attempt to look up those scripts can
find only the first instance. This change makes the script compilation
cache behave more consistently by checking the details while searching
the hash table for a match, rather than after a potential match has been
found.

Bug: v8:12808
Change-Id: Ic9da0bf74f359d4f1c88af89d585404f173056ee
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3671615Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#80919}

src/codegen/compilation-cache.cc

@@ -4,7 +4,6 @@
 
 #include "src/codegen/compilation-cache.h"
 
-#include "src/codegen/script-details.h"
 #include "src/common/globals.h"
 #include "src/heap/factory.h"
 #include "src/logging/counters.h"
@@ -149,84 +148,20 @@ void CompilationCacheEvalOrScript::Remove(
   CompilationCacheTable::cast(table_).Remove(*function_info);
 }
 
-namespace {
-
-// We only re-use a cached function for some script source code if the
-// script originates from the same place. This is to avoid issues
-// when reporting errors, etc.
-bool HasOrigin(Isolate* isolate, Handle<SharedFunctionInfo> function_info,
-               const ScriptDetails& script_details) {
-  Handle<Script> script =
-      Handle<Script>(Script::cast(function_info->script()), isolate);
-  // If the script name isn't set, the boilerplate script should have
-  // an undefined name to have the same origin.
-  Handle<Object> name;
-  if (!script_details.name_obj.ToHandle(&name)) {
-    return script->name().IsUndefined(isolate);
-  }
-  // Do the fast bailout checks first.
-  if (script_details.line_offset != script->line_offset()) return false;
-  if (script_details.column_offset != script->column_offset()) return false;
-  // Check that both names are strings. If not, no match.
-  if (!name->IsString() || !script->name().IsString()) return false;
-  // Are the origin_options same?
-  if (script_details.origin_options.Flags() !=
-      script->origin_options().Flags()) {
-    return false;
-  }
-  // Compare the two name strings for equality.
-  if (!String::Equals(isolate, Handle<String>::cast(name),
-                      Handle<String>(String::cast(script->name()), isolate))) {
-    return false;
-  }
-
-  // TODO(cbruni, chromium:1244145): Remove once migrated to the context
-  Handle<Object> maybe_host_defined_options;
-  if (!script_details.host_defined_options.ToHandle(
-          &maybe_host_defined_options)) {
-    maybe_host_defined_options = isolate->factory()->empty_fixed_array();
-  }
-  Handle<FixedArray> host_defined_options =
-      Handle<FixedArray>::cast(maybe_host_defined_options);
-  Handle<FixedArray> script_options(
-      FixedArray::cast(script->host_defined_options()), isolate);
-  int length = host_defined_options->length();
-  if (length != script_options->length()) return false;
-
-  for (int i = 0; i < length; i++) {
-    // host-defined options is a v8::PrimitiveArray.
-    DCHECK(host_defined_options->get(i).IsPrimitive());
-    DCHECK(script_options->get(i).IsPrimitive());
-    if (!host_defined_options->get(i).StrictEquals(script_options->get(i))) {
-      return false;
-    }
-  }
-  return true;
-}
-}  // namespace
-
-// TODO(245): Need to allow identical code from different contexts to
-// be cached in the same script generation. Currently the first use
-// will be cached, but subsequent code from different source / line
-// won't.
 MaybeHandle<SharedFunctionInfo> CompilationCacheScript::Lookup(
     Handle<String> source, const ScriptDetails& script_details) {
   MaybeHandle<SharedFunctionInfo> result;
 
-  // Probe the script generation tables. Make sure not to leak handles
+  // Probe the script table. Make sure not to leak handles
   // into the caller's handle scope.
   {
     HandleScope scope(isolate());
     Handle<CompilationCacheTable> table = GetTable();
-    MaybeHandle<SharedFunctionInfo> probe =
-        CompilationCacheTable::LookupScript(table, source, isolate());
+    MaybeHandle<SharedFunctionInfo> probe = CompilationCacheTable::LookupScript(
+        table, source, script_details, isolate());
     Handle<SharedFunctionInfo> function_info;
     if (probe.ToHandle(&function_info)) {
-      // Break when we've found a suitable shared function info that
-      // matches the origin.
-      if (HasOrigin(isolate(), function_info, script_details)) {
-        result = scope.CloseAndEscape(function_info);
-      }
+      result = scope.CloseAndEscape(function_info);
     }
   }
 
@@ -235,9 +170,6 @@ MaybeHandle<SharedFunctionInfo> CompilationCacheScript::Lookup(
   // handle created in the caller's handle scope.
   Handle<SharedFunctionInfo> function_info;
   if (result.ToHandle(&function_info)) {
-    // Since HasOrigin can allocate, we need to protect the SharedFunctionInfo
-    // with handles during the call.
-    DCHECK(HasOrigin(isolate(), function_info, script_details));
     isolate()->counters()->compilation_cache_hits()->Increment();
     LOG(isolate(), CompilationCacheEvent("hit", "script", *function_info));
   } else {

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

@@ -64,9 +64,15 @@ class ScriptCacheKey : public HashTableKey {
     kEnd,
   };
 
-  explicit ScriptCacheKey(Handle<String> source);
+  ScriptCacheKey(Handle<String> source, const ScriptDetails* script_details,
+                 Isolate* isolate);
+  ScriptCacheKey(Handle<String> source, MaybeHandle<Object> name,
+                 int line_offset, int column_offset,
+                 v8::ScriptOriginOptions origin_options,
+                 MaybeHandle<Object> host_defined_options, Isolate* isolate);
 
   bool IsMatch(Object other) override;
+  bool MatchesOrigin(Script script);
 
   Handle<Object> AsHandle(Isolate* isolate, Handle<SharedFunctionInfo> shared);
 
@@ -89,6 +95,12 @@ class ScriptCacheKey : public HashTableKey {
 
  private:
   Handle<String> source_;
+  MaybeHandle<Object> name_;
+  int line_offset_;
+  int column_offset_;
+  v8::ScriptOriginOptions origin_options_;
+  MaybeHandle<Object> host_defined_options_;
+  Isolate* isolate_;
 };
 
 uint32_t CompilationCacheShape::RegExpHash(String string, Smi flags) {
@@ -129,16 +141,6 @@ uint32_t CompilationCacheShape::HashForObject(ReadOnlyRoots roots,
   if (object.IsWeakFixedArray()) {
     uint32_t result = static_cast<uint32_t>(Smi::ToInt(
         WeakFixedArray::cast(object).Get(ScriptCacheKey::kHash).ToSmi()));
-#ifdef DEBUG
-    base::Optional<String> script_key =
-        ScriptCacheKey::SourceFromObject(object);
-    if (script_key) {
-      uint32_t source_hash;
-      if (script_key->TryGetHash(&source_hash)) {
-        DCHECK_EQ(result, source_hash);
-      }
-    }
-#endif
     return result;
   }
 

src/objects/compilation-cache-table.cc

@@ -4,6 +4,7 @@
 
 #include "src/objects/compilation-cache-table.h"
 
+#include "src/codegen/script-details.h"
 #include "src/common/assert-scope.h"
 #include "src/objects/compilation-cache-table-inl.h"
 
@@ -223,21 +224,118 @@ class CodeKey : public HashTableKey {
   Handle<SharedFunctionInfo> key_;
 };
 
+Smi ScriptHash(String source, MaybeHandle<Object> maybe_name, int line_offset,
+               int column_offset, v8::ScriptOriginOptions origin_options,
+               Isolate* isolate) {
+  DisallowGarbageCollection no_gc;
+  size_t hash = base::hash_combine(source.EnsureHash());
+  if (Handle<Object> name;
+      maybe_name.ToHandle(&name) && name->IsString(isolate)) {
+    hash =
+        base::hash_combine(hash, String::cast(*name).EnsureHash(), line_offset,
+                           column_offset, origin_options.Flags());
+  }
+  // The upper bits of the hash are discarded so that the value fits in a Smi.
+  return Smi::From31BitPattern(static_cast<int>(hash));
+}
+
 }  // namespace
 
-ScriptCacheKey::ScriptCacheKey(Handle<String> source)
-    : HashTableKey(source->EnsureHash()), source_(source) {
-  // Hash values must fit within a Smi.
-  static_assert(Name::HashBits::kSize <= kSmiValueSize);
-  DCHECK_EQ(
-      static_cast<uint32_t>(Smi::ToInt(Smi::FromInt(static_cast<int>(Hash())))),
-      Hash());
+// We only re-use a cached function for some script source code if the
+// script originates from the same place. This is to avoid issues
+// when reporting errors, etc.
+bool ScriptCacheKey::MatchesOrigin(Script script) {
+  DisallowGarbageCollection no_gc;
+
+  // If the script name isn't set, the boilerplate script should have
+  // an undefined name to have the same origin.
+  Handle<Object> name;
+  if (!name_.ToHandle(&name)) {
+    return script.name().IsUndefined(isolate_);
+  }
+  // Do the fast bailout checks first.
+  if (line_offset_ != script.line_offset()) return false;
+  if (column_offset_ != script.column_offset()) return false;
+  // Check that both names are strings. If not, no match.
+  if (!name->IsString(isolate_) || !script.name().IsString(isolate_))
+    return false;
+  // Are the origin_options same?
+  if (origin_options_.Flags() != script.origin_options().Flags()) {
+    return false;
+  }
+  // Compare the two name strings for equality.
+  if (!String::cast(*name).Equals(String::cast(script.name()))) {
+    return false;
+  }
+
+  // TODO(cbruni, chromium:1244145): Remove once migrated to the context
+  Handle<Object> maybe_host_defined_options;
+  if (!host_defined_options_.ToHandle(&maybe_host_defined_options)) {
+    maybe_host_defined_options = isolate_->factory()->empty_fixed_array();
+  }
+  FixedArray host_defined_options =
+      FixedArray::cast(*maybe_host_defined_options);
+  FixedArray script_options = FixedArray::cast(script.host_defined_options());
+  int length = host_defined_options.length();
+  if (length != script_options.length()) return false;
+
+  for (int i = 0; i < length; i++) {
+    // host-defined options is a v8::PrimitiveArray.
+    DCHECK(host_defined_options.get(i).IsPrimitive());
+    DCHECK(script_options.get(i).IsPrimitive());
+    if (!host_defined_options.get(i).StrictEquals(script_options.get(i))) {
+      return false;
+    }
+  }
+  return true;
+}
+
+ScriptCacheKey::ScriptCacheKey(Handle<String> source,
+                               const ScriptDetails* script_details,
+                               Isolate* isolate)
+    : ScriptCacheKey(source, script_details->name_obj,
+                     script_details->line_offset, script_details->column_offset,
+                     script_details->origin_options,
+                     script_details->host_defined_options, isolate) {}
+
+ScriptCacheKey::ScriptCacheKey(Handle<String> source, MaybeHandle<Object> name,
+                               int line_offset, int column_offset,
+                               v8::ScriptOriginOptions origin_options,
+                               MaybeHandle<Object> host_defined_options,
+                               Isolate* isolate)
+    : HashTableKey(static_cast<uint32_t>(ScriptHash(*source, name, line_offset,
+                                                    column_offset,
+                                                    origin_options, isolate)
+                                             .value())),
+      source_(source),
+      name_(name),
+      line_offset_(line_offset),
+      column_offset_(column_offset),
+      origin_options_(origin_options),
+      host_defined_options_(host_defined_options),
+      isolate_(isolate) {
+  DCHECK(Smi::IsValid(static_cast<int>(Hash())));
 }
 
 bool ScriptCacheKey::IsMatch(Object other) {
   DisallowGarbageCollection no_gc;
-  base::Optional<String> other_source = SourceFromObject(other);
-  return other_source && other_source->Equals(*source_);
+  DCHECK(other.IsWeakFixedArray());
+  WeakFixedArray other_array = WeakFixedArray::cast(other);
+  DCHECK_EQ(other_array.length(), kEnd);
+
+  // A hash check can quickly reject many non-matches, even though this step
+  // isn't strictly necessary.
+  uint32_t other_hash =
+      static_cast<uint32_t>(other_array.Get(kHash).ToSmi().value());
+  if (other_hash != Hash()) return false;
+
+  HeapObject other_script_object;
+  if (!other_array.Get(kWeakScript).GetHeapObjectIfWeak(&other_script_object)) {
+    return false;
+  }
+  Script other_script = Script::cast(other_script_object);
+  String other_source = String::cast(other_script.source());
+  return other_source.Equals(*source_) && MatchesOrigin(other_script);
 }
 
 Handle<Object> ScriptCacheKey::AsHandle(Isolate* isolate,
@@ -254,9 +352,10 @@ Handle<Object> ScriptCacheKey::AsHandle(Isolate* isolate,
 }
 
 MaybeHandle<SharedFunctionInfo> CompilationCacheTable::LookupScript(
-    Handle<CompilationCacheTable> table, Handle<String> src, Isolate* isolate) {
+    Handle<CompilationCacheTable> table, Handle<String> src,
+    const ScriptDetails& script_details, Isolate* isolate) {
   src = String::Flatten(isolate, src);
-  ScriptCacheKey key(src);
+  ScriptCacheKey key(src, &script_details, isolate);
   InternalIndex entry = table->FindEntry(isolate, &key);
   if (entry.is_not_found()) return MaybeHandle<SharedFunctionInfo>();
   DCHECK(table->KeyAt(entry).IsWeakFixedArray());
@@ -303,7 +402,16 @@ Handle<CompilationCacheTable> CompilationCacheTable::PutScript(
     Handle<CompilationCacheTable> cache, Handle<String> src,
     Handle<SharedFunctionInfo> value, Isolate* isolate) {
   src = String::Flatten(isolate, src);
-  ScriptCacheKey key(src);
+  Handle<Script> script = handle(Script::cast(value->script()), isolate);
+  MaybeHandle<Object> script_name;
+  if (script->name().IsString(isolate)) {
+    script_name = handle(script->name(), isolate);
+  }
+  Handle<FixedArray> host_defined_options(script->host_defined_options(),
+                                          isolate);
+  ScriptCacheKey key(src, script_name, script->line_offset(),
+                     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());

src/objects/compilation-cache-table.h

@@ -17,6 +17,8 @@
 namespace v8 {
 namespace internal {
 
+struct ScriptDetails;
+
 class CompilationCacheShape : public BaseShape<HashTableKey*> {
  public:
   static inline bool IsMatch(HashTableKey* key, Object value) {
@@ -84,7 +86,7 @@ class CompilationCacheTable
   // The 'script' cache contains SharedFunctionInfos.
   static MaybeHandle<SharedFunctionInfo> LookupScript(
       Handle<CompilationCacheTable> table, Handle<String> src,
-      Isolate* isolate);
+      const ScriptDetails& script_details, Isolate* isolate);
   static Handle<CompilationCacheTable> PutScript(
       Handle<CompilationCacheTable> cache, Handle<String> src,
       Handle<SharedFunctionInfo> value, Isolate* isolate);

test/cctest/test-serialize.cc

@@ -1811,10 +1811,12 @@ TEST(CodeSerializerPromotedToCompilationCache) {
   CompileScriptAndProduceCache(isolate, src, default_script_details, &cache,
                                v8::ScriptCompiler::kNoCompileOptions);
 
-  DisallowCompilation no_compile_expected(isolate);
-  Handle<SharedFunctionInfo> copy =
-      CompileScript(isolate, src, default_script_details, cache,
-                    v8::ScriptCompiler::kConsumeCodeCache);
+  Handle<SharedFunctionInfo> copy;
+  {
+    DisallowCompilation no_compile_expected(isolate);
+    copy = CompileScript(isolate, src, default_script_details, cache,
+                         v8::ScriptCompiler::kConsumeCodeCache);
+  }
 
   {
     ScriptDetails script_details(src);
@@ -1915,6 +1917,36 @@ TEST(CodeSerializerPromotedToCompilationCache) {
     CHECK(shared.is_null());
   }
 
+  // Compile the script again with different options.
+  ScriptDetails alternative_script_details(src);
+  Handle<SharedFunctionInfo> alternative_toplevel_sfi =
+      Compiler::GetSharedFunctionInfoForScript(
+          isolate, src, alternative_script_details,
+          ScriptCompiler::kNoCompileOptions, ScriptCompiler::kNoCacheNoReason,
+          NOT_NATIVES_CODE)
+          .ToHandleChecked();
+  CHECK_NE(*copy, *alternative_toplevel_sfi);
+
+  {
+    // The original script can still be found.
+    ScriptDetails script_details(src);
+    script_details.host_defined_options =
+        default_script_details.host_defined_options;
+    MaybeHandle<SharedFunctionInfo> shared =
+        isolate->compilation_cache()->LookupScript(src, script_details,
+                                                   LanguageMode::kSloppy);
+    CHECK_EQ(*shared.ToHandleChecked(), *copy);
+  }
+
+  {
+    // The new script can also be found.
+    ScriptDetails script_details(src);
+    MaybeHandle<SharedFunctionInfo> shared =
+        isolate->compilation_cache()->LookupScript(src, script_details,
+                                                   LanguageMode::kSloppy);
+    CHECK_EQ(*shared.ToHandleChecked(), *alternative_toplevel_sfi);
+  }
+
   delete cache;
 }