[SFI] Always store function_literal_id in SFI.

Calling FindIndexInScript performs a linear search on the script functions and can
take considerable time. With Bytecode flushing we will lose the function_literal_id
and have to call FindIndexInScript if we ever recompile the flushed function. This
can take a significant proportion of the recompilation time and has caused regressions
in rendering times for some web applications (e.g, 395ms in FindIndexInScript for 132ms
spent lazily re-compiling code).

To avoid this, add function_literal_id back into the SFI and remove it from
UnoptimizedCompileInfo. This will slightly regress memory usage (particularly
in cases where many of the SFIs are compiled), however it means we can remove
the FindIndexInScript function and avoid these long-tail regressions when
bytecode is flushed.

BUG=chromium:965833

Change-Id: Ia31e82eb6c871a6d698a518326a8555822a7a1d8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1669700Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62319}

src/builtins/base.tq

@@ -571,6 +571,7 @@ extern class SharedFunctionInfo extends HeapObject {
   expected_nof_properties: uint16;
   function_token_offset: int16;
   flags: int32;
+  function_literal_id: int32;
   @if(V8_SFI_HAS_UNIQUE_ID) unique_id: int32;
 }
 

src/debug/liveedit.cc

@@ -1119,13 +1119,11 @@ void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
     UpdatePositions(isolate, sfi, diffs);
 
     sfi->set_script(*new_script);
-    if (sfi->HasUncompiledData()) {
-      sfi->uncompiled_data().set_function_literal_id(
-          mapping.second->function_literal_id());
-    }
+    sfi->set_function_literal_id(mapping.second->function_literal_id());
     new_script->shared_function_infos().Set(
         mapping.second->function_literal_id(), HeapObjectReference::Weak(*sfi));
-    DCHECK_EQ(sfi->FunctionLiteralId(), mapping.second->function_literal_id());
+    DCHECK_EQ(sfi->function_literal_id(),
+              mapping.second->function_literal_id());
 
     // Save the new start_position -> id mapping, so that we can recover it when
     // iterating over changed functions' constant pools.
@@ -1222,7 +1220,7 @@ void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
     std::set<int> start_positions;
     for (SharedFunctionInfo sfi = it.Next(); !sfi.is_null(); sfi = it.Next()) {
       DCHECK_EQ(sfi.script(), *new_script);
-      DCHECK_EQ(sfi.FunctionLiteralId(), it.CurrentIndex());
+      DCHECK_EQ(sfi.function_literal_id(), it.CurrentIndex());
       // Don't check the start position of the top-level function, as it can
       // overlap with a function in the script.
       if (sfi.is_toplevel()) {
@@ -1242,7 +1240,7 @@ void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
             SharedFunctionInfo::cast(constants.get(i));
         DCHECK_EQ(inner_sfi.script(), *new_script);
         DCHECK_EQ(inner_sfi, new_script->shared_function_infos()
-                                 .Get(inner_sfi.FunctionLiteralId())
+                                 .Get(inner_sfi.function_literal_id())
                                  ->GetHeapObject());
       }
     }

src/heap/factory.cc

@@ -2583,15 +2583,14 @@ Handle<PreparseData> Factory::NewPreparseData(int data_length,
 Handle<UncompiledDataWithoutPreparseData>
 Factory::NewUncompiledDataWithoutPreparseData(Handle<String> inferred_name,
                                               int32_t start_position,
-                                              int32_t end_position,
-                                              int32_t function_literal_id) {
+                                              int32_t end_position) {
   Handle<UncompiledDataWithoutPreparseData> result(
       UncompiledDataWithoutPreparseData::cast(New(
           uncompiled_data_without_preparse_data_map(), AllocationType::kOld)),
       isolate());
 
   UncompiledData::Initialize(*result, *inferred_name, start_position,
-                             end_position, function_literal_id);
+                             end_position);
   return result;
 }
 
