Re-land: Use map transitions when defining accessor properties.

This is basically r11496, with the following changes:

 * Set back pointers in maps (cherry-picked from r11528)

 * Fixed size calculation in CopyInsert, as proposed by mstarzinger/rossberg

 * DefineFastAccessor uses GetCallbackObject instead of GetValue (for __proto__)

 * Put the code under a new flag, which is disabled by default

 * Cut down the corresponding regression test

 * Adapted bootup memory test, we actually only need a bit more memory on 64bit without snapshots, which can easily explained by more live maps lying around. Note that the snapshot variants are back to their previous limits.

Next steps: Investigate any performance degradationswith the flag enabled, and finally remove the flag when things are OK. Furthermore, GetCallbackObject should be merged into GetValue, the distinction is confusing and error-prone.

Review URL: https://chromiumcodereview.appspot.com/10445009

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

src/flag-definitions.h

@@ -397,6 +397,8 @@ DEFINE_int(random_seed, 0,
 
 // objects.cc
 DEFINE_bool(use_verbose_printer, true, "allows verbose printing")
+DEFINE_bool(fast_accessor_properties, false,
+            "use map transitions for JavaScript accessors")
 
 // parser.cc
 DEFINE_bool(allow_natives_syntax, false, "allow natives syntax")

src/objects.cc

@@ -4433,7 +4433,12 @@ MaybeObject* JSObject::CreateAccessorPairFor(String* name) {
   LookupResult result(GetHeap()->isolate());
   LocalLookupRealNamedProperty(name, &result);
   if (result.IsProperty() && result.type() == CALLBACKS) {
-    ASSERT(!result.IsDontDelete());
+    // Note that the result can actually have IsDontDelete() == true when we
+    // e.g. have to fall back to the slow case while adding a setter after
+    // successfully reusing a map transition for a getter. Nevertheless, this is
+    // OK, because the assertion only holds for the whole addition of both
+    // accessors, not for the addition of each part. See first comment in
+    // DefinePropertyAccessor below.
     Object* obj = result.GetCallbackObject();
     if (obj->IsAccessorPair()) {
       return AccessorPair::cast(obj)->CopyWithoutTransitions();
@@ -4447,6 +4452,29 @@ MaybeObject* JSObject::DefinePropertyAccessor(String* name,
                                               Object* getter,
                                               Object* setter,
                                               PropertyAttributes attributes) {
+  // We could assert that the property is configurable here, but we would need
+  // to do a lookup, which seems to be a bit of overkill.
+  Heap* heap = GetHeap();
+  bool only_attribute_changes = getter->IsNull() && setter->IsNull();
+  if (FLAG_fast_accessor_properties &&
+      HasFastProperties() && !only_attribute_changes) {
+    MaybeObject* getterOk = heap->undefined_value();
+    if (!getter->IsNull()) {
+      getterOk = DefineFastAccessor(name, ACCESSOR_GETTER, getter, attributes);
+      if (getterOk->IsFailure()) return getterOk;
+    }
+
+    MaybeObject* setterOk = heap->undefined_value();
+    if (getterOk != heap->null_value() && !setter->IsNull()) {
+      setterOk = DefineFastAccessor(name, ACCESSOR_SETTER, setter, attributes);
+      if (setterOk->IsFailure()) return setterOk;
+    }
+
+    if (getterOk != heap->null_value() && setterOk != heap->null_value()) {
+      return heap->undefined_value();
+    }
+  }
+
   AccessorPair* accessors;
   { MaybeObject* maybe_accessors = CreateAccessorPairFor(name);
     if (!maybe_accessors->To(&accessors)) return maybe_accessors;
@@ -4596,6 +4624,159 @@ MaybeObject* JSObject::DefineAccessor(String* name,
 }
 
 
+static MaybeObject* CreateFreshAccessor(JSObject* obj,
+                                        String* name,
+                                        AccessorComponent component,
+                                        Object* accessor,
+                                        PropertyAttributes attributes) {
+  // step 1: create a new getter/setter pair with only the accessor in it
+  Heap* heap = obj->GetHeap();
+  AccessorPair* accessors2;
+  { MaybeObject* maybe_accessors2 = heap->AllocateAccessorPair();
+    if (!maybe_accessors2->To(&accessors2)) return maybe_accessors2;
+  }
+  accessors2->set(component, accessor);
+
+  // step 2: create a copy of the descriptors, incl. the new getter/setter pair
+  Map* map1 = obj->map();
+  CallbacksDescriptor callbacks_descr2(name, accessors2, attributes);
+  DescriptorArray* descriptors2;
+  { MaybeObject* maybe_descriptors2 =
+        map1->instance_descriptors()->CopyInsert(&callbacks_descr2,
+                                                 REMOVE_TRANSITIONS);
+    if (!maybe_descriptors2->To(&descriptors2)) return maybe_descriptors2;
+  }
+
+  // step 3: create a new map with the new descriptors
+  Map* map2;
+  { MaybeObject* maybe_map2 = map1->CopyDropDescriptors();
+    if (!maybe_map2->To(&map2)) return maybe_map2;
+  }
+  map2->set_instance_descriptors(descriptors2);
+
+  // step 4: create a new getter/setter pair with a transition to the new map
+  AccessorPair* accessors1;
+  { MaybeObject* maybe_accessors1 = heap->AllocateAccessorPair();
+    if (!maybe_accessors1->To(&accessors1)) return maybe_accessors1;
+  }
+  accessors1->set(component, map2);
+
+  // step 5: create a copy of the descriptors, incl. the new getter/setter pair
+  // with the transition
+  CallbacksDescriptor callbacks_descr1(name, accessors1, attributes);
+  DescriptorArray* descriptors1;
+  { MaybeObject* maybe_descriptors1 =
+        map1->instance_descriptors()->CopyInsert(&callbacks_descr1,
+                                                 KEEP_TRANSITIONS);
+    if (!maybe_descriptors1->To(&descriptors1)) return maybe_descriptors1;
+  }
+
+  // step 6: everything went well so far, so we make our changes visible
+  obj->set_map(map2);
+  map1->set_instance_descriptors(descriptors1);
+  map2->SetBackPointer(map1);
+  return obj;
+}
+
+
+static bool TransitionToSameAccessor(Object* map,
+                                     String* name,
+                                     AccessorComponent component,
+                                     Object* accessor,
+                                     PropertyAttributes attributes ) {
+  DescriptorArray* descs = Map::cast(map)->instance_descriptors();
+  int number = descs->SearchWithCache(name);
+  ASSERT(number != DescriptorArray::kNotFound);
+  Object* target_accessor =
+      AccessorPair::cast(descs->GetCallbacksObject(number))->get(component);
+  PropertyAttributes target_attributes = descs->GetDetails(number).attributes();
+  return target_accessor == accessor && target_attributes == attributes;
+}
+
+
+static MaybeObject* NewCallbackTransition(JSObject* obj,
+                                          String* name,
+                                          AccessorComponent component,
+                                          Object* accessor,
+                                          PropertyAttributes attributes,
+                                          AccessorPair* accessors2) {
+  // step 1: copy the old getter/setter pair and set the new accessor
+  AccessorPair* accessors3;
+  { MaybeObject* maybe_accessors3 = accessors2->CopyWithoutTransitions();
+    if (!maybe_accessors3->To(&accessors3)) return maybe_accessors3;
+  }
+  accessors3->set(component, accessor);
+
+  // step 2: create a copy of the descriptors, incl. the new getter/setter pair
+  Map* map2 = obj->map();
+  CallbacksDescriptor callbacks_descr3(name, accessors3, attributes);
+  DescriptorArray* descriptors3;
+  { MaybeObject* maybe_descriptors3 =
+        map2->instance_descriptors()->CopyInsert(&callbacks_descr3,
+                                                 REMOVE_TRANSITIONS);
+    if (!maybe_descriptors3->To(&descriptors3)) return maybe_descriptors3;
+  }
+
+  // step 3: create a new map with the new descriptors
+  Map* map3;
+  { MaybeObject* maybe_map3 = map2->CopyDropDescriptors();
+    if (!maybe_map3->To(&map3)) return maybe_map3;
+  }
+  map3->set_instance_descriptors(descriptors3);
+
+  // step 4: everything went well so far, so we make our changes visible
+  obj->set_map(map3);
+  accessors2->set(component, map3);
+  map3->SetBackPointer(map2);
+  return obj;
+}
+
+
+MaybeObject* JSObject::DefineFastAccessor(String* name,
+                                          AccessorComponent component,
+                                          Object* accessor,
+                                          PropertyAttributes attributes) {
+  ASSERT(accessor->IsSpecFunction() || accessor->IsUndefined());
+  LookupResult result(GetIsolate());
+  LocalLookup(name, &result);
+
+  // If we have a new property, create a fresh accessor plus a transition to it.
+  if (!result.IsFound()) {
+    return CreateFreshAccessor(this, name, component, accessor, attributes);
+  }
+
+  // If the property is not a JavaScript accessor, fall back to the slow case.
+  if (result.type() != CALLBACKS) return GetHeap()->null_value();
+  Object* callback_value = result.GetCallbackObject();
+  if (!callback_value->IsAccessorPair()) return GetHeap()->null_value();
+  AccessorPair* accessors = AccessorPair::cast(callback_value);
+
+  // Follow a callback transition, if there is a fitting one.
+  Object* entry = accessors->get(component);
+  if (entry->IsMap() &&
+      TransitionToSameAccessor(entry, name, component, accessor, attributes)) {
+    set_map(Map::cast(entry));
+    return this;
+  }
+
+  // When we re-add the same accessor again, there is nothing to do.
+  if (entry == accessor && result.GetAttributes() == attributes) return this;
+
+  // Only the other accessor has been set so far, create a new transition.
+  if (entry->IsTheHole()) {
+    return NewCallbackTransition(this,
+                                 name,
+                                 component,
+                                 accessor,
+                                 attributes,
+                                 accessors);
+  }
+
+  // Nothing from the above worked, so we have to fall back to the slow case.
+  return GetHeap()->null_value();
+}
+
+
 MaybeObject* JSObject::DefineAccessor(AccessorInfo* info) {
   Isolate* isolate = GetIsolate();
   String* name = String::cast(info->name());
@@ -5754,21 +5935,15 @@ MaybeObject* DescriptorArray::CopyInsert(Descriptor* descriptor,
   int index = Search(descriptor->GetKey());
   const bool replacing = (index != kNotFound);
   bool keep_enumeration_index = false;
-  if (replacing) {
-    // We are replacing an existing descriptor.  We keep the enumeration
-    // index of a visible property.
-    PropertyType t = GetDetails(index).type();
-    if (t == CONSTANT_FUNCTION ||
-        t == FIELD ||
-        t == CALLBACKS ||
-        t == INTERCEPTOR) {
-      keep_enumeration_index = true;
-    } else if (remove_transitions) {
-     // Replaced descriptor has been counted as removed if it is
-     // a transition that will be replaced.  Adjust count in this case.
-      ++new_size;
-    }
-  } else {
+  if (!replacing) {
+    ++new_size;
+  } else if (!IsTransitionOnly(index)) {
+    // We are replacing an existing descriptor. We keep the enumeration index
+    // of a visible property.
+    keep_enumeration_index = true;
+  } else if (remove_transitions) {
+    // Replaced descriptor has been counted as removed if it is a transition
+    // that will be replaced. Adjust count in this case.
     ++new_size;
   }
 
@@ -5966,8 +6141,8 @@ MaybeObject* AccessorPair::CopyWithoutTransitions() {
 
 
 Object* AccessorPair::GetComponent(AccessorComponent component) {
-    Object* accessor = (component == ACCESSOR_GETTER) ? getter() : setter();
-    return accessor->IsTheHole() ? GetHeap()->undefined_value() : accessor;
+  Object* accessor = get(component);
+  return accessor->IsTheHole() ? GetHeap()->undefined_value() : accessor;
 }
 
 

src/objects.h

@@ -664,12 +664,13 @@ enum CompareResult {
                          WriteBarrierMode mode = UPDATE_WRITE_BARRIER); \
 
 
+class AccessorPair;
 class DictionaryElementsAccessor;
 class ElementsAccessor;
+class Failure;
 class FixedArrayBase;
 class ObjectVisitor;
 class StringStream;
-class Failure;
 
 struct ValueInfo : public Malloced {
   ValueInfo() : type(FIRST_TYPE), ptr(NULL), str(NULL), number(0) { }
@@ -1608,6 +1609,14 @@ class JSObject: public JSReceiver {
                                               Object* getter,
                                               Object* setter,
                                               PropertyAttributes attributes);
+  // Try to define a single accessor paying attention to map transitions.
+  // Returns a JavaScript null if this was not possible and we have to use the
+  // slow case. Note that we can fail due to allocations, too.
+  MUST_USE_RESULT MaybeObject* DefineFastAccessor(
+      String* name,
+      AccessorComponent component,
+      Object* accessor,
+      PropertyAttributes attributes);
   Object* LookupAccessor(String* name, AccessorComponent component);
 
   MUST_USE_RESULT MaybeObject* DefineAccessor(AccessorInfo* info);
@@ -8100,6 +8109,18 @@ class AccessorPair: public Struct {
 
   MUST_USE_RESULT MaybeObject* CopyWithoutTransitions();
 
+  Object* get(AccessorComponent component) {
+    return component == ACCESSOR_GETTER ? getter() : setter();
+  }
+
+  void set(AccessorComponent component, Object* value) {
+    if (component == ACCESSOR_GETTER) {
+      set_getter(value);
+    } else {
+      set_setter(value);
+    }
+  }
+
   // Note: Returns undefined instead in case of a hole.
   Object* GetComponent(AccessorComponent component);
 

test/cctest/test-mark-compact.cc

@@ -531,18 +531,18 @@ TEST(BootUpMemoryUse) {
   // there we just skip the test.
   if (initial_memory >= 0) {
     InitializeVM();
-    intptr_t booted_memory = MemoryInUse();
+    intptr_t delta = MemoryInUse() - initial_memory;
     if (sizeof(initial_memory) == 8) {
       if (v8::internal::Snapshot::IsEnabled()) {
-        CHECK_LE(booted_memory - initial_memory, 3600 * 1024);  // 3396.
+        CHECK_LE(delta, 3600 * 1024);  // 3396.
       } else {
-        CHECK_LE(booted_memory - initial_memory, 3600 * 1024);  // 3432.
+        CHECK_LE(delta, 4000 * 1024);  // 3948.
       }
     } else {
       if (v8::internal::Snapshot::IsEnabled()) {
-        CHECK_LE(booted_memory - initial_memory, 2800 * 1024);  // 2484.
+        CHECK_LE(delta, 2600 * 1024);  // 2484.
       } else {
-        CHECK_LE(booted_memory - initial_memory, 2950 * 1024);  // 2844
+        CHECK_LE(delta, 2950 * 1024);  // 2844
       }
     }
   }

test/mjsunit/accessor-map-sharing.js

+// Copyright 2012 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.
+
+// Flags: --allow-natives-syntax --fast-accessor-properties
+
+// Handy abbreviations.
+var dp = Object.defineProperty;
+var gop = Object.getOwnPropertyDescriptor;
+
+function getter() { return 111; }
+function setter(x) { print(222); }
+function anotherGetter() { return 333; }
+function anotherSetter(x) { print(444); }
+var obj1, obj2;
+
+// Two objects with the same getter.
+obj1 = {};
+dp(obj1, "alpha", { get: getter });
+obj2 = {};
+dp(obj2, "alpha", { get: getter });
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same getter, oldskool.
+obj1 = {};
+obj1.__defineGetter__("bravo", getter);
+assertEquals(getter, obj1.__lookupGetter__("bravo"));
+obj2 = {};
+obj2.__defineGetter__("bravo", getter);
+assertEquals(getter, obj2.__lookupGetter__("bravo"));
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same setter.
+obj1 = {};
+dp(obj1, "charlie", { set: setter });
+obj2 = {};
+dp(obj2, "charlie", { set: setter });
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same setter, oldskool.
+obj1 = {};
+obj1.__defineSetter__("delta", setter);
+assertEquals(setter, obj1.__lookupSetter__("delta"));
+obj2 = {};
+obj2.__defineSetter__("delta", setter);
+assertEquals(setter, obj2.__lookupSetter__("delta"));
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same getter and setter.
+obj1 = {};
+dp(obj1, "foxtrot", { get: getter, set: setter });
+obj2 = {};
+dp(obj2, "foxtrot", { get: getter, set: setter });
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same getter and setter, set separately.
+obj1 = {};
+dp(obj1, "golf", { get: getter, configurable: true });
+dp(obj1, "golf", { set: setter, configurable: true });
+obj2 = {};
+dp(obj2, "golf", { get: getter, configurable: true });
+dp(obj2, "golf", { set: setter, configurable: true });
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same getter and setter, set separately, oldskool.
+obj1 = {};
+obj1.__defineGetter__("hotel", getter);
+obj1.__defineSetter__("hotel", setter);
+obj2 = {};
+obj2.__defineGetter__("hotel", getter);
+obj2.__defineSetter__("hotel", setter);
+assertTrue(%HaveSameMap(obj1, obj2));
+
+// Attribute-only change, shouldn't affect previous descriptor properties.
+obj1 = {};
+dp(obj1, "india", { get: getter, configurable: true, enumerable: true });
+assertEquals(getter, gop(obj1, "india").get);
+assertTrue(gop(obj1, "india").configurable);
+assertTrue(gop(obj1, "india").enumerable);
+dp(obj1, "india", { enumerable: false });
+assertEquals(getter, gop(obj1, "india").get);
+assertTrue(gop(obj1, "india").configurable);
+assertFalse(gop(obj1, "india").enumerable);
+
+// Attribute-only change, shouldn't affect objects with previously shared maps.
+obj1 = {};
+dp(obj1, "juliet", { set: setter, configurable: true, enumerable: false });
+assertEquals(setter, gop(obj1, "juliet").set);
+assertTrue(gop(obj1, "juliet").configurable);
+assertFalse(gop(obj1, "juliet").enumerable);
+obj2 = {};
+dp(obj2, "juliet", { set: setter, configurable: true, enumerable: false });
+assertEquals(setter, gop(obj2, "juliet").set);
+assertTrue(gop(obj2, "juliet").configurable);
+assertFalse(gop(obj2, "juliet").enumerable);
+dp(obj1, "juliet", { set: setter, configurable: false, enumerable: true });
+assertEquals(setter, gop(obj1, "juliet").set);
+assertFalse(gop(obj1, "juliet").configurable);
+assertTrue(gop(obj1, "juliet").enumerable);
+assertEquals(setter, gop(obj2, "juliet").set);
+assertTrue(gop(obj2, "juliet").configurable);
+assertFalse(gop(obj2, "juliet").enumerable);
+
+// Two objects with the different getters.
+obj1 = {};
+dp(obj1, "kilo", { get: getter });
+obj2 = {};
+dp(obj2, "kilo", { get: anotherGetter });
+assertEquals(getter, gop(obj1, "kilo").get);
+assertEquals(anotherGetter, gop(obj2, "kilo").get);
+assertFalse(%HaveSameMap(obj1, obj2));
+
+// Two objects with the same getters and different setters.
+obj1 = {};
+dp(obj1, "lima", { get: getter, set: setter });
+obj2 = {};
+dp(obj2, "lima", { get: getter, set: anotherSetter });
+assertEquals(setter, gop(obj1, "lima").set);
+assertEquals(anotherSetter, gop(obj2, "lima").set);
+assertFalse(%HaveSameMap(obj1, obj2));
+
+// Even 'undefined' is a kind of getter.
+obj1 = {};
+dp(obj1, "mike", { get: undefined });
+assertTrue("mike" in obj1);
+assertEquals(undefined, gop(obj1, "mike").get);
+assertEquals(undefined, obj1.__lookupGetter__("mike"));
+assertEquals(undefined, gop(obj1, "mike").set);
+assertEquals(undefined, obj1.__lookupSetter__("mike"));
+
+// Even 'undefined' is a kind of setter.
+obj1 = {};
+dp(obj1, "november", { set: undefined });
+assertTrue("november" in obj1);
+assertEquals(undefined, gop(obj1, "november").get);
+assertEquals(undefined, obj1.__lookupGetter__("november"));
+assertEquals(undefined, gop(obj1, "november").set);
+assertEquals(undefined, obj1.__lookupSetter__("november"));
+
+// Redefining a data property.
+obj1 = {};
+obj1.oscar = 12345;
+dp(obj1, "oscar", { set: setter });
+assertEquals(setter, gop(obj1, "oscar").set);
+
+// Re-adding the same getter/attributes pair.
+obj1 = {};
+dp(obj1, "papa", { get: getter, configurable: true });
+dp(obj1, "papa", { get: getter, set: setter, configurable: true });
+assertEquals(getter, gop(obj1, "papa").get);
+assertEquals(setter, gop(obj1, "papa").set);
+assertTrue(gop(obj1, "papa").configurable);
+assertFalse(gop(obj1, "papa").enumerable);

test/mjsunit/regress/regress-128146.js

@@ -25,14 +25,9 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+// Define accessor properties, resulting in an AccessorPair with 2 transitions.
 Object.defineProperty({},"foo",{set:function(){},configurable:false});
 Object.defineProperty({},"foo",{get:function(){},configurable:false});
-Object.defineProperty({},"foo",{});
-
-// From WebKit layout tests (fast/js/prototypes.html)
-var wasSet = false;
-var o = { };
-o.__defineGetter__("__proto__", function() { wasSet = true });
-o.__proto__;
-assertFalse(wasSet);
 
+// Define a data property under the same name.
+Object.defineProperty({},"foo",{});