From 6564c6dfc46873877d7b51ac0280432e720af72e Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Tue, 7 May 2019 14:19:46 +0200 Subject: [PATCH] [map] Make field representation updates work with elements kind transitions. Generalize the existing work-around in the method `Map::GeneralizeIfCanHaveTransitionableFastElementsKind()` to also go to the most general field representation (in addition to going to the most field type) for objects with transitionable fast elements kinds. That means that we essentially disable field representation tracking for arrays, arguments objects and value wrappers (for which the field type tracking is already disabled). Drive-by-fix: Remove the `constness` parameter to the above mentioned helper method. And fix the printing of the descriptor expectations to properly print the field type. Change-Id: I1bba9415f4bdd2c916f9d105d9120c7071d2c498 Cq-Include-Trybots: luci.chromium.try:linux-rel,win7-rel Doc: http://bit.ly/v8-in-place-field-representation-changes Bug: v8:8749, v8:8865, v8:9114, chromium:959645, chromium:952682 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1598756 Commit-Queue: Benedikt Meurer Reviewed-by: Igor Sheludko Cr-Commit-Position: refs/heads/master@{#61284} --- src/field-type.h | 2 +- src/map-updater.cc | 7 +- src/objects/map-inl.h | 11 +- src/objects/map.cc | 2 +- src/objects/map.h | 11 +- test/cctest/test-field-type-tracking.cc | 133 +++--------------- test/cctest/test-inobject-slack-tracking.cc | 7 +- .../mjsunit/regress/regress-crbug-959645-1.js | 15 ++ .../mjsunit/regress/regress-crbug-959645-2.js | 15 ++ 9 files changed, 67 insertions(+), 136 deletions(-) create mode 100644 test/mjsunit/regress/regress-crbug-959645-1.js create mode 100644 test/mjsunit/regress/regress-crbug-959645-2.js diff --git a/src/field-type.h b/src/field-type.h index 405fa7ab14..9da2bcd2d1 100644 --- a/src/field-type.h +++ b/src/field-type.h @@ -41,7 +41,7 @@ class FieldType : public Object { bool NowIs(FieldType other) const; bool NowIs(Handle other) const; - void PrintTo(std::ostream& os) const; + V8_EXPORT_PRIVATE void PrintTo(std::ostream& os) const; FieldType* operator->() { return this; } const FieldType* operator->() const { return this; } diff --git a/src/map-updater.cc b/src/map-updater.cc index 5a0f3790d1..3f0b5d9aea 100644 --- a/src/map-updater.cc +++ b/src/map-updater.cc @@ -147,8 +147,8 @@ Handle MapUpdater::ReconfigureToDataField(int descriptor, } Map::GeneralizeIfCanHaveTransitionableFastElementsKind( - isolate_, old_map_->instance_type(), &new_constness_, - &new_representation_, &new_field_type_); + isolate_, old_map_->instance_type(), &new_representation_, + &new_field_type_); if (TryReconfigureToDataFieldInplace() == kEnd) return result_map_; if (FindRootMap() == kEnd) return result_map_; @@ -586,8 +586,7 @@ Handle MapUpdater::BuildDescriptorArray() { target_field_type, isolate_); Map::GeneralizeIfCanHaveTransitionableFastElementsKind( - isolate_, instance_type, &next_constness, &next_representation, - &next_field_type); + isolate_, instance_type, &next_representation, &next_field_type); MaybeObjectHandle wrapped_type( Map::WrapFieldType(isolate_, next_field_type)); diff --git a/src/objects/map-inl.h b/src/objects/map-inl.h index 7009f50d7e..b0a5389b28 100644 --- a/src/objects/map-inl.h +++ b/src/objects/map-inl.h @@ -122,19 +122,16 @@ bool Map::CanHaveFastTransitionableElementsKind() const { // static void Map::GeneralizeIfCanHaveTransitionableFastElementsKind( - Isolate* isolate, InstanceType instance_type, PropertyConstness* constness, + Isolate* isolate, InstanceType instance_type, Representation* representation, Handle* field_type) { if (CanHaveFastTransitionableElementsKind(instance_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 - // have the most general field type. - if (representation->IsHeapObject()) { - // The field type is either already Any or should become Any if it was - // something else. - *field_type = FieldType::Any(isolate); - } + // have the most general field representation and type. + *field_type = FieldType::Any(isolate); + *representation = Representation::Tagged(); } } diff --git a/src/objects/map.cc b/src/objects/map.cc index a42a6a7007..bc9299261c 100644 --- a/src/objects/map.cc +++ b/src/objects/map.cc @@ -447,7 +447,7 @@ MaybeHandle Map::CopyWithField(Isolate* isolate, Handle map, type = FieldType::Any(isolate); } else { Map::GeneralizeIfCanHaveTransitionableFastElementsKind( - isolate, map->instance_type(), &constness, &representation, &type); + isolate, map->instance_type(), &representation, &type); } MaybeObjectHandle wrapped_type = WrapFieldType(isolate, type); diff --git a/src/objects/map.h b/src/objects/map.h index 96c09e1664..42bbe36c02 100644 --- a/src/objects/map.h +++ b/src/objects/map.h @@ -518,17 +518,16 @@ class Map : public HeapObject { static inline bool IsMostGeneralFieldType(Representation representation, FieldType field_type); - // Generalizes constness, representation and field_type if objects with given - // instance type can have fast elements that can be transitioned by stubs or - // optimized code to more general elements kind. + // Generalizes representation and field_type if objects with given + // instance type can have fast elements that can be transitioned by + // stubs or optimized code to more general elements kind. // This generalization is necessary in order to ensure that elements kind // transitions performed by stubs / optimized code don't silently transition - // PropertyConstness::kMutable fields back to VariableMode::kConst state or + // fields with representation "Tagged" back to "Smi" or "HeapObject" or // fields with HeapObject representation and "Any" type back to "Class" type. static inline void GeneralizeIfCanHaveTransitionableFastElementsKind( Isolate* isolate, InstanceType instance_type, - PropertyConstness* constness, Representation* representation, - Handle* field_type); + Representation* representation, Handle* field_type); V8_EXPORT_PRIVATE static Handle ReconfigureProperty( Isolate* isolate, Handle map, int modify_index, diff --git a/test/cctest/test-field-type-tracking.cc b/test/cctest/test-field-type-tracking.cc index db4d259ea4..7d97bb04f9 100644 --- a/test/cctest/test-field-type-tracking.cc +++ b/test/cctest/test-field-type-tracking.cc @@ -123,6 +123,7 @@ class Expectations { // Maps with transitionable elements kinds must have the most general // field type. value = FieldType::Any(isolate_); + representation = Representation::Tagged(); } constnesses_[index] = constness; attributes_[index] = attributes; @@ -138,7 +139,7 @@ class Expectations { os << "Descriptor @ "; if (kinds_[i] == kData) { - os << Brief(*values_[i]); + Handle::cast(values_[i])->PrintTo(os); } else { // kAccessor os << "(get: " << Brief(*values_[i]) @@ -1727,104 +1728,6 @@ TEST(ReconfigureDataFieldAttribute_AccConstantToDataFieldAfterTargetMap) { namespace { -// This test ensures that field generalization is correctly propagated from one -// branch of transition tree (|map2) to another (|map|). -// -// + - p0 - p1 - p2A - p3 - p4: |map| -// | -// ek -// | -// {} - p0 - p1 - p2B - p3 - p4: |map2| -// -// where "p2A" and "p2B" differ only in the representation/field type. -// -static void TestReconfigureElementsKind_GeneralizeField( - const CRFTData& from, const CRFTData& to, const CRFTData& expected) { - Isolate* isolate = CcTest::i_isolate(); - - Expectations expectations(isolate, PACKED_SMI_ELEMENTS); - - // Create a map, add required properties to it and initialize expectations. - Handle initial_map = Map::Create(isolate, 0); - initial_map->set_instance_type(JS_ARRAY_TYPE); - initial_map->set_elements_kind(PACKED_SMI_ELEMENTS); - - Handle map = initial_map; - map = expectations.AsElementsKind(map, PACKED_ELEMENTS); - for (int i = 0; i < kPropCount; i++) { - map = expectations.AddDataField(map, NONE, from.constness, - from.representation, from.type); - } - CHECK(!map->is_deprecated()); - CHECK(map->is_stable()); - CHECK(expectations.Check(*map)); - - // Create another branch in transition tree (property at index |kDiffProp| - // has different representatio/field type), initialize expectations. - const int kDiffProp = kPropCount / 2; - Expectations expectations2(isolate, PACKED_SMI_ELEMENTS); - - Handle map2 = initial_map; - for (int i = 0; i < kPropCount; i++) { - if (i == kDiffProp) { - map2 = expectations2.AddDataField(map2, NONE, to.constness, - to.representation, to.type); - } else { - map2 = expectations2.AddDataField(map2, NONE, from.constness, - from.representation, from.type); - } - } - CHECK(!map2->is_deprecated()); - CHECK(map2->is_stable()); - CHECK(expectations2.Check(*map2)); - - // Create dummy optimized code object to test correct dependencies - // on the field owner. - Handle code = CreateDummyOptimizedCode(isolate); - Handle field_owner(map->FindFieldOwner(isolate, kDiffProp), isolate); - DependentCode::InstallDependency(isolate, MaybeObjectHandle::Weak(code), - field_owner, - DependentCode::kFieldOwnerGroup); - CHECK(!code->marked_for_deoptimization()); - - // Reconfigure elements kinds of |map2|, which should generalize - // representations in |map|. - Handle new_map = - Map::ReconfigureElementsKind(isolate, map2, PACKED_ELEMENTS); - - // |map2| should be left unchanged but marked unstable. - CHECK(!map2->is_stable()); - CHECK(!map2->is_deprecated()); - CHECK_NE(*map2, *new_map); - CHECK(expectations2.Check(*map2)); - - // |map| should be deprecated and |new_map| should match new expectations. - expectations.SetDataField(kDiffProp, expected.constness, - expected.representation, expected.type); - - CHECK(map->is_deprecated()); - CHECK(!code->marked_for_deoptimization()); - CHECK_NE(*map, *new_map); - - CHECK(!new_map->is_deprecated()); - CHECK(expectations.Check(*new_map)); - - // Update deprecated |map|, it should become |new_map|. - Handle updated_map = Map::Update(isolate, map); - CHECK_EQ(*new_map, *updated_map); - CheckMigrationTarget(isolate, *map, *updated_map); - - // Ensure Map::FindElementsKindTransitionedMap() is able to find the - // transitioned map. - { - MapHandles map_list; - map_list.push_back(updated_map); - Map transitioned_map = - map2->FindElementsKindTransitionedMap(isolate, map_list); - CHECK_EQ(*updated_map, transitioned_map); - } -} - // This test ensures that trivial field generalization (from HeapObject to // HeapObject) is correctly propagated from one branch of transition tree // (|map2|) to another (|map|). @@ -1934,22 +1837,22 @@ TEST(ReconfigureElementsKind_GeneralizeSmiFieldToDouble) { Handle any_type = FieldType::Any(isolate); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::Smi(), any_type}, {PropertyConstness::kConst, Representation::Double(), any_type}, {PropertyConstness::kConst, Representation::Double(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::Double(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::Smi(), any_type}, {PropertyConstness::kConst, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::Double(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::Double(), any_type}); @@ -1964,22 +1867,22 @@ TEST(ReconfigureElementsKind_GeneralizeSmiFieldToTagged) { Handle value_type = FieldType::Class(Map::Create(isolate, 0), isolate); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::Smi(), any_type}, {PropertyConstness::kConst, Representation::HeapObject(), value_type}, {PropertyConstness::kConst, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::Smi(), any_type}, {PropertyConstness::kConst, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); @@ -1994,22 +1897,22 @@ TEST(ReconfigureElementsKind_GeneralizeDoubleFieldToTagged) { Handle value_type = FieldType::Class(Map::Create(isolate, 0), isolate); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::Double(), any_type}, {PropertyConstness::kConst, Representation::HeapObject(), value_type}, {PropertyConstness::kConst, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kConst, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::Double(), any_type}, {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); @@ -2092,22 +1995,22 @@ TEST(ReconfigureElementsKind_GeneralizeHeapObjectFieldToTagged) { Handle value_type = FieldType::Class(Map::Create(isolate, 0), isolate); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::HeapObject(), value_type}, {PropertyConstness::kConst, Representation::Smi(), any_type}, {PropertyConstness::kConst, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kConst, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kConst, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); - TestReconfigureElementsKind_GeneralizeField( + TestReconfigureElementsKind_GeneralizeFieldTrivial( {PropertyConstness::kMutable, Representation::HeapObject(), value_type}, {PropertyConstness::kMutable, Representation::Smi(), any_type}, {PropertyConstness::kMutable, Representation::Tagged(), any_type}); diff --git a/test/cctest/test-inobject-slack-tracking.cc b/test/cctest/test-inobject-slack-tracking.cc index ac4ffb297e..62962f9647 100644 --- a/test/cctest/test-inobject-slack-tracking.cc +++ b/test/cctest/test-inobject-slack-tracking.cc @@ -85,8 +85,11 @@ static double GetDoubleFieldValue(JSObject obj, FieldIndex field_index) { return obj->RawFastDoublePropertyAt(field_index); } else { Object value = obj->RawFastPropertyAt(field_index); - CHECK(value->IsMutableHeapNumber()); - return MutableHeapNumber::cast(value)->value(); + if (value->IsMutableHeapNumber()) { + return MutableHeapNumber::cast(value)->value(); + } else { + return value->Number(); + } } } diff --git a/test/mjsunit/regress/regress-crbug-959645-1.js b/test/mjsunit/regress/regress-crbug-959645-1.js new file mode 100644 index 0000000000..afe9612db4 --- /dev/null +++ b/test/mjsunit/regress/regress-crbug-959645-1.js @@ -0,0 +1,15 @@ +// Copyright 2019 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 --modify-field-representations-inplace + +function f(array, x) { + array.x = x; + array[0] = 1.1; + return array; +} + +f([1], 1); +f([2], 1); +%HeapObjectVerify(f([3], undefined)); diff --git a/test/mjsunit/regress/regress-crbug-959645-2.js b/test/mjsunit/regress/regress-crbug-959645-2.js new file mode 100644 index 0000000000..634bfa9543 --- /dev/null +++ b/test/mjsunit/regress/regress-crbug-959645-2.js @@ -0,0 +1,15 @@ +// Copyright 2019 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 --modify-field-representations-inplace + +function f(array, x) { + array.x = x; + array[0] = undefined; + return array; +} + +f([1.1], 1); +f([2.2], 1); +%HeapObjectVerify(f([3.3], undefined));