Use map transitions when defining accessor properties.

AccessorPairs can now contain map transitions, which is similar to our current
handling of CONSTANT_FUNCTION/CONSTANT_TRANSITION, but generalized to a pair for
holding info about the getter and the setter. This way we can achieve map
sharing for objects with accessor properties, which is a prerequisite for making
them fast via inlining. We fall back to the previous way of handling accessor
properties when sharing is not possible or we don't handle a special case.

Note: When an exisiting accessor property is redefined we could in principle
move the AccessorPair out of the descriptor into the object itself (again just
like the way we do something similar for CONSTANT_FUNCTION/CONSTANT_TRANSITION),
but this would require a new property kind for holding a pair of values. Perhaps
we can implement this later, but for now this hopefully rare case is handled
like before, losing map sharing and potentially creating more maps than strictly
necessary.

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

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

src/objects.cc

@@ -4413,7 +4413,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();
@@ -4427,6 +4432,28 @@ 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 (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;
@@ -4576,6 +4603,157 @@ 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);
+  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);
+  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.GetValue();
+  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());
@@ -5939,7 +6117,7 @@ MaybeObject* AccessorPair::CopyWithoutTransitions() {
 
 
 Object* AccessorPair::GetComponent(AccessorComponent component) {
-    Object* accessor = (component == ACCESSOR_GETTER) ? getter() : setter();
+  Object* accessor = get(component);
   return accessor->IsTheHole() ? GetHeap()->undefined_value() : accessor;
 }
 

src/objects.h

@@ -704,12 +704,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) { }
@@ -1642,6 +1643,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);
@@ -8108,6 +8117,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/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
+
+// 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);