Revert change 1509 that flush ICs when adding setters on an object or

when setting a __proto__ to an object that holds a setter.

This seems to cause a major page load regression, so we need to tune
the clearing.
Review URL: http://codereview.chromium.org/50011

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1548 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/accessors.cc

@@ -509,17 +509,13 @@ 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()) {
-    JSObject *obj = JSObject::cast(pt);
-    if (obj == receiver) {
+    if (JSObject::cast(pt) == receiver) {
       // Cycle detected.
       HandleScope scope;
       return Top::Throw(*Factory::NewError("cyclic_proto",
                                            HandleVector<Object>(NULL, 0)));
     }
-    if (obj->HasLocalPropertyWithType(CALLBACKS))
-      clear_ics = true;
   }
 
   // Find the first object in the chain whose prototype object is not
@@ -538,10 +534,6 @@ 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;
 }

src/heap.cc

@@ -71,8 +71,6 @@ 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,14 +292,6 @@ 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
@@ -482,7 +472,6 @@ 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;
 }
 
 

src/heap.h

@@ -606,9 +606,6 @@ 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();
@@ -811,14 +808,6 @@ 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_;
@@ -884,8 +873,6 @@ 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)

src/ic.cc

@@ -912,7 +912,6 @@ 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.

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, never_used) {
+function DefineOneShotAccessor(obj, name, fun) {
   // 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, never_used) {
     obj[name] = value;
     return value;
   });
