From eef3bd7c04cb5fcbd621fb9766261636e030a80c Mon Sep 17 00:00:00 2001 From: "kaznacheev@chromium.org" Date: Thu, 16 Sep 2010 10:55:37 +0000 Subject: [PATCH] Prevent inline constructor generation when duplicate properties are present in the constructor. Currenly the constructor like this: function f() { this.a = 0; this.a = 1; this.a = 2; } creates a map with duplicate desciptors which is bad in many ways. Review URL: http://codereview.chromium.org/3434004 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@5476 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/heap.cc | 57 +++++++++++++++++++++++--------- src/objects.cc | 13 +++++++- src/objects.h | 9 +++++ test/mjsunit/this-property-assignment.js | 41 +++++++++++++++++++++++ 4 files changed, 103 insertions(+), 17 deletions(-) create mode 100644 test/mjsunit/this-property-assignment.js diff --git a/src/heap.cc b/src/heap.cc index 650800f..905d065 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -2650,6 +2650,20 @@ Object* Heap::AllocateArgumentsObject(Object* callee, int length) { } +static bool HasDuplicates(DescriptorArray* descriptors) { + int count = descriptors->number_of_descriptors(); + if (count > 1) { + String* prev_key = descriptors->GetKey(0); + for (int i = 1; i != count; i++) { + String* current_key = descriptors->GetKey(i); + if (prev_key == current_key) return true; + prev_key = current_key; + } + } + return false; +} + + Object* Heap::AllocateInitialMap(JSFunction* fun) { ASSERT(!fun->has_initial_map()); @@ -2683,23 +2697,34 @@ Object* Heap::AllocateInitialMap(JSFunction* fun) { if (fun->shared()->CanGenerateInlineConstructor(prototype)) { int count = fun->shared()->this_property_assignments_count(); if (count > in_object_properties) { - count = in_object_properties; - } - Object* descriptors_obj = DescriptorArray::Allocate(count); - if (descriptors_obj->IsFailure()) return descriptors_obj; - DescriptorArray* descriptors = DescriptorArray::cast(descriptors_obj); - for (int i = 0; i < count; i++) { - String* name = fun->shared()->GetThisPropertyAssignmentName(i); - ASSERT(name->IsSymbol()); - FieldDescriptor field(name, i, NONE); - field.SetEnumerationIndex(i); - descriptors->Set(i, &field); + // Inline constructor can only handle inobject properties. + fun->shared()->ForbidInlineConstructor(); + } else { + Object* descriptors_obj = DescriptorArray::Allocate(count); + if (descriptors_obj->IsFailure()) return descriptors_obj; + DescriptorArray* descriptors = DescriptorArray::cast(descriptors_obj); + for (int i = 0; i < count; i++) { + String* name = fun->shared()->GetThisPropertyAssignmentName(i); + ASSERT(name->IsSymbol()); + FieldDescriptor field(name, i, NONE); + field.SetEnumerationIndex(i); + descriptors->Set(i, &field); + } + descriptors->SetNextEnumerationIndex(count); + descriptors->SortUnchecked(); + + // The descriptors may contain duplicates because the compiler does not + // guarantee the uniqueness of property names (it would have required + // quadratic time). Once the descriptors are sorted we can check for + // duplicates in linear time. + if (HasDuplicates(descriptors)) { + fun->shared()->ForbidInlineConstructor(); + } else { + map->set_instance_descriptors(descriptors); + map->set_pre_allocated_property_fields(count); + map->set_unused_property_fields(in_object_properties - count); + } } - descriptors->SetNextEnumerationIndex(count); - descriptors->Sort(); - map->set_instance_descriptors(descriptors); - map->set_pre_allocated_property_fields(count); - map->set_unused_property_fields(in_object_properties - count); } return map; } diff --git a/src/objects.cc b/src/objects.cc index ef51851..f77800a 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -3825,7 +3825,7 @@ Object* DescriptorArray::RemoveTransitions() { } -void DescriptorArray::Sort() { +void DescriptorArray::SortUnchecked() { // In-place heap sort. int len = number_of_descriptors(); @@ -3875,7 +3875,11 @@ void DescriptorArray::Sort() { parent_index = child_index; } } +} + +void DescriptorArray::Sort() { + SortUnchecked(); SLOW_ASSERT(IsSortedNoDuplicates()); } @@ -5269,6 +5273,13 @@ bool SharedFunctionInfo::CanGenerateInlineConstructor(Object* prototype) { } +void SharedFunctionInfo::ForbidInlineConstructor() { + set_compiler_hints(BooleanBit::set(compiler_hints(), + kHasOnlySimpleThisPropertyAssignments, + false)); +} + + void SharedFunctionInfo::SetThisPropertyAssignmentsInfo( bool only_simple_this_property_assignments, FixedArray* assignments) { diff --git a/src/objects.h b/src/objects.h index 7f6538c..1036a5a 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1892,6 +1892,11 @@ class DescriptorArray: public FixedArray { MUST_USE_RESULT Object* RemoveTransitions(); // Sort the instance descriptors by the hash codes of their keys. + // Does not check for duplicates. + void SortUnchecked(); + + // Sort the instance descriptors by the hash codes of their keys. + // Checks the result for duplicates. void Sort(); // Search the instance descriptors for given name. @@ -3542,6 +3547,10 @@ class SharedFunctionInfo: public HeapObject { // prototype. bool CanGenerateInlineConstructor(Object* prototype); + // Prevents further attempts to generate inline constructors. + // To be called if generation failed for any reason. + void ForbidInlineConstructor(); + // For functions which only contains this property assignments this provides // access to the names for the properties assigned. DECL_ACCESSORS(this_property_assignments, Object) diff --git a/test/mjsunit/this-property-assignment.js b/test/mjsunit/this-property-assignment.js new file mode 100644 index 0000000..c681999 --- /dev/null +++ b/test/mjsunit/this-property-assignment.js @@ -0,0 +1,41 @@ +// Copyright 2010 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Tests the handling of multiple assignments to the same property in a +// constructor that only has simple this property assignments. + +function Node() { + this.a = 1; + this.a = 2; + this.a = 3; +} + +var n1 = new Node(); +assertEquals(3, n1.a); + +var n2 = new Node(); +assertEquals(3, n2.a); -- 2.7.4