Reland "Allow lookup of matching scripts in Isolate compilation cache"

This is a reland of commit c443858f

The original version included an operation which could left-shift
signed values, which is undefined behavior; the updated version masks
the value first to avoid the problem.

Original change's description:
> 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/+/3671615
> Reviewed-by: Camillo Bruni <cbruni@chromium.org>
> Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#80919}

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

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 & (~(1u << 31))));
+}
+
 }  // 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;
 }