Layout descriptor sharing issue fixed.

BUG=chromium:437713, v8:3832
LOG=Y

Review URL: https://codereview.chromium.org/885003002

Cr-Commit-Position: refs/heads/master@{#26354}

src/layout-descriptor.cc

@@ -72,13 +72,16 @@ Handle<LayoutDescriptor> LayoutDescriptor::New(
 }
 
 
-Handle<LayoutDescriptor> LayoutDescriptor::Append(Handle<Map> map,
-                                                  PropertyDetails details) {
+Handle<LayoutDescriptor> LayoutDescriptor::ShareAppend(
+    Handle<Map> map, PropertyDetails details) {
+  DCHECK(map->owns_descriptors());
   Isolate* isolate = map->GetIsolate();
   Handle<LayoutDescriptor> layout_descriptor(map->GetLayoutDescriptor(),
                                              isolate);
 
   if (!InobjectUnboxedField(map->inobject_properties(), details)) {
+    DCHECK(details.location() != kField ||
+           layout_descriptor->IsTagged(details.field_index()));
     return layout_descriptor;
   }
   int field_index = details.field_index();
@@ -104,6 +107,8 @@ Handle<LayoutDescriptor> LayoutDescriptor::AppendIfFastOrUseFull(
     return full_layout_descriptor;
   }
   if (!InobjectUnboxedField(map->inobject_properties(), details)) {
+    DCHECK(details.location() != kField ||
+           layout_descriptor->IsTagged(details.field_index()));
     return handle(layout_descriptor, map->GetIsolate());
   }
   int field_index = details.field_index();
@@ -127,7 +132,6 @@ Handle<LayoutDescriptor> LayoutDescriptor::EnsureCapacity(
     int new_capacity) {
   int old_capacity = layout_descriptor->capacity();
   if (new_capacity <= old_capacity) {
-    // Nothing to do with layout in Smi-form.
     return layout_descriptor;
   }
   Handle<LayoutDescriptor> new_layout_descriptor =

src/layout-descriptor.h

@@ -53,9 +53,9 @@ class LayoutDescriptor : public FixedTypedArray<Uint32ArrayTraits> {
                                       Handle<DescriptorArray> descriptors,
                                       int num_descriptors);
 
-  // Creates new layout descriptor by appending property with |details| to
-  // |map|'s layout descriptor.
-  static Handle<LayoutDescriptor> Append(Handle<Map> map,
+  // Modifies |map|'s layout descriptor or creates a new one if necessary by
+  // appending property with |details| to it.
+  static Handle<LayoutDescriptor> ShareAppend(Handle<Map> map,
                                               PropertyDetails details);
 
   // Creates new layout descriptor by appending property with |details| to

src/objects.cc

@@ -6633,7 +6633,7 @@ Handle<Map> Map::ShareDescriptor(Handle<Map> map,
 
   Handle<LayoutDescriptor> layout_descriptor =
       FLAG_unbox_double_fields
-          ? LayoutDescriptor::Append(map, descriptor->GetDetails())
+          ? LayoutDescriptor::ShareAppend(map, descriptor->GetDetails())
           : handle(LayoutDescriptor::FastPointerLayout(), map->GetIsolate());
 
   {
@@ -7150,13 +7150,14 @@ Handle<Map> Map::CopyAddDescriptor(Handle<Map> map,
     return ShareDescriptor(map, descriptors, descriptor);
   }
 
-  Handle<DescriptorArray> new_descriptors = DescriptorArray::CopyUpTo(
-      descriptors, map->NumberOfOwnDescriptors(), 1);
+  int nof = map->NumberOfOwnDescriptors();
+  Handle<DescriptorArray> new_descriptors =
+      DescriptorArray::CopyUpTo(descriptors, nof, 1);
   new_descriptors->Append(descriptor);
 
   Handle<LayoutDescriptor> new_layout_descriptor =
       FLAG_unbox_double_fields
-          ? LayoutDescriptor::Append(map, descriptor->GetDetails())
+          ? LayoutDescriptor::New(map, new_descriptors, nof + 1)
           : handle(LayoutDescriptor::FastPointerLayout(), map->GetIsolate());
 
   return CopyReplaceDescriptors(map, new_descriptors, new_layout_descriptor,

test/cctest/test-unboxed-doubles.cc

@@ -21,6 +21,24 @@ using namespace v8::internal;
 #if (V8_DOUBLE_FIELDS_UNBOXING)
 
 
+//
+// Helper functions.
+//
+
+static Handle<String> MakeString(const char* str) {
+  Isolate* isolate = CcTest::i_isolate();
+  Factory* factory = isolate->factory();
+  return factory->InternalizeUtf8String(str);
+}
+
+
+static Handle<String> MakeName(const char* str, int suffix) {
+  EmbeddedVector<char, 128> buffer;
+  SNPrintF(buffer, "%s%d", str, suffix);
+  return MakeString(buffer.start());
+}
+
+
 static double GetDoubleFieldValue(JSObject* obj, FieldIndex field_index) {
   if (obj->IsUnboxedDoubleField(field_index)) {
     return obj->RawFastDoublePropertyAt(field_index);
@@ -601,14 +619,14 @@ static Handle<LayoutDescriptor> TestLayoutDescriptorAppend(
     TestPropertyKind kind = props[i];
     if (kind == PROP_CONSTANT) {
       DataConstantDescriptor d(name, func, NONE);
-      layout_descriptor = LayoutDescriptor::Append(map, d.GetDetails());
+      layout_descriptor = LayoutDescriptor::ShareAppend(map, d.GetDetails());
       descriptors->Append(&d);
 
     } else {
       DataDescriptor f(name, next_field_offset, NONE, representations[kind]);
       int field_width_in_words = f.GetDetails().field_width_in_words();
       next_field_offset += field_width_in_words;
-      layout_descriptor = LayoutDescriptor::Append(map, f.GetDetails());
+      layout_descriptor = LayoutDescriptor::ShareAppend(map, f.GetDetails());
       descriptors->Append(&f);
 
       int field_index = f.GetDetails().field_index();
@@ -1096,6 +1114,52 @@ TEST(LayoutDescriptorHelperAllDoubles) {
 }
 
 
+TEST(LayoutDescriptorSharing) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+  Isolate* isolate = CcTest::i_isolate();
+  Handle<HeapType> any_type = HeapType::Any(isolate);
+
+  Handle<Map> split_map;
+  {
+    Handle<Map> map = Map::Create(isolate, 64);
+    for (int i = 0; i < 32; i++) {
+      Handle<String> name = MakeName("prop", i);
+      map = Map::CopyWithField(map, name, any_type, NONE, Representation::Smi(),
+                               INSERT_TRANSITION).ToHandleChecked();
+    }
+    split_map = Map::CopyWithField(map, MakeString("dbl"), any_type, NONE,
+                                   Representation::Double(),
+                                   INSERT_TRANSITION).ToHandleChecked();
+  }
+  Handle<LayoutDescriptor> split_layout_descriptor(
+      split_map->layout_descriptor(), isolate);
+  DCHECK(split_layout_descriptor->IsConsistentWithMap(*split_map));
+  CHECK(split_layout_descriptor->IsSlowLayout());
+  CHECK(split_map->owns_descriptors());
+
+  Handle<Map> map1 = Map::CopyWithField(split_map, MakeString("foo"), any_type,
+                                        NONE, Representation::Double(),
+                                        INSERT_TRANSITION).ToHandleChecked();
+  CHECK(!split_map->owns_descriptors());
+  CHECK_EQ(*split_layout_descriptor, split_map->layout_descriptor());
+
+  // Layout descriptors should be shared with |split_map|.
+  CHECK(map1->owns_descriptors());
+  CHECK_EQ(*split_layout_descriptor, map1->layout_descriptor());
+  DCHECK(map1->layout_descriptor()->IsConsistentWithMap(*map1));
+
+  Handle<Map> map2 = Map::CopyWithField(split_map, MakeString("bar"), any_type,
+                                        NONE, Representation::Tagged(),
+                                        INSERT_TRANSITION).ToHandleChecked();
+
+  // Layout descriptors should not be shared with |split_map|.
+  CHECK(map2->owns_descriptors());
+  CHECK_NE(*split_layout_descriptor, map2->layout_descriptor());
+  DCHECK(map2->layout_descriptor()->IsConsistentWithMap(*map2));
+}
+
+
 TEST(StoreBufferScanOnScavenge) {
   CcTest::InitializeVM();
   Isolate* isolate = CcTest::i_isolate();

test/mjsunit/regress/regress-437713.js

+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --enable-slow-asserts
+
+var o1 = {
+  a00:0, a01:0, a02:0, a03:0, a04:0, a05:0, a06:0, a07:0, a08:0, a09:0, a0a:0, a0b:0, a0c:0, a0d:0, a0e:0, a0f:0,
+  a10:0, a11:0, a12:0, a13:0, a14:0, a15:0, a16:0, a17:0, a18:0, a19:0, a1a:0, a1b:0, a1c:0, a1d:0, a1e:0, a1f:0,
+
+  dbl: 0.1,
+
+  some_double: 2.13,
+};
+
+var o2 = {
+  a00:0, a01:0, a02:0, a03:0, a04:0, a05:0, a06:0, a07:0, a08:0, a09:0, a0a:0, a0b:0, a0c:0, a0d:0, a0e:0, a0f:0,
+  a10:0, a11:0, a12:0, a13:0, a14:0, a15:0, a16:0, a17:0, a18:0, a19:0, a1a:0, a1b:0, a1c:0, a1d:0, a1e:0, a1f:0,
+
+  dbl: 0.1,
+
+  boom: [],
+};
+
+o2.boom.push(42);
+assertEquals(42, o2.boom[0]);