@@ -2599,7 +2598,6 @@ Handle<UncompiledDataWithPreparseData>
 Factory::NewUncompiledDataWithPreparseData(Handle<String> inferred_name,
                                            int32_t start_position,
                                            int32_t end_position,
-                                           int32_t function_literal_id,
                                            Handle<PreparseData> preparse_data) {
   Handle<UncompiledDataWithPreparseData> result(
       UncompiledDataWithPreparseData::cast(
@@ -2607,8 +2605,7 @@ Factory::NewUncompiledDataWithPreparseData(Handle<String> inferred_name,
       isolate());
 
   UncompiledDataWithPreparseData::Initialize(
-      *result, *inferred_name, start_position, end_position,
-      function_literal_id, *preparse_data);
+      *result, *inferred_name, start_position, end_position, *preparse_data);
 
   return result;
 }

src/heap/factory.h

@@ -776,12 +776,11 @@ class V8_EXPORT_PRIVATE Factory {
   Handle<UncompiledDataWithoutPreparseData>
   NewUncompiledDataWithoutPreparseData(Handle<String> inferred_name,
                                        int32_t start_position,
-                                       int32_t end_position,
-                                       int32_t function_literal_id);
+                                       int32_t end_position);
 
   Handle<UncompiledDataWithPreparseData> NewUncompiledDataWithPreparseData(
       Handle<String> inferred_name, int32_t start_position,
-      int32_t end_position, int32_t function_literal_id, Handle<PreparseData>);
+      int32_t end_position, Handle<PreparseData>);
 
   // Create an External object for V8's external API.
   Handle<JSObject> NewExternal(void* value);

src/heap/mark-compact.cc

@@ -2093,7 +2093,6 @@ void MarkCompactCollector::FlushBytecodeFromSFI(
   UncompiledData uncompiled_data = UncompiledData::cast(compiled_data);
   UncompiledData::Initialize(
       uncompiled_data, inferred_name, start_position, end_position,
-      kFunctionLiteralIdInvalid,
       [](HeapObject object, ObjectSlot slot, HeapObject target) {
         RecordSlot(object, slot, target);
       });

src/objects/objects.cc

@@ -4862,7 +4862,7 @@ const char* SharedFunctionInfo::kTraceScope =
 
 uint64_t SharedFunctionInfo::TraceID(FunctionLiteral* literal) const {
   int literal_id =
-      literal ? literal->function_literal_id() : FunctionLiteralId();
+      literal ? literal->function_literal_id() : function_literal_id();
   Script script = Script::cast(this->script());
   return (static_cast<uint64_t>(script.id() + 1) << 32) |
          (static_cast<uint64_t>(literal_id));
@@ -5141,7 +5141,6 @@ void SharedFunctionInfo::DiscardCompiled(
       handle(shared_info->inferred_name(), isolate);
   int start_position = shared_info->StartPosition();
   int end_position = shared_info->EndPosition();
-  int function_literal_id = shared_info->FunctionLiteralId();
 
   shared_info->DiscardCompiledMetadata(isolate);
 
@@ -5156,8 +5155,7 @@ void SharedFunctionInfo::DiscardCompiled(
     // validity checks, since we're performing the unusual task of decompiling.
     Handle<UncompiledData> data =
         isolate->factory()->NewUncompiledDataWithoutPreparseData(
-            inferred_name_val, start_position, end_position,
-            function_literal_id);
+            inferred_name_val, start_position, end_position);
     shared_info->set_function_data(*data);
   }
 }
@@ -5266,27 +5264,6 @@ bool SharedFunctionInfo::IsInlineable() {
 
 int SharedFunctionInfo::SourceSize() { return EndPosition() - StartPosition(); }
 
-int SharedFunctionInfo::FindIndexInScript() const {
-  DisallowHeapAllocation no_gc;
-
-  Object script_obj = script();
-  if (!script_obj.IsScript()) return kFunctionLiteralIdInvalid;
-
-  WeakFixedArray shared_info_list =
-      Script::cast(script_obj).shared_function_infos();
-  SharedFunctionInfo::ScriptIterator iterator(
-      Handle<WeakFixedArray>(reinterpret_cast<Address*>(&shared_info_list)));
-
-  for (SharedFunctionInfo shared = iterator.Next(); !shared.is_null();
-       shared = iterator.Next()) {
-    if (shared == *this) {
-      return iterator.CurrentIndex();
-    }
-  }
-
-  return kFunctionLiteralIdInvalid;
-}
-
 // Output the source code without any allocation in the heap.
 std::ostream& operator<<(std::ostream& os, const SourceCodeOf& v) {
   const SharedFunctionInfo s = v.value;
@@ -5357,6 +5334,7 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
   shared_info->set_allows_lazy_compilation(lit->AllowsLazyCompilation());
   shared_info->set_language_mode(lit->language_mode());
   shared_info->set_is_wrapped(lit->is_wrapped());
+  shared_info->set_function_literal_id(lit->function_literal_id());
   //  shared_info->set_kind(lit->kind());
   // FunctionKind must have already been set.
   DCHECK(lit->kind() == shared_info->kind());
@@ -5401,7 +5379,7 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
       Handle<UncompiledData> data =
           isolate->factory()->NewUncompiledDataWithPreparseData(
               lit->inferred_name(), lit->start_position(), lit->end_position(),
-              lit->function_literal_id(), preparse_data);
+              preparse_data);
       shared_info->set_uncompiled_data(*data);
       needs_position_info = false;
     }
@@ -5410,8 +5388,7 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
   if (needs_position_info) {
     Handle<UncompiledData> data =
         isolate->factory()->NewUncompiledDataWithoutPreparseData(
-            lit->inferred_name(), lit->start_position(), lit->end_position(),
-            lit->function_literal_id());
+            lit->inferred_name(), lit->start_position(), lit->end_position());
     shared_info->set_uncompiled_data(*data);
   }
 }
