From 8dffcb9efa3e4623815c20fd57e8c1cd0992caa7 Mon Sep 17 00:00:00 2001 From: "christian.plesner.hansen@gmail.com" Date: Fri, 13 Mar 2009 13:43:07 +0000 Subject: [PATCH] Flush ICs when adding setters to an object or setting a __proto__ to an object that holds a setter. If there are no store ics then no flushing is done. The implementation has been tweaked so that no ICs are cleared during normal context creation. This may cost us some performance but I'm submitting it as it is and if there are problems we can either decide to be smarter about when, what and/or how we clear, or back this change out altogether. git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1509 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/accessors.cc | 10 +++- src/heap.cc | 11 ++++ src/heap.h | 13 +++++ src/ic.cc | 1 + src/messages.js | 8 +-- src/objects.cc | 52 +++++++++++++++-- src/objects.h | 9 ++- src/regexp-delay.js | 31 +++++----- src/runtime.cc | 12 +++- src/v8-counters.h | 3 +- .../bug-1344252.js => regress/regress-1344252.js} | 5 +- test/mjsunit/regress/regress-92.js | 67 ++++++++++++++++++++++ 12 files changed, 190 insertions(+), 32 deletions(-) rename test/mjsunit/{bugs/bug-1344252.js => regress/regress-1344252.js} (98%) create mode 100644 test/mjsunit/regress/regress-92.js diff --git a/src/accessors.cc b/src/accessors.cc index d779eb2..303aa43 100644 --- a/src/accessors.cc +++ b/src/accessors.cc @@ -509,13 +509,17 @@ Object* Accessors::ObjectSetPrototype(JSObject* receiver, // SpiderMonkey behaves this way. if (!value->IsJSObject() && !value->IsNull()) return value; + bool clear_ics = false; for (Object* pt = value; pt != Heap::null_value(); pt = pt->GetPrototype()) { - if (JSObject::cast(pt) == receiver) { + JSObject *obj = JSObject::cast(pt); + if (obj == receiver) { // Cycle detected. HandleScope scope; return Top::Throw(*Factory::NewError("cyclic_proto", HandleVector(NULL, 0))); } + if (obj->HasLocalPropertyWithType(CALLBACKS)) + clear_ics = true; } // Find the first object in the chain whose prototype object is not @@ -534,6 +538,10 @@ Object* Accessors::ObjectSetPrototype(JSObject* receiver, Map::cast(new_map)->set_prototype(value); current->set_map(Map::cast(new_map)); + // Finally, if the prototype contains a setter we may have broken + // the assumptions made when creating ics so we have to clear them. + if (clear_ics) Heap::ClearStoreICs(); + // To be consistent with other Set functions, return the value. return value; } diff --git a/src/heap.cc b/src/heap.cc index 8159311..481895b 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -72,6 +72,8 @@ int Heap::old_gen_allocation_limit_ = kMinimumAllocationLimit; int Heap::old_gen_exhausted_ = false; +bool Heap::has_store_ics_ = false; + int Heap::amount_of_external_allocated_memory_ = 0; int Heap::amount_of_external_allocated_memory_at_last_global_gc_ = 0; @@ -294,6 +296,14 @@ void Heap::CollectAllGarbage() { } +void Heap::ClearStoreICs() { + if (has_store_ics_) { + Counters::clear_store_ic.Increment(); + CollectAllGarbage(); + } +} + + void Heap::CollectAllGarbageIfContextDisposed() { // If the garbage collector interface is exposed through the global // gc() function, we avoid being clever about forcing GCs when @@ -475,6 +485,7 @@ void Heap::MarkCompactPrologue(bool is_compacting) { void Heap::MarkCompactEpilogue(bool is_compacting) { Top::MarkCompactEpilogue(is_compacting); ThreadManager::MarkCompactEpilogue(is_compacting); + Heap::has_store_ics_ = false; } diff --git a/src/heap.h b/src/heap.h index 6fb28db..9fa4374 100644 --- a/src/heap.h +++ b/src/heap.h @@ -605,6 +605,9 @@ class Heap : public AllStatic { // Performs a full garbage collection. static void CollectAllGarbage(); + // Clears all inline caches by forcing a global garbage collection. + static void ClearStoreICs(); + // Performs a full garbage collection if a context has been disposed // since the last time the check was performed. static void CollectAllGarbageIfContextDisposed(); @@ -807,6 +810,14 @@ class Heap : public AllStatic { > old_gen_allocation_limit_; } + static bool has_store_ics() { + return has_store_ics_; + } + + static void store_ic_created() { + has_store_ics_ = true; + } + private: static int semispace_size_; static int initial_semispace_size_; @@ -872,6 +883,8 @@ class Heap : public AllStatic { // last GC. static int old_gen_exhausted_; + static bool has_store_ics_; + // Declare all the roots #define ROOT_DECLARATION(type, name) static type* name##_; ROOT_LIST(ROOT_DECLARATION) diff --git a/src/ic.cc b/src/ic.cc index d7bd764..048fe3c 100644 --- a/src/ic.cc +++ b/src/ic.cc @@ -912,6 +912,7 @@ void StoreIC::UpdateCaches(LookupResult* lookup, // Patch the call site depending on the state of the cache. if (state == UNINITIALIZED || state == MONOMORPHIC_PROTOTYPE_FAILURE) { + Heap::store_ic_created(); set_target(Code::cast(code)); } else if (state == MONOMORPHIC) { // Only move to mega morphic if the target changes. diff --git a/src/messages.js b/src/messages.js index cd9a1e8..f8b3c51 100644 --- a/src/messages.js +++ b/src/messages.js @@ -602,7 +602,7 @@ var kAddMessageAccessorsMarker = { }; // Defines accessors for a property that is calculated the first time // the property is read and then replaces the accessor with the value. // Also, setting the property causes the accessors to be deleted. -function DefineOneShotAccessor(obj, name, fun) { +function DefineOneShotAccessor(obj, name, fun, never_used) { // Note that the accessors consistently operate on 'obj', not 'this'. // Since the object may occur in someone else's prototype chain we // can't rely on 'this' being the same as 'obj'. @@ -611,10 +611,10 @@ function DefineOneShotAccessor(obj, name, fun) { obj[name] = value; return value; }); - obj.__defineSetter__(name, function (v) { + %DefineAccessor(ToObject(obj), ToString(name), SETTER, function (v) { delete obj[name]; obj[name] = v; - }); + }, 0, never_used); } function DefineError(f) { @@ -648,7 +648,7 @@ function DefineError(f) { if (m === kAddMessageAccessorsMarker) { DefineOneShotAccessor(this, 'message', function (obj) { return FormatMessage({type: obj.type, args: obj.arguments}); - }); + }, true); } else if (!IS_UNDEFINED(m)) { this.message = ToString(m); } diff --git a/src/objects.cc b/src/objects.cc index 5116a7c..15ce483 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -2489,6 +2489,38 @@ void JSObject::LocalLookup(String* name, LookupResult* result) { } +bool JSObject::HasLocalPropertyWithType(PropertyType type) { + if (IsJSGlobalProxy()) { + Object* proto = GetPrototype(); + if (proto->IsNull()) return false; + ASSERT(proto->IsJSGlobalObject()); + return JSObject::cast(proto)->HasLocalPropertyWithType(type); + } + + if (HasFastProperties()) { + DescriptorArray* descriptors = map()->instance_descriptors(); + int length = descriptors->number_of_descriptors(); + for (int i = 0; i < length; i++) { + PropertyDetails details(descriptors->GetDetails(i)); + if (details.type() == type) + return true; + } + } else { + Handle properties = Handle(property_dictionary()); + int capacity = properties->Capacity(); + for (int i = 0; i < capacity; i++) { + if (properties->IsKey(properties->KeyAt(i))) { + PropertyDetails details = properties->DetailsAt(i); + if (details.type() == type) + return true; + } + } + } + + return false; +} + + void JSObject::Lookup(String* name, LookupResult* result) { // Ecma-262 3rd 8.6.2.4 for (Object* current = this; @@ -2616,8 +2648,11 @@ Object* JSObject::DefineGetterSetter(String* name, } -Object* JSObject::DefineAccessor(String* name, bool is_getter, JSFunction* fun, - PropertyAttributes attributes) { +Object* JSObject::DefineAccessor(String* name, + bool is_getter, + JSFunction* fun, + PropertyAttributes attributes, + bool never_used) { // Check access rights if needed. if (IsAccessCheckNeeded() && !Top::MayNamedAccess(this, name, v8::ACCESS_HAS)) { @@ -2629,13 +2664,22 @@ Object* JSObject::DefineAccessor(String* name, bool is_getter, JSFunction* fun, Object* proto = GetPrototype(); if (proto->IsNull()) return this; ASSERT(proto->IsJSGlobalObject()); - return JSObject::cast(proto)->DefineAccessor(name, is_getter, - fun, attributes); + return JSObject::cast(proto)->DefineAccessor(name, + is_getter, + fun, + attributes, + never_used); } Object* array = DefineGetterSetter(name, attributes); if (array->IsFailure() || array->IsUndefined()) return array; FixedArray::cast(array)->set(is_getter ? 0 : 1, fun); + + // Because setters are nonlocal (they're accessible through the + // prototype chain) but our inline caches are local we clear them + // when a new setter is introduced. + if (!(is_getter || never_used)) Heap::ClearStoreICs(); + return this; } diff --git a/src/objects.h b/src/objects.h index ffee300..e6be74b 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1200,8 +1200,13 @@ class JSObject: public HeapObject { String* name); PropertyAttributes GetLocalPropertyAttribute(String* name); + // Defines an accessor. This may violate some of the assumptions we + // make when setting up ics so unless it is guaranteed that no ics + // exist for this property we have to clear all ics. Set the 'never_used' + // flag to true if you know there can be no ics. Object* DefineAccessor(String* name, bool is_getter, JSFunction* fun, - PropertyAttributes attributes); + PropertyAttributes attributes, bool never_used); + Object* LookupAccessor(String* name, bool is_getter); // Used from Object::GetProperty(). @@ -1273,6 +1278,8 @@ class JSObject: public HeapObject { inline bool HasNamedInterceptor(); inline bool HasIndexedInterceptor(); + bool HasLocalPropertyWithType(PropertyType type); + // Support functions for v8 api (needed for correct interceptor behavior). bool HasRealNamedProperty(String* key); bool HasRealElementProperty(uint32_t index); diff --git a/src/regexp-delay.js b/src/regexp-delay.js index 99f9208..e80c39f 100644 --- a/src/regexp-delay.js +++ b/src/regexp-delay.js @@ -357,12 +357,15 @@ function SetupRegExp() { ToString(string); }; + // All these accessors are set with the 'never_used' flag set to true. + // This is because at this point there can be no existing ics for setting + // these properties and so we don't have to bother clearing them. %DefineAccessor($RegExp, 'input', GETTER, RegExpGetInput, DONT_DELETE); - %DefineAccessor($RegExp, 'input', SETTER, RegExpSetInput, DONT_DELETE); + %DefineAccessor($RegExp, 'input', SETTER, RegExpSetInput, DONT_DELETE, true); %DefineAccessor($RegExp, '$_', GETTER, RegExpGetInput, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, '$_', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, '$_', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE, true); %DefineAccessor($RegExp, '$input', GETTER, RegExpGetInput, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, '$input', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, '$input', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE, true); // The properties multiline and $* are aliases for each other. When this // value is set in SpiderMonkey, the value it is set to is coerced to a @@ -377,9 +380,9 @@ function SetupRegExp() { function RegExpSetMultiline(flag) { multiline = flag ? true : false; }; %DefineAccessor($RegExp, 'multiline', GETTER, RegExpGetMultiline, DONT_DELETE); - %DefineAccessor($RegExp, 'multiline', SETTER, RegExpSetMultiline, DONT_DELETE); + %DefineAccessor($RegExp, 'multiline', SETTER, RegExpSetMultiline, DONT_DELETE, true); %DefineAccessor($RegExp, '$*', GETTER, RegExpGetMultiline, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, '$*', SETTER, RegExpSetMultiline, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, '$*', SETTER, RegExpSetMultiline, DONT_ENUM | DONT_DELETE, true); function NoOpSetter(ignored) {} @@ -387,25 +390,25 @@ function SetupRegExp() { // Static properties set by a successful match. %DefineAccessor($RegExp, 'lastMatch', GETTER, RegExpGetLastMatch, DONT_DELETE); - %DefineAccessor($RegExp, 'lastMatch', SETTER, NoOpSetter, DONT_DELETE); + %DefineAccessor($RegExp, 'lastMatch', SETTER, NoOpSetter, DONT_DELETE, true); %DefineAccessor($RegExp, '$&', GETTER, RegExpGetLastMatch, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, '$&', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, '$&', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true); %DefineAccessor($RegExp, 'lastParen', GETTER, RegExpGetLastParen, DONT_DELETE); - %DefineAccessor($RegExp, 'lastParen', SETTER, NoOpSetter, DONT_DELETE); + %DefineAccessor($RegExp, 'lastParen', SETTER, NoOpSetter, DONT_DELETE, true); %DefineAccessor($RegExp, '$+', GETTER, RegExpGetLastParen, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, '$+', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, '$+', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true); %DefineAccessor($RegExp, 'leftContext', GETTER, RegExpGetLeftContext, DONT_DELETE); - %DefineAccessor($RegExp, 'leftContext', SETTER, NoOpSetter, DONT_DELETE); + %DefineAccessor($RegExp, 'leftContext', SETTER, NoOpSetter, DONT_DELETE, true); %DefineAccessor($RegExp, '$`', GETTER, RegExpGetLeftContext, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, '$`', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, '$`', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true); %DefineAccessor($RegExp, 'rightContext', GETTER, RegExpGetRightContext, DONT_DELETE); - %DefineAccessor($RegExp, 'rightContext', SETTER, NoOpSetter, DONT_DELETE); + %DefineAccessor($RegExp, 'rightContext', SETTER, NoOpSetter, DONT_DELETE, true); %DefineAccessor($RegExp, "$'", GETTER, RegExpGetRightContext, DONT_ENUM | DONT_DELETE); - %DefineAccessor($RegExp, "$'", SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineAccessor($RegExp, "$'", SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true); for (var i = 1; i < 10; ++i) { %DefineAccessor($RegExp, '$' + i, GETTER, RegExpMakeCaptureGetter(i), DONT_DELETE); - %DefineAccessor($RegExp, '$' + i, SETTER, NoOpSetter, DONT_DELETE); + %DefineAccessor($RegExp, '$' + i, SETTER, NoOpSetter, DONT_DELETE, true); } } diff --git a/src/runtime.cc b/src/runtime.cc index 454aeb4..0c5d433 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -5143,10 +5143,10 @@ static Object* Runtime_GetArrayKeys(Arguments args) { // and setter on the first call to DefineAccessor and ignored on // subsequent calls. static Object* Runtime_DefineAccessor(Arguments args) { - RUNTIME_ASSERT(args.length() == 4 || args.length() == 5); + RUNTIME_ASSERT(4 <= args.length() && args.length() <= 6); // Compute attributes. PropertyAttributes attributes = NONE; - if (args.length() == 5) { + if (args.length() >= 5) { CONVERT_CHECKED(Smi, attrs, args[4]); int value = attrs->value(); // Only attribute bits should be set. @@ -5154,11 +5154,17 @@ static Object* Runtime_DefineAccessor(Arguments args) { attributes = static_cast(value); } + bool never_used = (args.length() == 6) && (args[5] == Heap::true_value()); + CONVERT_CHECKED(JSObject, obj, args[0]); CONVERT_CHECKED(String, name, args[1]); CONVERT_CHECKED(Smi, flag, args[2]); CONVERT_CHECKED(JSFunction, fun, args[3]); - return obj->DefineAccessor(name, flag->value() == 0, fun, attributes); + return obj->DefineAccessor(name, + flag->value() == 0, + fun, + attributes, + never_used); } diff --git a/src/v8-counters.h b/src/v8-counters.h index acd3b23..69de359 100644 --- a/src/v8-counters.h +++ b/src/v8-counters.h @@ -122,7 +122,8 @@ namespace v8 { namespace internal { SC(enum_cache_misses, V8.EnumCacheMisses) \ SC(reloc_info_count, V8.RelocInfoCount) \ SC(reloc_info_size, V8.RelocInfoSize) \ - SC(zone_segment_bytes, V8.ZoneSegmentBytes) + SC(zone_segment_bytes, V8.ZoneSegmentBytes) \ + SC(clear_store_ic, V8.ClearStoreIC) // This file contains all the v8 counters that are in use. diff --git a/test/mjsunit/bugs/bug-1344252.js b/test/mjsunit/regress/regress-1344252.js similarity index 98% rename from test/mjsunit/bugs/bug-1344252.js rename to test/mjsunit/regress/regress-1344252.js index 1723834..1686f80 100644 --- a/test/mjsunit/bugs/bug-1344252.js +++ b/test/mjsunit/regress/regress-1344252.js @@ -52,14 +52,12 @@ assertTrue(typeof f.x == 'undefined'); var result_y; var proto = new Object(); proto.__defineSetter__('y', function (value) { result_y = value; }); -var f = new F(); -f.y = undefined; +var f = { }; f.__proto__ = proto; F.call(f); assertEquals(87, result_y); assertTrue(typeof f.y == 'undefined'); - // Test the same issue in the runtime system. Make sure that // accessors added to the prototype chain are called instead of // following map transitions. @@ -76,4 +74,3 @@ Object.prototype.__defineSetter__('z', function(value) { result_z = value; }); o2.z = 27; assertEquals(27, result_z); assertTrue(typeof o2.z == 'undefined'); - diff --git a/test/mjsunit/regress/regress-92.js b/test/mjsunit/regress/regress-92.js new file mode 100644 index 0000000..a774139 --- /dev/null +++ b/test/mjsunit/regress/regress-92.js @@ -0,0 +1,67 @@ +// Copyright 2009 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. + +function introduceSetter(useProto, Constructor) { + // Before introducing the setter this test expects 'y' to be set + // normally. Afterwards setting 'y' will throw an exception. + var runTest = new Function("Constructor", + "var p = new Constructor(3); p.y = 4; assertEquals(p.y, 4);"); + + // Create the prototype object first because defining a setter should + // clear inline caches. + if (useProto) { + var newProto = { }; + newProto.__defineSetter__('y', function () { throw signal; }); + } + + // Ensure that monomorphic ics have been set up. + runTest(Constructor); + runTest(Constructor); + + var signal = "was called"; + if (useProto) { + // Either introduce the setter through __proto__... + Constructor.prototype.__proto__ = newProto; + } else { + // ...or introduce it directly using __defineSetter__. + Constructor.prototype.__defineSetter__('y', function () { throw signal; }); + } + + // Now setting 'y' should throw an exception. + try { + runTest(Constructor); + fail("Accessor was not called."); + } catch (e) { + assertEquals(e, signal); + } + +} + +introduceSetter(false, function FastCase(x) { this.x = x; }); +introduceSetter(true, function FastCase(x) { this.x = x; }); +introduceSetter(false, function SlowCase(x) { this.x = x; delete this.x; }); +introduceSetter(true, function SlowCase(x) { this.x = x; delete this.x; }); -- 2.7.4