From bc8c41c08d64f5d051ae82db54d59ae2a167f430 Mon Sep 17 00:00:00 2001 From: "ishell@chromium.org" Date: Thu, 13 Nov 2014 11:56:13 +0100 Subject: [PATCH] Avoid fast short-cut in Map::GeneralizeRepresentation() for literals with non-simple transitions. It started showing after r25253. BUG=v8:3687 LOG=N R=verwaest@chromium.org Review URL: https://codereview.chromium.org/715313003 Cr-Commit-Position: refs/heads/master@{#25324} --- src/json-parser.h | 3 ++- src/objects.cc | 44 ++++++++++++++++++++++++------------ src/objects.h | 5 ++-- test/mjsunit/regress/regress-3687.js | 22 ++++++++++++++++++ 4 files changed, 56 insertions(+), 18 deletions(-) create mode 100644 test/mjsunit/regress/regress-3687.js diff --git a/src/json-parser.h b/src/json-parser.h index 2993249..5ebbcdd 100644 --- a/src/json-parser.h +++ b/src/json-parser.h @@ -400,7 +400,8 @@ Handle JsonParser::ParseJsonObject() { descriptor)->NowContains(value)) { Handle value_type(value->OptimalType( isolate(), expected_representation)); - Map::GeneralizeFieldType(target, descriptor, value_type); + Map::GeneralizeFieldType(target, descriptor, + expected_representation, value_type); } DCHECK(target->instance_descriptors()->GetFieldType( descriptor)->NowContains(value)); diff --git a/src/objects.cc b/src/objects.cc index 85b14f4..047adc5 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -2354,6 +2354,7 @@ Map* Map::FindFieldOwner(int descriptor) { void Map::UpdateFieldType(int descriptor, Handle name, + Representation new_representation, Handle new_type) { DisallowHeapAllocation no_allocation; PropertyDetails details = instance_descriptors()->GetDetails(descriptor); @@ -2361,13 +2362,18 @@ void Map::UpdateFieldType(int descriptor, Handle name, if (HasTransitionArray()) { TransitionArray* transitions = this->transitions(); for (int i = 0; i < transitions->number_of_transitions(); ++i) { - transitions->GetTarget(i)->UpdateFieldType(descriptor, name, new_type); + transitions->GetTarget(i) + ->UpdateFieldType(descriptor, name, new_representation, new_type); } } + // It is allowed to change representation here only from None to something. + DCHECK(details.representation().Equals(new_representation) || + details.representation().IsNone()); + // Skip if already updated the shared descriptor. if (instance_descriptors()->GetFieldType(descriptor) == *new_type) return; FieldDescriptor d(name, instance_descriptors()->GetFieldIndex(descriptor), - new_type, details.attributes(), details.representation()); + new_type, details.attributes(), new_representation); instance_descriptors()->Replace(descriptor, &d); } @@ -2393,15 +2399,20 @@ Handle Map::GeneralizeFieldType(Handle type1, // static -void Map::GeneralizeFieldType(Handle map, - int modify_index, +void Map::GeneralizeFieldType(Handle map, int modify_index, + Representation new_representation, Handle new_field_type) { Isolate* isolate = map->GetIsolate(); // Check if we actually need to generalize the field type at all. - Handle old_field_type( - map->instance_descriptors()->GetFieldType(modify_index), isolate); - if (new_field_type->NowIs(old_field_type)) { + Handle old_descriptors(map->instance_descriptors(), isolate); + Representation old_representation = + old_descriptors->GetDetails(modify_index).representation(); + Handle old_field_type(old_descriptors->GetFieldType(modify_index), + isolate); + + if (old_representation.Equals(new_representation) && + new_field_type->NowIs(old_field_type)) { DCHECK(Map::GeneralizeFieldType(old_field_type, new_field_type, isolate)->NowIs(old_field_type)); @@ -2420,7 +2431,8 @@ void Map::GeneralizeFieldType(Handle map, PropertyDetails details = descriptors->GetDetails(modify_index); Handle name(descriptors->GetKey(modify_index)); - field_owner->UpdateFieldType(modify_index, name, new_field_type); + field_owner->UpdateFieldType(modify_index, name, new_representation, + new_field_type); field_owner->dependent_code()->DeoptimizeDependentCodeGroup( isolate, DependentCode::kFieldTypeGroup); @@ -2471,12 +2483,9 @@ Handle Map::GeneralizeRepresentation(Handle old_map, // modification to the object, because the default uninitialized value for // representation None can be overwritten by both smi and tagged values. // Doubles, however, would require a box allocation. - if (old_representation.IsNone() && - !new_representation.IsNone() && + if (old_representation.IsNone() && !new_representation.IsNone() && !new_representation.IsDouble()) { DCHECK(old_details.type() == FIELD); - DCHECK(old_descriptors->GetFieldType(modify_index)->NowIs( - HeapType::None())); if (FLAG_trace_generalization) { old_map->PrintGeneralization( stdout, "uninitialized field", @@ -2485,8 +2494,13 @@ Handle Map::GeneralizeRepresentation(Handle old_map, old_representation, new_representation, old_descriptors->GetFieldType(modify_index), *new_field_type); } - old_descriptors->SetRepresentation(modify_index, new_representation); - old_descriptors->SetValue(modify_index, *new_field_type); + Handle field_owner(old_map->FindFieldOwner(modify_index), isolate); + + GeneralizeFieldType(field_owner, modify_index, new_representation, + new_field_type); + DCHECK(old_descriptors->GetDetails(modify_index).representation().Equals( + new_representation)); + DCHECK(old_descriptors->GetFieldType(modify_index)->NowIs(new_field_type)); return old_map; } @@ -2547,7 +2561,7 @@ Handle Map::GeneralizeRepresentation(Handle old_map, old_field_type = GeneralizeFieldType( new_field_type, old_field_type, isolate); } - GeneralizeFieldType(tmp_map, i, old_field_type); + GeneralizeFieldType(tmp_map, i, tmp_representation, old_field_type); } else if (tmp_type == CONSTANT) { if (old_type != CONSTANT || old_descriptors->GetConstant(i) != tmp_descriptors->GetConstant(i)) { diff --git a/src/objects.h b/src/objects.h index f458a54..5a42c73 100644 --- a/src/objects.h +++ b/src/objects.h @@ -5825,8 +5825,8 @@ class Map: public HeapObject { Handle type1, Handle type2, Isolate* isolate); - static void GeneralizeFieldType(Handle map, - int modify_index, + static void GeneralizeFieldType(Handle map, int modify_index, + Representation new_representation, Handle new_field_type); static Handle GeneralizeRepresentation( Handle map, @@ -6377,6 +6377,7 @@ class Map: public HeapObject { Map* FindLastMatchMap(int verbatim, int length, DescriptorArray* descriptors); void UpdateFieldType(int descriptor_number, Handle name, + Representation new_representation, Handle new_type); void PrintGeneralization(FILE* file, diff --git a/test/mjsunit/regress/regress-3687.js b/test/mjsunit/regress/regress-3687.js new file mode 100644 index 0000000..e1df1b4 --- /dev/null +++ b/test/mjsunit/regress/regress-3687.js @@ -0,0 +1,22 @@ +// Copyright 2014 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 + +var t1 = { f1: 0 }; +var t2 = { f2: 0 }; + +var z = { + x: { + x: t1, + y: { + x: {}, + z1: { + x: t2, + y: 1 + } + } + }, + z2: 0 +}; -- 2.7.4