@@ -5502,21 +5479,6 @@ int SharedFunctionInfo::EndPosition() const {
   return kNoSourcePosition;
 }
 
-int SharedFunctionInfo::FunctionLiteralId() const {
-  // Fast path for the common case when the SFI is uncompiled and so the
-  // function literal id is already in the uncompiled data.
-  if (HasUncompiledData() && uncompiled_data().has_function_literal_id()) {
-    int id = uncompiled_data().function_literal_id();
-    // Make sure the id is what we should have found with the slow path.
-    DCHECK_EQ(id, FindIndexInScript());
-    return id;
-  }
-
-  // Otherwise, search for the function in the SFI's script's function list,
-  // and return its index in that list.
-  return FindIndexInScript();
-}
-
 void SharedFunctionInfo::SetPosition(int start_position, int end_position) {
   Object maybe_scope_info = name_or_scope_info();
   if (maybe_scope_info.IsScopeInfo()) {

src/objects/shared-function-info-inl.h

@@ -91,7 +91,6 @@ CAST_ACCESSOR(UncompiledData)
 ACCESSORS(UncompiledData, inferred_name, String, kInferredNameOffset)
 INT32_ACCESSORS(UncompiledData, start_position, kStartPositionOffset)
 INT32_ACCESSORS(UncompiledData, end_position, kEndPositionOffset)
-INT32_ACCESSORS(UncompiledData, function_literal_id, kFunctionLiteralIdOffset)
 
 void UncompiledData::clear_padding() {
   if (FIELD_SIZE(kOptionalPaddingOffset) == 0) return;
@@ -128,6 +127,9 @@ ACCESSORS(SharedFunctionInfo, name_or_scope_info, Object,
 ACCESSORS(SharedFunctionInfo, script_or_debug_info, Object,
           kScriptOrDebugInfoOffset)
 
+INT32_ACCESSORS(SharedFunctionInfo, function_literal_id,
+                kFunctionLiteralIdOffset)
+
 #if V8_SFI_HAS_UNIQUE_ID
 INT_ACCESSORS(SharedFunctionInfo, unique_id, kUniqueIdOffset)
 #endif
@@ -629,7 +631,7 @@ void SharedFunctionInfo::ClearPreparseData() {
 // static
 void UncompiledData::Initialize(
     UncompiledData data, String inferred_name, int start_position,
-    int end_position, int function_literal_id,
+    int end_position,
     std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
         gc_notify_updated_slot) {
   data.set_inferred_name(inferred_name);
@@ -637,28 +639,22 @@ void UncompiledData::Initialize(
       data, data.RawField(UncompiledData::kInferredNameOffset), inferred_name);
   data.set_start_position(start_position);
   data.set_end_position(end_position);
-  data.set_function_literal_id(function_literal_id);
   data.clear_padding();
 }
 
 void UncompiledDataWithPreparseData::Initialize(
     UncompiledDataWithPreparseData data, String inferred_name,
-    int start_position, int end_position, int function_literal_id,
-    PreparseData scope_data,
+    int start_position, int end_position, PreparseData scope_data,
     std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
         gc_notify_updated_slot) {
   UncompiledData::Initialize(data, inferred_name, start_position, end_position,
-                             function_literal_id, gc_notify_updated_slot);
+                             gc_notify_updated_slot);
   data.set_preparse_data(scope_data);
   gc_notify_updated_slot(
       data, data.RawField(UncompiledDataWithPreparseData::kPreparseDataOffset),
       scope_data);
 }
 
-bool UncompiledData::has_function_literal_id() {
-  return function_literal_id() != kFunctionLiteralIdInvalid;
-}
-
 bool SharedFunctionInfo::HasWasmExportedFunctionData() const {
   return function_data().IsWasmExportedFunctionData();
 }

src/objects/shared-function-info.h

@@ -104,16 +104,12 @@ class UncompiledData : public HeapObject {
   DECL_ACCESSORS(inferred_name, String)
   DECL_INT32_ACCESSORS(start_position)
   DECL_INT32_ACCESSORS(end_position)
-  DECL_INT32_ACCESSORS(function_literal_id)
-
-  // Returns true if the UncompiledData contains a valid function_literal_id.
-  inline bool has_function_literal_id();
 
   DECL_CAST(UncompiledData)
 
   inline static void Initialize(
       UncompiledData data, String inferred_name, int start_position,
-      int end_position, int function_literal_id,
+      int end_position,
       std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
           gc_notify_updated_slot =
               [](HeapObject object, ObjectSlot slot, HeapObject target) {});
@@ -126,7 +122,6 @@ class UncompiledData : public HeapObject {
   /* Raw data fields. */                                                  \
   V(kStartPositionOffset, kInt32Size)                                     \
   V(kEndPositionOffset, kInt32Size)                                       \
-  V(kFunctionLiteralIdOffset, kInt32Size)                                 \
   V(kOptionalPaddingOffset, POINTER_SIZE_PADDING(kOptionalPaddingOffset)) \
   /* Header size. */                                                      \
   V(kSize, 0)
@@ -172,8 +167,7 @@ class UncompiledDataWithPreparseData : public UncompiledData {
 
   inline static void Initialize(
       UncompiledDataWithPreparseData data, String inferred_name,
-      int start_position, int end_position, int function_literal_id,
-      PreparseData scope_data,
+      int start_position, int end_position, PreparseData scope_data,
       std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
           gc_notify_updated_slot =
               [](HeapObject object, ObjectSlot slot, HeapObject target) {});
@@ -316,6 +310,11 @@ class SharedFunctionInfo : public HeapObject {
   // function. The value is only reliable when the function has been compiled.
   DECL_UINT16_ACCESSORS(expected_nof_properties)
 
+  // [function_literal_id] - uniquely identifies the FunctionLiteral this
+  // SharedFunctionInfo represents within its script, or -1 if this
+  // SharedFunctionInfo object doesn't correspond to a parsed FunctionLiteral.
+  DECL_INT32_ACCESSORS(function_literal_id)
+
 #if V8_SFI_HAS_UNIQUE_ID
   // [unique_id] - For --trace-maps purposes, an identifier that's persistent
   // even if the GC moves this SharedFunctionInfo.
@@ -385,9 +384,6 @@ class SharedFunctionInfo : public HeapObject {
   inline bool HasInferredName();
   inline String inferred_name();
 
-  // Get the function literal id associated with this function, for parsing.
-  V8_EXPORT_PRIVATE int FunctionLiteralId() const;
-
   // Break infos are contained in DebugInfo, this is a convenience method
   // to simplify access.
   V8_EXPORT_PRIVATE bool HasBreakInfo() const;
@@ -743,10 +739,6 @@ class SharedFunctionInfo : public HeapObject {
   friend class V8HeapExplorer;
   FRIEND_TEST(PreParserTest, LazyFunctionLength);
 
-  // Find the index of this function in the parent script. Slow path of
-  // FunctionLiteralId.
-  int FindIndexInScript() const;
-
   OBJECT_CONSTRUCTORS(SharedFunctionInfo, HeapObject);
 };
 

src/parsing/parse-info.cc

@@ -97,7 +97,7 @@ ParseInfo::ParseInfo(Isolate* isolate, Handle<SharedFunctionInfo> shared)
 
   set_start_position(shared->StartPosition());
   set_end_position(shared->EndPosition());
-  function_literal_id_ = shared->FunctionLiteralId();
+  function_literal_id_ = shared->function_literal_id();
   SetFunctionInfo(shared);
 
   Handle<Script> script(Script::cast(shared->script()), isolate);

test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc

@@ -94,7 +94,7 @@ class CompilerDispatcherTest : public TestWithNativeContext {
             FunctionLiteral::kNoDuplicateParameters,
             FunctionLiteral::kAnonymousExpression,
             FunctionLiteral::kShouldEagerCompile, shared->StartPosition(), true,
-            shared->FunctionLiteralId(), nullptr);
+            shared->function_literal_id(), nullptr);
 
     return dispatcher->Enqueue(outer_parse_info.get(), function_name,
                                function_literal);

test/unittests/tasks/background-compile-task-unittest.cc

@@ -73,7 +73,7 @@ class BackgroundCompileTaskTest : public TestWithNativeContext {
             FunctionLiteral::kNoDuplicateParameters,
             FunctionLiteral::kAnonymousExpression,
             FunctionLiteral::kShouldEagerCompile, shared->StartPosition(), true,
-            shared->FunctionLiteralId(), nullptr);
+            shared->function_literal_id(), nullptr);
 
     return new BackgroundCompileTask(
         allocator(), outer_parse_info.get(), function_name, function_literal,

test/unittests/test-helpers.cc

@@ -42,11 +42,11 @@ Handle<SharedFunctionInfo> CreateSharedFunctionInfo(
           isolate->factory()->NewStringFromAsciiChecked("f"),
           Builtins::kCompileLazy);
   int function_literal_id = 1;
+  shared->set_function_literal_id(function_literal_id);
   // Ensure that the function can be compiled lazily.
   shared->set_uncompiled_data(
       *isolate->factory()->NewUncompiledDataWithoutPreparseData(
-          ReadOnlyRoots(isolate).empty_string_handle(), 0, source->length(),
-          function_literal_id));
+          ReadOnlyRoots(isolate).empty_string_handle(), 0, source->length()));
   // Make sure we have an outer scope info, even though it's empty
   shared->set_raw_outer_scope_info_or_feedback_metadata(
       ScopeInfo::Empty(isolate));