Tweak how v8 preallocates instance fields

Currently v8 ignores class instance fields when determining how many
properties to preallocate for a given function. This cl changes v8's
behavior to start preallocating for instance fields in addition to
properties.

Bug: v8:8774
Change-Id: If598c2ba8a1b14bd0293f36bae7d35e2d85f7898
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1560216
Commit-Queue: Joshua Litt <joshualitt@google.com>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60882}

src/ast/ast.h

@@ -2237,11 +2237,10 @@ class FunctionLiteral final : public Expression {
 
   static bool NeedsHomeObject(Expression* expr);
 
-  int expected_property_count() {
-    // Not valid for lazy functions.
-    DCHECK(ShouldEagerCompile());
-    return expected_property_count_;
+  void add_expected_properties(int number_properties) {
+    expected_property_count_ += number_properties;
   }
+  int expected_property_count() { return expected_property_count_; }
   int parameter_count() { return parameter_count_; }
   int function_length() { return function_length_; }
 
@@ -2394,6 +2393,8 @@ class FunctionLiteral final : public Expression {
       : public BitField<bool, RequiresInstanceMembersInitializer::kNext, 1> {};
   class OneshotIIFEBit : public BitField<bool, HasBracesField::kNext, 1> {};
 
+  // expected_property_count_ is the sum of instance fields and properties.
+  // It can vary depending on whether a function is lazily or eagerly parsed.
   int expected_property_count_;
   int parameter_count_;
   int function_length_;

src/compiler.cc

@@ -444,7 +444,7 @@ void SetSharedFunctionFlagsFromLiteral(FunctionLiteral* literal,
   shared_info->set_has_duplicate_parameters(
       literal->has_duplicate_parameters());
   shared_info->set_is_oneshot_iife(literal->is_oneshot_iife());
-  shared_info->SetExpectedNofPropertiesFromEstimate(literal);
+  shared_info->UpdateAndFinalizeExpectedNofPropertiesFromEstimate(literal);
   if (literal->dont_optimize_reason() != BailoutReason::kNoReason) {
     shared_info->DisableOptimization(literal->dont_optimize_reason());
   }

src/objects.cc

@@ -5504,7 +5504,7 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
   // really parsed and compiled.
   if (lit->ShouldEagerCompile()) {
     shared_info->set_has_duplicate_parameters(lit->has_duplicate_parameters());
-    shared_info->SetExpectedNofPropertiesFromEstimate(lit);
+    shared_info->UpdateAndFinalizeExpectedNofPropertiesFromEstimate(lit);
     shared_info->set_is_safe_to_skip_arguments_adaptor(
         lit->SafeToSkipArgumentsAdaptor());
     DCHECK_NULL(lit->produced_preparse_data());
@@ -5526,6 +5526,7 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
       shared_info->set_uncompiled_data(*data);
       needs_position_info = false;
     }
+    shared_info->UpdateExpectedNofPropertiesFromEstimate(lit);
   }
   if (needs_position_info) {
     Handle<UncompiledData> data =
@@ -5536,10 +5537,30 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
   }
 }
 
-void SharedFunctionInfo::SetExpectedNofPropertiesFromEstimate(
+uint16_t SharedFunctionInfo::get_property_estimate_from_literal(
     FunctionLiteral* literal) {
   int estimate = literal->expected_property_count();
 
+  // If this is a class constructor, we may have already parsed fields.
+  if (is_class_constructor()) {
+    estimate += expected_nof_properties();
+  }
+  return estimate;
+}
+
+void SharedFunctionInfo::UpdateExpectedNofPropertiesFromEstimate(
+    FunctionLiteral* literal) {
+  set_expected_nof_properties(get_property_estimate_from_literal(literal));
+}
+
+void SharedFunctionInfo::UpdateAndFinalizeExpectedNofPropertiesFromEstimate(
+    FunctionLiteral* literal) {
+  DCHECK(literal->ShouldEagerCompile());
+  if (are_properties_final()) {
+    return;
+  }
+  int estimate = get_property_estimate_from_literal(literal);
+
   // If no properties are added in the constructor, they are more likely
   // to be added later.
   if (estimate == 0) estimate = 2;
@@ -5550,6 +5571,7 @@ void SharedFunctionInfo::SetExpectedNofPropertiesFromEstimate(
   estimate = std::min(estimate, kMaxUInt8);
 
   set_expected_nof_properties(estimate);
+  set_are_properties_final(true);
 }
 
 void SharedFunctionInfo::SetFunctionTokenPosition(int function_token_position,

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

@@ -223,8 +223,9 @@ BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags, is_named_expression,
                     SharedFunctionInfo::IsNamedExpressionBit)
 BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags, is_toplevel,
                     SharedFunctionInfo::IsTopLevelBit)
-BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags, is_oneshot_iife,
-                    SharedFunctionInfo::IsOneshotIIFEBit)
+BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags,
+                    is_oneshot_iife_or_properties_are_final,
+                    SharedFunctionInfo::IsOneshotIIFEOrPropertiesAreFinalBit)
 BIT_FIELD_ACCESSORS(SharedFunctionInfo, flags,
                     is_safe_to_skip_arguments_adaptor,
                     SharedFunctionInfo::IsSafeToSkipArgumentsAdaptorBit)
@@ -735,6 +736,33 @@ bool SharedFunctionInfo::CanDiscardCompiled() const {
   return can_decompile;
 }
 
+bool SharedFunctionInfo::is_class_constructor() const {
+  return IsClassConstructorBit::decode(flags());
+}
+
+bool SharedFunctionInfo::is_oneshot_iife() const {
+  bool bit = is_oneshot_iife_or_properties_are_final();
+  return bit && !is_class_constructor();
+}
+
+void SharedFunctionInfo::set_is_oneshot_iife(bool value) {
+  DCHECK(!value || !is_class_constructor());
+  if (!is_class_constructor()) {
+    set_is_oneshot_iife_or_properties_are_final(value);
+  }
+}
+
+void SharedFunctionInfo::set_are_properties_final(bool value) {
+  if (is_class_constructor()) {
+    set_is_oneshot_iife_or_properties_are_final(value);
+  }
+}
+
+bool SharedFunctionInfo::are_properties_final() const {
+  bool bit = is_oneshot_iife_or_properties_are_final();
+  return bit && is_class_constructor();
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/shared-function-info.h

@@ -330,6 +330,7 @@ class SharedFunctionInfo : public HeapObject {
   DECL_ACCESSORS(function_data, Object)
 
   inline bool IsApiFunction() const;
+  inline bool is_class_constructor() const;
   inline FunctionTemplateInfo get_api_func_data();
   inline void set_api_func_data(FunctionTemplateInfo data);
   inline bool HasBytecodeArray() const;
@@ -467,6 +468,9 @@ class SharedFunctionInfo : public HeapObject {
   // is only executed once.
   DECL_BOOLEAN_ACCESSORS(is_oneshot_iife)
 
+  // Whether or not the number of expected properties may change.
+  DECL_BOOLEAN_ACCESSORS(are_properties_final)
+
   // Indicates that the function represented by the shared function info
   // cannot observe the actual parameters passed at a call site, which
   // means the function doesn't use the arguments object, doesn't use
@@ -564,8 +568,10 @@ class SharedFunctionInfo : public HeapObject {
   static void InitFromFunctionLiteral(Handle<SharedFunctionInfo> shared_info,
                                       FunctionLiteral* lit, bool is_toplevel);
 
-  // Sets the expected number of properties based on estimate from parser.
-  void SetExpectedNofPropertiesFromEstimate(FunctionLiteral* literal);
+  // Updates the expected number of properties based on estimate from parser.
+  void UpdateExpectedNofPropertiesFromEstimate(FunctionLiteral* literal);
+  void UpdateAndFinalizeExpectedNofPropertiesFromEstimate(
+      FunctionLiteral* literal);
 
   // Sets the FunctionTokenOffset field based on the given token position and
   // start position.
@@ -680,7 +686,7 @@ class SharedFunctionInfo : public HeapObject {
   V(HasReportedBinaryCoverageBit, bool, 1, _)                \
   V(IsNamedExpressionBit, bool, 1, _)                        \
   V(IsTopLevelBit, bool, 1, _)                               \
-  V(IsOneshotIIFEBit, bool, 1, _)                            \
+  V(IsOneshotIIFEOrPropertiesAreFinalBit, bool, 1, _)        \
   V(IsSafeToSkipArgumentsAdaptorBit, bool, 1, _)
   DEFINE_BIT_FIELDS(FLAGS_BIT_FIELDS)
 #undef FLAGS_BIT_FIELDS
@@ -713,10 +719,20 @@ class SharedFunctionInfo : public HeapObject {
   // function.
   DECL_ACCESSORS(outer_scope_info, HeapObject)
 
+  // [is_oneshot_iife_or_properties_are_final]: This bit is used to track
+  // two mutually exclusive cases. Either this SharedFunctionInfo is
+  // a oneshot_iife or we have finished parsing its properties. These cases
+  // are mutually exclusive because the properties final bit is only used by
+  // class constructors to handle lazily parsed properties and class
+  // constructors can never be oneshot iifes.
+  DECL_BOOLEAN_ACCESSORS(is_oneshot_iife_or_properties_are_final)
+
   inline void set_kind(FunctionKind kind);
 
   inline void set_needs_home_object(bool value);
 
+  inline uint16_t get_property_estimate_from_literal(FunctionLiteral* literal);
+
   friend class Factory;
   friend class V8HeapExplorer;
   FRIEND_TEST(PreParserTest, LazyFunctionLength);

src/parsing/parser-base.h

@@ -4075,7 +4075,7 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
     return impl()->FailureExpression();
   }
 
-  int expected_property_count = -1;
+  int expected_property_count = 0;
   int suspend_count = 0;
   int function_literal_id = GetNextFunctionLiteralId();
 

src/parsing/parser.cc

@@ -35,7 +35,7 @@ namespace internal {
 FunctionLiteral* Parser::DefaultConstructor(const AstRawString* name,
                                             bool call_super, int pos,
                                             int end_pos) {
-  int expected_property_count = -1;
+  int expected_property_count = 0;
   const int parameter_count = 0;
 
   FunctionKind kind = call_super ? FunctionKind::kDefaultDerivedConstructor
@@ -2365,7 +2365,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
                          should_preparse_inner || should_post_parallel_task;
 
   ScopedPtrList<Statement> body(pointer_buffer());
-  int expected_property_count = -1;
+  int expected_property_count = 0;
   int suspend_count = -1;
   int num_parameters = -1;
   int function_length = -1;
@@ -2918,6 +2918,8 @@ Expression* Parser::RewriteClassLiteral(ClassScope* block_scope,
         "<instance_members_initializer>", class_info->instance_members_scope,
         class_info->instance_fields);
     class_info->constructor->set_requires_instance_members_initializer(true);
+    class_info->constructor->add_expected_properties(
+        class_info->instance_fields->length());
   }
 
   ClassLiteral* class_literal = factory()->NewClassLiteral(

test/cctest/test-inobject-slack-tracking.cc

@@ -1358,6 +1358,116 @@ TEST(Regress8853_FunctionConstructor) {
   CHECK_EQ(6 + 8, obj->map()->GetInObjectProperties());
 }
 
+TEST(InstanceFieldsArePropertiesDefaultConstructorLazy) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  Handle<JSObject> obj = CompileRunI<JSObject>(
+      "new (class {\n"
+      "  x00 = null;\n"
+      "  x01 = null;\n"
+      "  x02 = null;\n"
+      "  x03 = null;\n"
+      "  x04 = null;\n"
+      "  x05 = null;\n"
+      "  x06 = null;\n"
+      "  x07 = null;\n"
+      "  x08 = null;\n"
+      "  x09 = null;\n"
+      "  x10 = null;\n"
+      "});\n");
+  CHECK_EQ(11 + 8, obj->map()->GetInObjectProperties());
+}
+
+TEST(InstanceFieldsArePropertiesFieldsAndConstructorLazy) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  Handle<JSObject> obj = CompileRunI<JSObject>(
+      "new (class {\n"
+      "  x00 = null;\n"
+      "  x01 = null;\n"
+      "  x02 = null;\n"
+      "  x03 = null;\n"
+      "  x04 = null;\n"
+      "  x05 = null;\n"
+      "  x06 = null;\n"
+      "  x07 = null;\n"
+      "  x08 = null;\n"
+      "  x09 = null;\n"
+      "  x10 = null;\n"
+      "  constructor() {\n"
+      "    this.x11 = null;\n"
+      "    this.x12 = null;\n"
+      "    this.x12 = null;\n"
+      "    this.x14 = null;\n"
+      "    this.x15 = null;\n"
+      "    this.x16 = null;\n"
+      "    this.x17 = null;\n"
+      "    this.x18 = null;\n"
+      "    this.x19 = null;\n"
+      "    this.x20 = null;\n"
+      "  }\n"
+      "});\n");
+  CHECK_EQ(21 + 8, obj->map()->GetInObjectProperties());
+}
+
+TEST(InstanceFieldsArePropertiesDefaultConstructorEager) {
+  i::FLAG_lazy = false;
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  Handle<JSObject> obj = CompileRunI<JSObject>(
+      "new (class {\n"
+      "  x00 = null;\n"
+      "  x01 = null;\n"
+      "  x02 = null;\n"
+      "  x03 = null;\n"
+      "  x04 = null;\n"
+      "  x05 = null;\n"
+      "  x06 = null;\n"
+      "  x07 = null;\n"
+      "  x08 = null;\n"
+      "  x09 = null;\n"
+      "  x10 = null;\n"
+      "});\n");
+  CHECK_EQ(11 + 8, obj->map()->GetInObjectProperties());
+}
+
+TEST(InstanceFieldsArePropertiesFieldsAndConstructorEager) {
+  i::FLAG_lazy = false;
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  Handle<JSObject> obj = CompileRunI<JSObject>(
+      "new (class {\n"
+      "  x00 = null;\n"
+      "  x01 = null;\n"
+      "  x02 = null;\n"
+      "  x03 = null;\n"
+      "  x04 = null;\n"
+      "  x05 = null;\n"
+      "  x06 = null;\n"
+      "  x07 = null;\n"
+      "  x08 = null;\n"
+      "  x09 = null;\n"
+      "  x10 = null;\n"
+      "  constructor() {\n"
+      "    this.x11 = null;\n"
+      "    this.x12 = null;\n"
+      "    this.x12 = null;\n"
+      "    this.x14 = null;\n"
+      "    this.x15 = null;\n"
+      "    this.x16 = null;\n"
+      "    this.x17 = null;\n"
+      "    this.x18 = null;\n"
+      "    this.x19 = null;\n"
+      "    this.x20 = null;\n"
+      "  }\n"
+      "});\n");
+  CHECK_EQ(21 + 8, obj->map()->GetInObjectProperties());
+}
+
 }  // namespace test_inobject_slack_tracking
 }  // namespace internal
 }  // namespace v8