[runtime] Don't track "class" field types for arrays with properties.

... in order to avoid the need to update field types through elements
kind transitions.

Bug: chromium:738763, chromium:745844
Change-Id: I9f0e7f321e7f44ab5b36c06dd4c5633611370807
Reviewed-on: https://chromium-review.googlesource.com/581647Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46830}

src/objects.cc

@@ -3599,6 +3599,16 @@ MaybeHandle<Map> Map::CopyWithField(Handle<Map> map, Handle<Name> name,
   if (map->instance_type() == JS_CONTEXT_EXTENSION_OBJECT_TYPE) {
     representation = Representation::Tagged();
     type = FieldType::Any(isolate);
+  } else if (IsTransitionableFastElementsKind(map->elements_kind()) &&
+             IsInplaceGeneralizableField(constness, representation, *type)) {
+    // We don't support propagation of field generalization through elements
+    // kind transitions because they are inserted into the transition tree
+    // before field transitions. In order to avoid complexity of handling
+    // such a case we ensure that all maps with transitionable elements kinds
+    // do not have fields that can be generalized in-place (without creation
+    // of a new map).
+    DCHECK(representation.IsHeapObject());
+    type = FieldType::Any(isolate);
   }
 
   Handle<Object> wrapped_type(WrapFieldType(type));
@@ -4260,7 +4270,6 @@ Handle<Map> Map::CopyGeneralizeAllFields(Handle<Map> map,
   return new_map;
 }
 
-
 void Map::DeprecateTransitionTree() {
   if (is_deprecated()) return;
   Object* transitions = raw_transitions();
@@ -4404,7 +4413,6 @@ Handle<FieldType> Map::GeneralizeFieldType(Representation rep1,
   return FieldType::Any(isolate);
 }
 
-
 // static
 void Map::GeneralizeField(Handle<Map> map, int modify_index,
                           PropertyConstness new_constness,
@@ -4438,8 +4446,8 @@ void Map::GeneralizeField(Handle<Map> map, int modify_index,
 
   // Determine the field owner.
   Handle<Map> field_owner(map->FindFieldOwner(modify_index), isolate);
-  Handle<DescriptorArray> descriptors(
-      field_owner->instance_descriptors(), isolate);
+  Handle<DescriptorArray> descriptors(field_owner->instance_descriptors(),
+                                      isolate);
   DCHECK_EQ(*old_field_type, descriptors->GetFieldType(modify_index));
 
   new_field_type =
@@ -4467,6 +4475,20 @@ void Map::GeneralizeField(Handle<Map> map, int modify_index,
   }
 }
 
+bool Map::IsInplaceGeneralizableField(PropertyConstness constness,
+                                      Representation representation,
+                                      FieldType* field_type) {
+  if (FLAG_track_constant_fields && FLAG_modify_map_inplace &&
+      (constness == kConst)) {
+    // kConst -> kMutable field generalization may happen in-place.
+    return true;
+  }
+  if (representation.IsHeapObject() && !field_type->IsAny()) {
+    return true;
+  }
+  return false;
+}
+
 // TODO(ishell): remove.
 // static
 Handle<Map> Map::ReconfigureProperty(Handle<Map> map, int modify_index,
@@ -5188,7 +5210,7 @@ Map* Map::FindElementsKindTransitionedMap(MapHandles const& candidates) {
   if (IsTransitionableFastElementsKind(kind)) {
     // Check the state of the root map.
     Map* root_map = FindRootMap();
-    if (!EquivalentToForTransition(root_map)) return nullptr;
+    if (!EquivalentToForElementsKindTransition(root_map)) return nullptr;
     root_map = root_map->LookupElementsTransitionMap(kind);
     DCHECK_NOT_NULL(root_map);
     // Starting from the next existing elements kind transition try to
@@ -12134,7 +12156,7 @@ bool Map::EquivalentToForTransition(const Map* other) const {
   if (!CheckEquivalent(this, other)) return false;
   if (instance_type() == JS_FUNCTION_TYPE) {
     // JSFunctions require more checks to ensure that sloppy function is
-    // not equvalent to strict function.
+    // not equivalent to strict function.
     int nof = Min(NumberOfOwnDescriptors(), other->NumberOfOwnDescriptors());
     return instance_descriptors()->IsEqualUpTo(other->instance_descriptors(),
                                                nof);
@@ -12142,6 +12164,26 @@ bool Map::EquivalentToForTransition(const Map* other) const {
   return true;
 }
 
+bool Map::EquivalentToForElementsKindTransition(const Map* other) const {
+  if (!EquivalentToForTransition(other)) return false;
+#ifdef DEBUG
+  // Ensure that we don't try to generate elements kind transitions from maps
+  // with fields that may be generalized in-place. This must already be handled
+  // during addition of a new field.
+  DescriptorArray* descriptors = instance_descriptors();
+  int nof = NumberOfOwnDescriptors();
+  for (int i = 0; i < nof; i++) {
+    PropertyDetails details = descriptors->GetDetails(i);
+    if (details.location() == kField) {
+      DCHECK(!IsInplaceGeneralizableField(details.constness(),
+                                          details.representation(),
+                                          descriptors->GetFieldType(i)));
+    }
+  }
+#endif
+  return true;
+}
+
 bool Map::EquivalentToForNormalization(const Map* other,
                                        PropertyNormalizationMode mode) const {
   int properties =

src/objects.h

@@ -252,7 +252,6 @@ enum SimpleTransitionFlag {
   SPECIAL_TRANSITION
 };
 
-
 // Indicates whether we are only interested in the descriptors of a particular
 // map, or in all descriptors in the descriptor array.
 enum DescriptorFlag {

src/objects/map.h

@@ -344,6 +344,11 @@ class Map : public HeapObject {
                               PropertyConstness new_constness,
                               Representation new_representation,
                               Handle<FieldType> new_field_type);
+  // Returns true if |descriptor|'th property is a field that may be generalized
+  // by just updating current map.
+  static bool IsInplaceGeneralizableField(PropertyConstness constness,
+                                          Representation representation,
+                                          FieldType* field_type);
 
   static Handle<Map> ReconfigureProperty(Handle<Map> map, int modify_index,
                                          PropertyKind new_kind,
@@ -767,6 +772,7 @@ class Map : public HeapObject {
                                 Handle<Name> name, SimpleTransitionFlag flag);
 
   bool EquivalentToForTransition(const Map* other) const;
+  bool EquivalentToForElementsKindTransition(const Map* other) const;
   static Handle<Map> RawCopy(Handle<Map> map, int instance_size);
   static Handle<Map> ShareDescriptor(Handle<Map> map,
                                      Handle<DescriptorArray> descriptors,

test/cctest/test-field-type-tracking.cc

@@ -320,8 +320,21 @@ class Expectations {
                            Handle<FieldType> heap_type) {
     CHECK_EQ(number_of_properties_, map->NumberOfOwnDescriptors());
     int property_index = number_of_properties_++;
-    SetDataField(property_index, attributes, constness, representation,
-                 heap_type);
+    PropertyConstness expected_constness = constness;
+    Representation expected_representation = representation;
+    Handle<FieldType> expected_heap_type = heap_type;
+    if (IsTransitionableFastElementsKind(map->elements_kind())) {
+      // Maps with transitionable elements kinds must have non in-place
+      // generalizable fields.
+      if (FLAG_track_constant_fields && FLAG_modify_map_inplace) {
+        expected_constness = kMutable;
+      }
+      if (representation.IsHeapObject() && heap_type->IsClass()) {
+        expected_heap_type = FieldType::Any(isolate_);
+      }
+    }
+    SetDataField(property_index, attributes, expected_constness,
+                 expected_representation, expected_heap_type);
 
     Handle<String> name = MakeName("prop", property_index);
     return Map::CopyWithField(map, name, heap_type, attributes, constness,
@@ -1786,8 +1799,7 @@ static void TestReconfigureElementsKind_GeneralizeField(
 // where "p2A" and "p2B" differ only in the representation/field type.
 //
 static void TestReconfigureElementsKind_GeneralizeFieldTrivial(
-    const CRFTData& from, const CRFTData& to, const CRFTData& expected,
-    bool expected_field_type_dependency = true) {
+    const CRFTData& from, const CRFTData& to, const CRFTData& expected) {
   Isolate* isolate = CcTest::i_isolate();
 
   Expectations expectations(isolate, PACKED_SMI_ELEMENTS);
@@ -1849,7 +1861,7 @@ static void TestReconfigureElementsKind_GeneralizeFieldTrivial(
                             expected.representation, expected.type);
   CHECK(!map->is_deprecated());
   CHECK_EQ(*map, *new_map);
-  CHECK_EQ(expected_field_type_dependency, dependencies.HasAborted());
+  CHECK(!dependencies.HasAborted());
   dependencies.Rollback();  // Properly cleanup compilation info.
 
   CHECK(!new_map->is_deprecated());
@@ -2010,7 +2022,7 @@ TEST(ReconfigureElementsKind_GeneralizeHeapObjFieldToHeapObj) {
     TestReconfigureElementsKind_GeneralizeFieldTrivial(
         {kConst, Representation::HeapObject(), any_type},
         {kConst, Representation::HeapObject(), new_type},
-        {kConst, Representation::HeapObject(), any_type}, false);
+        {kConst, Representation::HeapObject(), any_type});
 
     if (FLAG_modify_map_inplace) {
       // kConst to kMutable migration does not create a new map, therefore
@@ -2031,12 +2043,12 @@ TEST(ReconfigureElementsKind_GeneralizeHeapObjFieldToHeapObj) {
     TestReconfigureElementsKind_GeneralizeFieldTrivial(
         {kMutable, Representation::HeapObject(), any_type},
         {kConst, Representation::HeapObject(), new_type},
-        {kMutable, Representation::HeapObject(), any_type}, false);
+        {kMutable, Representation::HeapObject(), any_type});
   }
   TestReconfigureElementsKind_GeneralizeFieldTrivial(
       {kMutable, Representation::HeapObject(), any_type},
       {kMutable, Representation::HeapObject(), new_type},
-      {kMutable, Representation::HeapObject(), any_type}, false);
+      {kMutable, Representation::HeapObject(), any_type});
 }
 
 TEST(ReconfigureElementsKind_GeneralizeHeapObjectFieldToTagged) {

test/mjsunit/regress/regress-crbug-738763.js

+// Copyright 2017 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: --verify-heap --allow-natives-syntax --expose-gc
+
+let constant = { a: 1 };
+
+function update_array(array) {
+  array.x = constant;
+  %HeapObjectVerify(array);
+  array[0] = undefined;
+  %HeapObjectVerify(array);
+  return array;
+}
+
+let ar1 = [1];
+let ar2 = [2];
+let ar3 = [3];
+gc();
+gc();
+
+update_array(ar1);
+constant = update_array(ar2);
+update_array(ar3);