From 4cc4bc591cc8ffe7d458f4e89e1fbe3944d44c9f Mon Sep 17 00:00:00 2001 From: ishell Date: Fri, 12 Jun 2015 05:36:36 -0700 Subject: [PATCH] Map::TryUpdate() must be in sync with Map::Update(). This CL fixes elements kind transitions handling in Map::TryUpdate(). BUG=v8:4121 LOG=Y Review URL: https://codereview.chromium.org/1181163002 Cr-Commit-Position: refs/heads/master@{#28999} --- src/objects.cc | 11 +++++++- test/cctest/test-migrations.cc | 23 ++++++++++----- test/mjsunit/regress/regress-4121.js | 55 ++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 8 deletions(-) create mode 100644 test/mjsunit/regress/regress-4121.js diff --git a/src/objects.cc b/src/objects.cc index ef56f08..cd31a49 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -2420,7 +2420,7 @@ Handle Map::ReconfigureProperty(Handle old_map, int modify_index, ElementsKind from_kind = root_map->elements_kind(); ElementsKind to_kind = old_map->elements_kind(); - if (from_kind != to_kind && + if (from_kind != to_kind && to_kind != DICTIONARY_ELEMENTS && !(IsTransitionableFastElementsKind(from_kind) && IsMoreGeneralElementsKindTransition(from_kind, to_kind))) { return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode, @@ -2884,6 +2884,15 @@ MaybeHandle Map::TryUpdate(Handle old_map) { // Check the state of the root map. Map* root_map = old_map->FindRootMap(); if (!old_map->EquivalentToForTransition(root_map)) return MaybeHandle(); + + ElementsKind from_kind = root_map->elements_kind(); + ElementsKind to_kind = old_map->elements_kind(); + if (from_kind != to_kind) { + // Try to follow existing elements kind transitions. + root_map = root_map->LookupElementsTransitionMap(to_kind); + if (root_map == NULL) return MaybeHandle(); + // From here on, use the map with correct elements kind as root map. + } int root_nof = root_map->NumberOfOwnDescriptors(); int old_nof = old_map->NumberOfOwnDescriptors(); diff --git a/test/cctest/test-migrations.cc b/test/cctest/test-migrations.cc index a5eee5d..e80ec7c 100644 --- a/test/cctest/test-migrations.cc +++ b/test/cctest/test-migrations.cc @@ -20,11 +20,6 @@ using namespace v8::internal; -// TODO(ishell): fix this once ReconfigureProperty supports "non equivalent" -// transitions. -const bool IS_NON_EQUIVALENT_TRANSITION_SUPPORTED = false; - - // TODO(ishell): fix this once TransitionToPrototype stops generalizing // all field representations (similar to crbug/448711 where elements kind // and observed transitions caused generalization of all field representations). @@ -1591,7 +1586,14 @@ static void TestGeneralizeRepresentationWithSpecialTransition( CHECK(!new_map2->is_deprecated()); CHECK(!new_map2->is_dictionary_map()); - if (!IS_NON_EQUIVALENT_TRANSITION_SUPPORTED) { + Handle tmp_map; + if (Map::TryUpdate(map2).ToHandle(&tmp_map)) { + // If Map::TryUpdate() manages to succeed the result must match the result + // of Map::Update(). + CHECK_EQ(*new_map2, *tmp_map); + } + + if (config.is_non_equevalent_transition()) { // In case of non-equivalent transition currently we generalize all // representations. for (int i = 0; i < kPropCount; i++) { @@ -1600,7 +1602,8 @@ static void TestGeneralizeRepresentationWithSpecialTransition( CHECK(new_map2->GetBackPointer()->IsUndefined()); CHECK(expectations2.Check(*new_map2)); } else { - CHECK(expectations.Check(*new_map2)); + CHECK(!new_map2->GetBackPointer()->IsUndefined()); + CHECK(expectations2.Check(*new_map2)); } } @@ -1632,6 +1635,7 @@ TEST(ElementsKindTransitionFromMapOwningDescriptor) { } // TODO(ishell): remove once IS_PROTO_TRANS_ISSUE_FIXED is removed. bool generalizes_representations() const { return false; } + bool is_non_equevalent_transition() const { return false; } }; TestConfig config; TestGeneralizeRepresentationWithSpecialTransition( @@ -1666,6 +1670,7 @@ TEST(ElementsKindTransitionFromMapNotOwningDescriptor) { } // TODO(ishell): remove once IS_PROTO_TRANS_ISSUE_FIXED is removed. bool generalizes_representations() const { return false; } + bool is_non_equevalent_transition() const { return false; } }; TestConfig config; TestGeneralizeRepresentationWithSpecialTransition( @@ -1688,6 +1693,7 @@ TEST(ForObservedTransitionFromMapOwningDescriptor) { } // TODO(ishell): remove once IS_PROTO_TRANS_ISSUE_FIXED is removed. bool generalizes_representations() const { return false; } + bool is_non_equevalent_transition() const { return true; } }; TestConfig config; TestGeneralizeRepresentationWithSpecialTransition( @@ -1721,6 +1727,7 @@ TEST(ForObservedTransitionFromMapNotOwningDescriptor) { } // TODO(ishell): remove once IS_PROTO_TRANS_ISSUE_FIXED is removed. bool generalizes_representations() const { return false; } + bool is_non_equevalent_transition() const { return true; } }; TestConfig config; TestGeneralizeRepresentationWithSpecialTransition( @@ -1754,6 +1761,7 @@ TEST(PrototypeTransitionFromMapOwningDescriptor) { bool generalizes_representations() const { return !IS_PROTO_TRANS_ISSUE_FIXED; } + bool is_non_equevalent_transition() const { return true; } }; TestConfig config; TestGeneralizeRepresentationWithSpecialTransition( @@ -1798,6 +1806,7 @@ TEST(PrototypeTransitionFromMapNotOwningDescriptor) { bool generalizes_representations() const { return !IS_PROTO_TRANS_ISSUE_FIXED; } + bool is_non_equevalent_transition() const { return true; } }; TestConfig config; TestGeneralizeRepresentationWithSpecialTransition( diff --git a/test/mjsunit/regress/regress-4121.js b/test/mjsunit/regress/regress-4121.js new file mode 100644 index 0000000..3d777c2 --- /dev/null +++ b/test/mjsunit/regress/regress-4121.js @@ -0,0 +1,55 @@ +// 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 + +function Migrator(o) { + return o.foo; +} +function Loader(o) { + return o[0]; +} + +var first_smi_array = [1]; +var second_smi_array = [2]; +var first_object_array = ["first"]; +var second_object_array = ["string"]; + +assertTrue(%HasFastSmiElements(first_smi_array)); +assertTrue(%HasFastSmiElements(second_smi_array)); +assertTrue(%HasFastObjectElements(first_object_array)); +assertTrue(%HasFastObjectElements(second_object_array)); + +// Prepare identical transition chains for smi and object arrays. +first_smi_array.foo = 0; +second_smi_array.foo = 0; +first_object_array.foo = 0; +second_object_array.foo = 0; + +// Collect type feedback for not-yet-deprecated original object array map. +for (var i = 0; i < 3; i++) Migrator(second_object_array); + +// Blaze a migration trail for smi array maps. +// This marks the migrated smi array map as a migration target. +first_smi_array.foo = 0.5; +print(second_smi_array.foo); + +// Deprecate original object array map. +// Use TryMigrate from deferred optimized code to migrate second object array. +first_object_array.foo = 0.5; +%OptimizeFunctionOnNextCall(Migrator); +Migrator(second_object_array); + +// |second_object_array| now erroneously has a smi map. +// Optimized code assuming smi elements will expose this. + +for (var i = 0; i < 3; i++) Loader(second_smi_array); +%OptimizeFunctionOnNextCall(Loader); +assertEquals("string", Loader(second_object_array)); + +// Any of the following checks will also fail: +assertTrue(%HasFastObjectElements(second_object_array)); +assertFalse(%HasFastSmiElements(second_object_array)); +assertTrue(%HaveSameMap(first_object_array, second_object_array)); +assertFalse(%HaveSameMap(first_smi_array, second_object_array)); -- 2.7.4