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

This reverts commit c443858f.

Reason for revert: Several UBSan failures: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20UBSan/21547/overview

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: I6d007374fb607a2670ca260c6bd0d6774d7f51d7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3687311
Auto-Submit: Deepti Gandluri <gdeepti@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#80922}

src/codegen/compilation-cache.cc

@@ -4,6 +4,7 @@
 
 #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"
@@ -148,20 +149,84 @@ 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 table. Make sure not to leak handles
+  // Probe the script generation tables. 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, script_details, isolate());
+    MaybeHandle<SharedFunctionInfo> probe =
+        CompilationCacheTable::LookupScript(table, source, isolate());
     Handle<SharedFunctionInfo> function_info;
     if (probe.ToHandle(&function_info)) {
-      result = scope.CloseAndEscape(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);
+      }
     }
   }
 
@@ -170,6 +235,9 @@ 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,15 +64,9 @@ class ScriptCacheKey : public HashTableKey {
     kEnd,
   };
 
-  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);
+  explicit ScriptCacheKey(Handle<String> source);
 
   bool IsMatch(Object other) override;
-  bool MatchesOrigin(Script script);
 
   Handle<Object> AsHandle(Isolate* isolate, Handle<SharedFunctionInfo> shared);
 
@@ -95,12 +89,6 @@ 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) {
@@ -141,6 +129,16 @@ 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,7 +4,6 @@
 
 #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"
 
@@ -224,118 +223,21 @@ 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
 
-// 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())));
+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());
 }
 
 bool ScriptCacheKey::IsMatch(Object other) {
   DisallowGarbageCollection no_gc;
-  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);
+  base::Optional<String> other_source = SourceFromObject(other);
+  return other_source && other_source->Equals(*source_);
 }
 
 Handle<Object> ScriptCacheKey::AsHandle(Isolate* isolate,
@@ -352,10 +254,9 @@ Handle<Object> ScriptCacheKey::AsHandle(Isolate* isolate,
 }
 
 MaybeHandle<SharedFunctionInfo> CompilationCacheTable::LookupScript(
-    Handle<CompilationCacheTable> table, Handle<String> src,
-    const ScriptDetails& script_details, Isolate* isolate) {
+    Handle<CompilationCacheTable> table, Handle<String> src, Isolate* isolate) {
   src = String::Flatten(isolate, src);
-  ScriptCacheKey key(src, &script_details, isolate);
+  ScriptCacheKey key(src);
   InternalIndex entry = table->FindEntry(isolate, &key);
   if (entry.is_not_found()) return MaybeHandle<SharedFunctionInfo>();
   DCHECK(table->KeyAt(entry).IsWeakFixedArray());
@@ -402,16 +303,7 @@ Handle<CompilationCacheTable> CompilationCacheTable::PutScript(
     Handle<CompilationCacheTable> cache, Handle<String> src,
     Handle<SharedFunctionInfo> value, Isolate* isolate) {
   src = String::Flatten(isolate, 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);
+  ScriptCacheKey key(src);
   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,8 +17,6 @@
 namespace v8 {
 namespace internal {
 
-struct ScriptDetails;
-
 class CompilationCacheShape : public BaseShape<HashTableKey*> {
  public:
   static inline bool IsMatch(HashTableKey* key, Object value) {
@@ -86,7 +84,7 @@ class CompilationCacheTable
   // The 'script' cache contains SharedFunctionInfos.
   static MaybeHandle<SharedFunctionInfo> LookupScript(
       Handle<CompilationCacheTable> table, Handle<String> src,
-      const ScriptDetails& script_details, Isolate* isolate);
+      Isolate* isolate);
   static Handle<CompilationCacheTable> PutScript(
       Handle<CompilationCacheTable> cache, Handle<String> src,
       Handle<SharedFunctionInfo> value, Isolate* isolate);

test/cctest/test-serialize.cc

@@ -1811,12 +1811,10 @@ TEST(CodeSerializerPromotedToCompilationCache) {
   CompileScriptAndProduceCache(isolate, src, default_script_details, &cache,
                                v8::ScriptCompiler::kNoCompileOptions);
 
-  Handle<SharedFunctionInfo> copy;
-  {
-    DisallowCompilation no_compile_expected(isolate);
-    copy = CompileScript(isolate, src, default_script_details, cache,
-                         v8::ScriptCompiler::kConsumeCodeCache);
-  }
+  DisallowCompilation no_compile_expected(isolate);
+  Handle<SharedFunctionInfo> copy =
+      CompileScript(isolate, src, default_script_details, cache,
+                    v8::ScriptCompiler::kConsumeCodeCache);
 
   {
     ScriptDetails script_details(src);
@@ -1917,36 +1915,6 @@ 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;
 }