-  %DefineAccessor(ToObject(obj), ToString(name), SETTER, function (v) {
+  obj.__defineSetter__(name, 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);
       }

src/objects.cc

@@ -2483,38 +2483,6 @@ 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<Dictionary> properties = Handle<Dictionary>(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;
@@ -2642,11 +2610,8 @@ Object* JSObject::DefineGetterSetter(String* name,
 }
 
 
-Object* JSObject::DefineAccessor(String* name,
-                                 bool is_getter,
-                                 JSFunction* fun,
-                                 PropertyAttributes attributes,
-                                 bool never_used) {
+Object* JSObject::DefineAccessor(String* name, bool is_getter, JSFunction* fun,
+                                 PropertyAttributes attributes) {
   // Check access rights if needed.
   if (IsAccessCheckNeeded() &&
       !Top::MayNamedAccess(this, name, v8::ACCESS_HAS)) {
@@ -2658,22 +2623,13 @@ Object* JSObject::DefineAccessor(String* name,
     Object* proto = GetPrototype();
     if (proto->IsNull()) return this;
     ASSERT(proto->IsJSGlobalObject());
-    return JSObject::cast(proto)->DefineAccessor(name,
-                                                 is_getter,
-                                                 fun,
-                                                 attributes,
-                                                 never_used);
+    return JSObject::cast(proto)->DefineAccessor(name, is_getter,
+                                                 fun, attributes);
   }
 
   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;
 }
 

src/objects.h

@@ -1200,13 +1200,8 @@ 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, bool never_used);
-
+                         PropertyAttributes attributes);
   Object* LookupAccessor(String* name, bool is_getter);
 
   // Used from Object::GetProperty().
@@ -1278,8 +1273,6 @@ 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);

src/regexp-delay.js

@@ -356,15 +356,12 @@ function SetupRegExp() {
     LAST_INPUT(lastMatchInfo) = 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, true);
+  %DefineAccessor($RegExp, 'input', SETTER, RegExpSetInput, DONT_DELETE);
   %DefineAccessor($RegExp, '$_', GETTER, RegExpGetInput, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, '$_', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, '$_', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE);
   %DefineAccessor($RegExp, '$input', GETTER, RegExpGetInput, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, '$input', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, '$input', SETTER, RegExpSetInput, DONT_ENUM | DONT_DELETE);
 
   // 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
@@ -379,9 +376,9 @@ function SetupRegExp() {
   function RegExpSetMultiline(flag) { multiline = flag ? true : false; };
 
   %DefineAccessor($RegExp, 'multiline', GETTER, RegExpGetMultiline, DONT_DELETE);
-  %DefineAccessor($RegExp, 'multiline', SETTER, RegExpSetMultiline, DONT_DELETE, true);
+  %DefineAccessor($RegExp, 'multiline', SETTER, RegExpSetMultiline, DONT_DELETE);
   %DefineAccessor($RegExp, '$*', GETTER, RegExpGetMultiline, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, '$*', SETTER, RegExpSetMultiline, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, '$*', SETTER, RegExpSetMultiline, DONT_ENUM | DONT_DELETE);
 
 
   function NoOpSetter(ignored) {}
@@ -389,25 +386,25 @@ function SetupRegExp() {
 
   // Static properties set by a successful match.
   %DefineAccessor($RegExp, 'lastMatch', GETTER, RegExpGetLastMatch, DONT_DELETE);
-  %DefineAccessor($RegExp, 'lastMatch', SETTER, NoOpSetter, DONT_DELETE, true);
+  %DefineAccessor($RegExp, 'lastMatch', SETTER, NoOpSetter, DONT_DELETE);
   %DefineAccessor($RegExp, '$&', GETTER, RegExpGetLastMatch, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, '$&', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, '$&', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE);
   %DefineAccessor($RegExp, 'lastParen', GETTER, RegExpGetLastParen, DONT_DELETE);
-  %DefineAccessor($RegExp, 'lastParen', SETTER, NoOpSetter, DONT_DELETE, true);
+  %DefineAccessor($RegExp, 'lastParen', SETTER, NoOpSetter, DONT_DELETE);
   %DefineAccessor($RegExp, '$+', GETTER, RegExpGetLastParen, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, '$+', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, '$+', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE);
   %DefineAccessor($RegExp, 'leftContext', GETTER, RegExpGetLeftContext, DONT_DELETE);
-  %DefineAccessor($RegExp, 'leftContext', SETTER, NoOpSetter, DONT_DELETE, true);
+  %DefineAccessor($RegExp, 'leftContext', SETTER, NoOpSetter, DONT_DELETE);
   %DefineAccessor($RegExp, '$`', GETTER, RegExpGetLeftContext, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, '$`', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, '$`', SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE);
   %DefineAccessor($RegExp, 'rightContext', GETTER, RegExpGetRightContext, DONT_DELETE);
-  %DefineAccessor($RegExp, 'rightContext', SETTER, NoOpSetter, DONT_DELETE, true);
+  %DefineAccessor($RegExp, 'rightContext', SETTER, NoOpSetter, DONT_DELETE);
   %DefineAccessor($RegExp, "$'", GETTER, RegExpGetRightContext, DONT_ENUM | DONT_DELETE);
-  %DefineAccessor($RegExp, "$'", SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE, true);
+  %DefineAccessor($RegExp, "$'", SETTER, NoOpSetter, DONT_ENUM | DONT_DELETE);
 
   for (var i = 1; i < 10; ++i) {
     %DefineAccessor($RegExp, '$' + i, GETTER, RegExpMakeCaptureGetter(i), DONT_DELETE);
-    %DefineAccessor($RegExp, '$' + i, SETTER, NoOpSetter, DONT_DELETE, true);
+    %DefineAccessor($RegExp, '$' + i, SETTER, NoOpSetter, DONT_DELETE);
   }
 }
 

src/runtime.cc

@@ -5074,10 +5074,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(4 <= args.length() && args.length() <= 6);
+  RUNTIME_ASSERT(args.length() == 4 || args.length() == 5);
   // 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.
@@ -5085,17 +5085,11 @@ static Object* Runtime_DefineAccessor(Arguments args) {
     attributes = static_cast<PropertyAttributes>(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,
-                             never_used);
+  return obj->DefineAccessor(name, flag->value() == 0, fun, attributes);
 }
 
 

src/v8-counters.h

@@ -122,8 +122,7 @@ 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(clear_store_ic, V8.ClearStoreIC)
+  SC(zone_segment_bytes, V8.ZoneSegmentBytes)
 
 
 // This file contains all the v8 counters that are in use.

test/mjsunit/regress/regress-1344252.js → test/mjsunit/bugs/bug-1344252.js

@@ -52,12 +52,14 @@ assertTrue(typeof f.x == 'undefined');
 var result_y;
 var proto = new Object();
 proto.__defineSetter__('y', function (value) { result_y = value; });
-var f = { };
+var f = new F();
+f.y = undefined;
 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.
@@ -74,3 +76,4 @@ Object.prototype.__defineSetter__('z', function(value) { result_z = value; });
 o2.z = 27;
 assertEquals(27, result_z);
 assertTrue(typeof o2.z == 'undefined');
+

test/mjsunit/regress/regress-92.js

-// 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; });