[objects] Move SFI init from factory to Init method

After allocation of an object, we need to initialize it to make it safe
for the GC to see it. For complex objects like SharedFunctionInfo, this
initialization code is long and requires understanding of the object. So,
it makes sense for the initialization to live in the SharedFunctionInfo
code itself (as an Init method) rather than in the factory.

Aside from being a neat cleanup, this will allow us to share this
initialization logic between different allocation methods, as part of the
off-thread allocation project:
https://docs.google.com/document/d/1-_96kok0AcavkbcdqqZvpqt_2q-_XWAsAJwbRXlfwCo/

Bug: chromium:1011762
Change-Id: Ie276eb711423272f85abfeb3d88df1826a77b984
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1872402
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64926}

src/heap/factory.cc

@@ -56,6 +56,7 @@
 #include "src/objects/struct-inl.h"
 #include "src/objects/template-objects-inl.h"
 #include "src/objects/transitions-inl.h"
+#include "src/roots/roots.h"
 #include "src/strings/unicode-inl.h"
 
 namespace v8 {
@@ -3441,70 +3442,59 @@ Handle<SharedFunctionInfo> Factory::NewSharedFunctionInfoForBuiltin(
 Handle<SharedFunctionInfo> Factory::NewSharedFunctionInfo(
     MaybeHandle<String> maybe_name, MaybeHandle<HeapObject> maybe_function_data,
     int maybe_builtin_index, FunctionKind kind) {
+  Handle<SharedFunctionInfo> shared = NewSharedFunctionInfo();
+
   // Function names are assumed to be flat elsewhere. Must flatten before
   // allocating SharedFunctionInfo to avoid GC seeing the uninitialized SFI.
   Handle<String> shared_name;
   bool has_shared_name = maybe_name.ToHandle(&shared_name);
   if (has_shared_name) {
     shared_name = String::Flatten(isolate(), shared_name, AllocationType::kOld);
+    shared->set_name_or_scope_info(*shared_name);
+  } else {
+    DCHECK_EQ(shared->name_or_scope_info(),
+              SharedFunctionInfo::kNoSharedNameSentinel);
+  }
+
+  Handle<HeapObject> function_data;
+  if (maybe_function_data.ToHandle(&function_data)) {
+    // If we pass function_data then we shouldn't pass a builtin index, and
+    // the function_data should not be code with a builtin.
+    DCHECK(!Builtins::IsBuiltinId(maybe_builtin_index));
+    DCHECK_IMPLIES(function_data->IsCode(),
+                   !Code::cast(*function_data).is_builtin());
+    shared->set_function_data(*function_data);
+  } else if (Builtins::IsBuiltinId(maybe_builtin_index)) {
+    shared->set_builtin_id(maybe_builtin_index);
+  } else {
+    shared->set_builtin_id(Builtins::kIllegal);
   }
 
+  shared->CalculateConstructAsBuiltin();
+  shared->set_kind(kind);
+
+#ifdef VERIFY_HEAP
+  shared->SharedFunctionInfoVerify(isolate());
+#endif  // VERIFY_HEAP
+  return shared;
+}
+
+Handle<SharedFunctionInfo> Factory::NewSharedFunctionInfo() {
   Handle<Map> map = shared_function_info_map();
-  Handle<SharedFunctionInfo> share(
+
+  Handle<SharedFunctionInfo> shared(
       SharedFunctionInfo::cast(New(map, AllocationType::kOld)), isolate());
-  {
-    DisallowHeapAllocation no_allocation;
-
-    // Set pointer fields.
-    share->set_name_or_scope_info(
-        has_shared_name ? Object::cast(*shared_name)
-                        : SharedFunctionInfo::kNoSharedNameSentinel);
-    Handle<HeapObject> function_data;
-    if (maybe_function_data.ToHandle(&function_data)) {
-      // If we pass function_data then we shouldn't pass a builtin index, and
-      // the function_data should not be code with a builtin.
-      DCHECK(!Builtins::IsBuiltinId(maybe_builtin_index));
-      DCHECK_IMPLIES(function_data->IsCode(),
-                     !Code::cast(*function_data).is_builtin());
-      share->set_function_data(*function_data);
-    } else if (Builtins::IsBuiltinId(maybe_builtin_index)) {
-      share->set_builtin_id(maybe_builtin_index);
-    } else {
-      share->set_builtin_id(Builtins::kIllegal);
-    }
-    // Generally functions won't have feedback, unless they have been created
-    // from a FunctionLiteral. Those can just reset this field to keep the
-    // SharedFunctionInfo in a consistent state.
-    if (maybe_builtin_index == Builtins::kCompileLazy) {
-      share->set_raw_outer_scope_info_or_feedback_metadata(*the_hole_value(),
-                                                           SKIP_WRITE_BARRIER);
-    } else {
-      share->set_raw_outer_scope_info_or_feedback_metadata(
-          *empty_feedback_metadata(), SKIP_WRITE_BARRIER);
-    }
-    share->set_script_or_debug_info(*undefined_value(), SKIP_WRITE_BARRIER);
-    share->set_function_literal_id(kFunctionLiteralIdInvalid);
+  int unique_id = -1;
 #if V8_SFI_HAS_UNIQUE_ID
-    share->set_unique_id(isolate()->GetNextUniqueSharedFunctionInfoId());
-#endif
+  unique_id = isolate()->GetNextUniqueSharedFunctionInfoId();
+#endif  // V8_SFI_HAS_UNIQUE_ID
 
-    // Set integer fields (smi or int, depending on the architecture).
-    share->set_length(0);
-    share->set_internal_formal_parameter_count(0);
-    share->set_expected_nof_properties(0);
-    share->set_raw_function_token_offset(0);
-    // All flags default to false or 0.
-    share->set_flags(0);
-    share->CalculateConstructAsBuiltin();
-    share->set_kind(kind);
-
-    share->clear_padding();
-  }
+  shared->Init(ReadOnlyRoots(isolate()), unique_id);
 
 #ifdef VERIFY_HEAP
-  share->SharedFunctionInfoVerify(isolate());
-#endif
-  return share;
+  shared->SharedFunctionInfoVerify(isolate());
+#endif  // VERIFY_HEAP
+  return shared;
 }
 
 namespace {

src/heap/factory.h

@@ -1077,6 +1077,7 @@ class V8_EXPORT_PRIVATE Factory {
       ElementsKind elements_kind, int capacity,
       ArrayStorageAllocationMode mode = DONT_INITIALIZE_ARRAY_ELEMENTS);
 
+  Handle<SharedFunctionInfo> NewSharedFunctionInfo();
   Handle<SharedFunctionInfo> NewSharedFunctionInfo(
       MaybeHandle<String> name, MaybeHandle<HeapObject> maybe_function_data,
       int maybe_builtin_index, FunctionKind kind = kNormalFunction);

src/objects/objects.cc

@@ -66,6 +66,7 @@
 #include "src/objects/map-updater.h"
 #include "src/objects/objects-body-descriptors-inl.h"
 #include "src/objects/property-details.h"
+#include "src/roots/roots.h"
 #include "src/utils/identity-map.h"
 #ifdef V8_INTL_SUPPORT
 #include "src/objects/js-break-iterator.h"
@@ -4824,6 +4825,44 @@ uint32_t SharedFunctionInfo::Hash() {
   return static_cast<uint32_t>(base::hash_combine(start_pos, script_id));
 }
 
+void SharedFunctionInfo::Init(ReadOnlyRoots ro_roots, int unique_id) {
+  DisallowHeapAllocation no_allocation;
+
+  // Set the function data to the "illegal" builtin. Ideally we'd use some sort
+  // of "uninitialized" marker here, but it's cheaper to use a valid buitin and
+  // avoid having to do uninitialized checks elsewhere.
+  set_builtin_id(Builtins::kIllegal);
+
+  // Set the name to the no-name sentinel, this can be updated later.
+  set_name_or_scope_info(SharedFunctionInfo::kNoSharedNameSentinel,
+                         SKIP_WRITE_BARRIER);
+
+  // Generally functions won't have feedback, unless they have been created
+  // from a FunctionLiteral. Those can just reset this field to keep the
+  // SharedFunctionInfo in a consistent state.
+  set_raw_outer_scope_info_or_feedback_metadata(ro_roots.the_hole_value(),
+                                                SKIP_WRITE_BARRIER);
+  set_script_or_debug_info(ro_roots.undefined_value(), SKIP_WRITE_BARRIER);
+  set_function_literal_id(kFunctionLiteralIdInvalid);
+#if V8_SFI_HAS_UNIQUE_ID
+  set_unique_id(unique_id);
+#endif
+
+  // Set integer fields (smi or int, depending on the architecture).
+  set_length(0);
+  set_internal_formal_parameter_count(0);
+  set_expected_nof_properties(0);
+  set_raw_function_token_offset(0);
+
+  // All flags default to false or 0, except ConstructAsBuiltinBit just because
+  // we're using the kIllegal builtin.
+  set_flags(ConstructAsBuiltinBit::encode(true));
+
+  UpdateFunctionMapIndex();
+
+  clear_padding();
+}
+
 Code SharedFunctionInfo::GetCode() const {
   // ======
   // NOTE: This chain of checks MUST be kept in sync with the equivalent CSA

src/objects/shared-function-info.h

@@ -169,6 +169,13 @@ class SharedFunctionInfo : public HeapObject {
  public:
   NEVER_READ_ONLY_SPACE
 
+  // This initializes the SharedFunctionInfo after allocation. It must
+  // initialize all fields, and leave the SharedFunctionInfo in a state where
+  // it is safe for the GC to visit it.
+  //
+  // Important: This function MUST not allocate.
+  void Init(ReadOnlyRoots roots, int unique_id);
+
   V8_EXPORT_PRIVATE static constexpr Object const kNoSharedNameSentinel =
       Smi::kZero;