[objects] Adjust overly aggressive over-allocation.

When setting up the initial map for a (class or function) constructor,
we always over-allocate a bunch of in-object properties, in case not
all property assignments happen as `this.prop = val` assignments in
the constructor. However this over-allocation was a bit too aggressive
and added a slack of 8 to each class constructor (plus a minimum of
two, when there was no `this.prop = val` assignment). So in total this
would yield an object with initially 40 in-object property slots in
case of a simple class hierarchy like this:

```js
class A {};
class B extends A {};
class C extends B {};
class D extends C {};
new D;
```

While the slack tracking takes care of eventually shrinking the objects
to appropriate sizes, this aggressive over-allocation is still going to
hurt performance quite a bit in the beginning, and will also lead to
more traffic on the minor GC for now good reason.

Instead of the above, we now allocate a minimum of 2 in-object
properties per class (in a hierarchy) and then add a slack of 8 in the
end. Meaning for the example above we end up with 16 initial in-object
property slots, which seems sensible.

Bug: v8:8853
Change-Id: I4a11e35a8612ceef1d776ca2f0543a26c8c2a2bf
Reviewed-on: https://chromium-review.googlesource.com/c/1477276
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59670}

src/objects.cc

@@ -5423,10 +5423,6 @@ void SharedFunctionInfo::SetExpectedNofPropertiesFromEstimate(
   // to be added later.
   if (estimate == 0) estimate = 2;
 
-  // Inobject slack tracking will reclaim redundant inobject space later,
-  // so we can afford to adjust the estimate generously.
-  estimate += 8;
-
   // Limit actual estimate to fit in a 8 bit field, we will never allocate
   // more than this in any case.
   STATIC_ASSERT(JSObject::kMaxInObjectProperties <= kMaxUInt8);

src/objects/js-objects.cc

@@ -5446,6 +5446,15 @@ int JSFunction::CalculateExpectedNofProperties(Isolate* isolate,
       break;
     }
   }
+  // Inobject slack tracking will reclaim redundant inobject space
+  // later, so we can afford to adjust the estimate generously,
+  // meaning we over-allocate by at least 8 slots in the beginning.
+  if (expected_nof_properties > 0) {
+    expected_nof_properties += 8;
+    if (expected_nof_properties > JSObject::kMaxInObjectProperties) {
+      expected_nof_properties = JSObject::kMaxInObjectProperties;
+    }
+  }
   return expected_nof_properties;
 }
 

test/cctest/test-elements-kind.cc

@@ -113,7 +113,8 @@ TEST(JSObjectInObjectAddingProperties) {
       factory->NewFunctionForTest(factory->empty_string());
   int nof_inobject_properties = 10;
   // force in object properties by changing the expected_nof_properties
-  function->shared()->set_expected_nof_properties(nof_inobject_properties);
+  // (we always reserve 8 inobject properties slack on top).
+  function->shared()->set_expected_nof_properties(nof_inobject_properties - 8);
   Handle<Object> value(Smi::FromInt(42), isolate);
 
   Handle<JSObject> object = factory->NewJSObject(function);

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

@@ -1287,6 +1287,77 @@ TEST(SubclassTranspiledClassHierarchy) {
   CHECK_EQ(JS_OBJECT_TYPE, obj->map()->instance_type());
 }
 
+TEST(Regress8853_ClassConstructor) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  // For classes without any this.prop assignments in their
+  // constructors we start out with 10 inobject properties.
+  Handle<JSObject> obj = CompileRunI<JSObject>("new (class {});\n");
+  CHECK(obj->map()->IsInobjectSlackTrackingInProgress());
+  CHECK(IsObjectShrinkable(*obj));
+  CHECK_EQ(10, obj->map()->GetInObjectProperties());
+
+  // For classes with N explicit this.prop assignments in their
+  // constructors we start out with N+8 inobject properties.
+  obj = CompileRunI<JSObject>(
+      "new (class {\n"
+      "  constructor() {\n"
+      "    this.x = 1;\n"
+      "    this.y = 2;\n"
+      "    this.z = 3;\n"
+      "  }\n"
+      "});\n");
+  CHECK(obj->map()->IsInobjectSlackTrackingInProgress());
+  CHECK(IsObjectShrinkable(*obj));
+  CHECK_EQ(3 + 8, obj->map()->GetInObjectProperties());
+}
+
+TEST(Regress8853_ClassHierarchy) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  // For class hierarchies without any this.prop assignments in their
+  // constructors we reserve 2 inobject properties per constructor plus
+  // 8 inobject properties slack on top.
+  std::string base = "(class {})";
+  for (int i = 1; i < 10; ++i) {
+    std::string script = "new " + base + ";\n";
+    Handle<JSObject> obj = CompileRunI<JSObject>(script.c_str());
+    CHECK(obj->map()->IsInobjectSlackTrackingInProgress());
+    CHECK(IsObjectShrinkable(*obj));
+    CHECK_EQ(8 + 2 * i, obj->map()->GetInObjectProperties());
+    base = "(class extends " + base + " {})";
+  }
+}
+
+TEST(Regress8853_FunctionConstructor) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  // For constructor functions without any this.prop assignments in
+  // them we start out with 10 inobject properties.
+  Handle<JSObject> obj = CompileRunI<JSObject>("new (function() {});\n");
+  CHECK(obj->map()->IsInobjectSlackTrackingInProgress());
+  CHECK(IsObjectShrinkable(*obj));
+  CHECK_EQ(10, obj->map()->GetInObjectProperties());
+
+  // For constructor functions with N explicit this.prop assignments
+  // in them we start out with N+8 inobject properties.
+  obj = CompileRunI<JSObject>(
+      "new (function() {\n"
+      "  this.a = 1;\n"
+      "  this.b = 2;\n"
+      "  this.c = 3;\n"
+      "  this.d = 3;\n"
+      "  this.c = 3;\n"
+      "  this.f = 3;\n"
+      "});\n");
+  CHECK(obj->map()->IsInobjectSlackTrackingInProgress());
+  CHECK(IsObjectShrinkable(*obj));
+  CHECK_EQ(6 + 8, obj->map()->GetInObjectProperties());
+}
+
 }  // namespace test_inobject_slack_tracking
 }  // namespace internal
 }  // namespace v8