Reviewers: Toon Verwaest,
Message:
PTAL
Description:
Map::TryUpdate() must be in sync with Map::Update().
This CL fixes elements kind transitions handling in Map::TryUpdate().
BUG=v8:4121
LOG=Y
Please review this at https://codereview.chromium.org/1181163002/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+81, -8 lines):
M src/objects.cc
M test/cctest/test-migrations.cc
A test/mjsunit/regress/regress-4121.js
Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index
f363ffae9630e5d9d81c5a9e5b3c614c9ef6bc36..3e6d1146441ec61eb457e6d09c93375b8bf4ebb1
100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2420,7 +2420,7 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map>
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> Map::TryUpdate(Handle<Map> old_map)
{
// Check the state of the root map.
Map* root_map = old_map->FindRootMap();
if (!old_map->EquivalentToForTransition(root_map)) return
MaybeHandle<Map>();
+
+ 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<Map>();
+ // 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();
Index: test/cctest/test-migrations.cc
diff --git a/test/cctest/test-migrations.cc b/test/cctest/test-migrations.cc
index
a5eee5d7df0079cb62f6f8815923f70a545f228d..e80ec7c06c948e61b25a92ab7d312bc295ebd2ef
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<Map> 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(
Index: test/mjsunit/regress/regress-4121.js
diff --git a/test/mjsunit/regress/regress-4121.js
b/test/mjsunit/regress/regress-4121.js
new file mode 100644
index
0000000000000000000000000000000000000000..3d777c26b355054d71cd0f0efea6a5e203af4b8a
--- /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));
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
