Cleaned up setting of accessors.

src/objects.cc

 // 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
@@ skipping 4344 matching lines @@
   for (Object* current = this;
        current != heap->null_value() && current->IsJSObject();
        current = JSObject::cast(current)->GetPrototype()) {
     JSObject::cast(current)->LocalLookupRealNamedProperty(name, result);
     if (result->IsFound() && result->type() == CALLBACKS) return;
   }
   result->NotFound();
 }
 
 
-// Search for a getter or setter in an elements dictionary and update its
-// attributes.  Returns either undefined if the element is non-deletable, or the
-// getter/setter pair if there is an existing one, or the hole value if the
-// element does not exist or is a normal non-getter/setter data element.
-static Object* UpdateGetterSetterInDictionary(
+// Try to update an accessor in an elements dictionary. Return true if the
+// update succeeded, and false otherwise.
+static bool UpdateGetterSetterInDictionary(
     SeededNumberDictionary* dictionary,
     uint32_t index,
-    PropertyAttributes attributes,
-    Heap* heap) {
+    bool is_getter,
+    Object* fun,
+    PropertyAttributes attributes) {
   int entry = dictionary->FindEntry(index);
   if (entry != SeededNumberDictionary::kNotFound) {
     Object* result = dictionary->ValueAt(entry);
     PropertyDetails details = dictionary->DetailsAt(entry);
     // TODO(mstarzinger): We should check for details.IsDontDelete() here once
     // we only call into the runtime once to set both getter and setter.
     if (details.type() == CALLBACKS && result->IsAccessorPair()) {
       if (details.attributes() != attributes) {
         dictionary->DetailsAtPut(entry,
                                  PropertyDetails(attributes, CALLBACKS, index));
       }
-      return result;
+      AccessorPair::cast(result)->set(is_getter, fun);
+      return true;
     }
   }
-  return heap->the_hole_value();
+  return false;
 }
 
 
-MaybeObject* JSObject::DefineGetterSetter(String* name,
-                                          PropertyAttributes attributes) {
-  Heap* heap = GetHeap();
-  // Make sure that the top context does not change when doing callbacks or
-  // interceptor calls.
-  AssertNoContextChange ncc;
-
-  // Try to flatten before operating on the string.
-  name->TryFlatten();
-
-  if (!CanSetCallback(name)) {
-    return heap->undefined_value();
+MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
+                                             bool is_getter,
+                                             Object* fun,
+                                             PropertyAttributes attributes) {
+  switch (GetElementsKind()) {
+    case FAST_SMI_ONLY_ELEMENTS:
+    case FAST_ELEMENTS:
+    case FAST_DOUBLE_ELEMENTS:
+      break;
+    case EXTERNAL_PIXEL_ELEMENTS:
+    case EXTERNAL_BYTE_ELEMENTS:
+    case EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
+    case EXTERNAL_SHORT_ELEMENTS:
+    case EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
+    case EXTERNAL_INT_ELEMENTS:
+    case EXTERNAL_UNSIGNED_INT_ELEMENTS:
+    case EXTERNAL_FLOAT_ELEMENTS:
+    case EXTERNAL_DOUBLE_ELEMENTS:
+      // Ignore getters and setters on pixel and external array elements.
+      return GetHeap()->undefined_value();
+    case DICTIONARY_ELEMENTS:
+      if (UpdateGetterSetterInDictionary(element_dictionary(),
+                                         index,
+                                         is_getter,
+                                         fun,
+                                         attributes)) {
+        return GetHeap()->undefined_value();

[Michael Starzinger, 2012/02/22 10:24:38]

I prefer having the result of GetHeap() in a local "heap" variable. It might not
matter much performance-wise and my opinion about that is not very strong. So I
leave the decision up to you.

[Sven Panne, 2012/02/22 10:52:18]

I don't think that one possible additional memory access will really hurt us
here in these expensive runtime functions, so I'd like to leave it as it is,
preferring conciseness over a nano-optimization. A world premiere in v8 code!
;-)

[fschneider, 2012/02/24 10:33:17]

I don't buy the conciseness argument. heap->undefined_value() results in shorter
lines.

[Sven Panne, 2012/02/24 11:47:17]

It's not about shorter lines, it's all about having a smaller "mental context"
one has to carry around in one's brain when  trying to understand code, and
every local variable adds to this mental context. In the end it's all a
judgement call...

[Michael Starzinger, 2012/02/24 12:44:09]

Knowing that "heap" refers to the one and only heap object in the current
isolate is constant in my brain. So no additional pressure on my mental context
in this case. Having to read "GetHeap()" makes me start to wonder: "Hang on, it
needs the map to get the heap, is the map in a consistent state? How much does
it cost to take that indirection?", that's much more pressure on my mental
context.

Just my two cents.

+      }
+      break;
+    case NON_STRICT_ARGUMENTS_ELEMENTS: {
+      // Ascertain whether we have read-only properties or an existing
+      // getter/setter pair in an arguments elements dictionary backing
+      // store.
+      FixedArray* parameter_map = FixedArray::cast(elements());
+      uint32_t length = parameter_map->length();
+      Object* probe =
+          index < (length - 2) ? parameter_map->get(index + 2) : NULL;
+      if (probe == NULL || probe->IsTheHole()) {
+        FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
+        if (arguments->IsDictionary()) {
+          SeededNumberDictionary* dictionary =
+              SeededNumberDictionary::cast(arguments);
+          if (UpdateGetterSetterInDictionary(dictionary,
+                                             index,
+                                             is_getter,
+                                             fun,
+                                             attributes)) {
+            return GetHeap()->undefined_value();
+          }
+        }
+      }
+      break;
+    }
   }
 
-  uint32_t index = 0;
-  bool is_element = name->AsArrayIndex(&index);
+  AccessorPair* accessors;
+  { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
+    if (!maybe_accessors->To(&accessors)) return maybe_accessors;
+  }
+  accessors->set(is_getter, fun);
 
-  if (is_element) {
-    switch (GetElementsKind()) {
-      case FAST_SMI_ONLY_ELEMENTS:
-      case FAST_ELEMENTS:
-      case FAST_DOUBLE_ELEMENTS:
-        break;
-      case EXTERNAL_PIXEL_ELEMENTS:
-      case EXTERNAL_BYTE_ELEMENTS:
-      case EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
-      case EXTERNAL_SHORT_ELEMENTS:
-      case EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
-      case EXTERNAL_INT_ELEMENTS:
-      case EXTERNAL_UNSIGNED_INT_ELEMENTS:
-      case EXTERNAL_FLOAT_ELEMENTS:
-      case EXTERNAL_DOUBLE_ELEMENTS:
-        // Ignore getters and setters on pixel and external array
-        // elements.
-        return heap->undefined_value();
-      case DICTIONARY_ELEMENTS: {
-        Object* probe = UpdateGetterSetterInDictionary(element_dictionary(),
-                                                       index,
-                                                       attributes,
-                                                       heap);
-        if (!probe->IsTheHole()) return probe;
-        // Otherwise allow to override it.
-        break;
-      }
-      case NON_STRICT_ARGUMENTS_ELEMENTS: {
-        // Ascertain whether we have read-only properties or an existing
-        // getter/setter pair in an arguments elements dictionary backing
-        // store.
-        FixedArray* parameter_map = FixedArray::cast(elements());
-        uint32_t length = parameter_map->length();
-        Object* probe =
-            index < (length - 2) ? parameter_map->get(index + 2) : NULL;
-        if (probe == NULL || probe->IsTheHole()) {
-          FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
-          if (arguments->IsDictionary()) {
-            SeededNumberDictionary* dictionary =
-                SeededNumberDictionary::cast(arguments);
-            probe = UpdateGetterSetterInDictionary(dictionary,
-                                                   index,
-                                                   attributes,
-                                                   heap);
-            if (!probe->IsTheHole()) return probe;
-          }
+  { MaybeObject* maybe_ok = SetElementCallback(index, accessors, attributes);

[Sven Panne, 2012/02/24 11:47:17]

I'll apply the same simplification here...

+    if (maybe_ok->IsFailure()) return maybe_ok;
+  }
+  return GetHeap()->undefined_value();
+}
+
+
+MaybeObject* JSObject::DefinePropertyAccessor(String* name,
+                                              bool is_getter,
+                                              Object* fun,
+                                              PropertyAttributes attributes) {
+  // Lookup the name.
+  LookupResult result(GetHeap()->isolate());
+  LocalLookupRealNamedProperty(name, &result);
+  if (result.IsFound()) {
+    // TODO(mstarzinger): We should check for result.IsDontDelete() here once
+    // we only call into the runtime once to set both getter and setter.
+    if (result.type() == CALLBACKS) {
+      Object* obj = result.GetCallbackObject();
+      // Need to preserve old getters/setters.
+      if (obj->IsAccessorPair()) {
+        AccessorPair::cast(obj)->set(is_getter, fun);
+        // Use set to update attributes.
+        { MaybeObject* maybe_ok = SetPropertyCallback(name, obj, attributes);
+          if (maybe_ok->IsFailure()) return maybe_ok;
         }
-        break;
-      }
-    }
-  } else {
-    // Lookup the name.
-    LookupResult result(heap->isolate());
-    LocalLookupRealNamedProperty(name, &result);
-    if (result.IsFound()) {
-      // TODO(mstarzinger): We should check for result.IsDontDelete() here once
-      // we only call into the runtime once to set both getter and setter.
-      if (result.type() == CALLBACKS) {
-        Object* obj = result.GetCallbackObject();
-        // Need to preserve old getters/setters.
-        if (obj->IsAccessorPair()) {
-          // Use set to update attributes.
-          return SetPropertyCallback(name, obj, attributes);
-        }
+        return GetHeap()->undefined_value();

[fschneider, 2012/02/24 10:33:17]

return SetPropertyCallback(name, obj, attributes);

would be more concise.

[Sven Panne, 2012/02/24 11:47:17]

See remark below.

       }
     }
   }
 
   AccessorPair* accessors;
-  { MaybeObject* maybe_accessors = heap->AllocateAccessorPair();
-    if (!maybe_accessors->To<AccessorPair>(&accessors)) return maybe_accessors;
+  { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
+    if (!maybe_accessors->To(&accessors)) return maybe_accessors;
   }
+  accessors->set(is_getter, fun);
 
-  if (is_element) {
-    return SetElementCallback(index, accessors, attributes);
-  } else {
-    return SetPropertyCallback(name, accessors, attributes);
+  { MaybeObject* maybe_ok = SetPropertyCallback(name, accessors, attributes);
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
+  return GetHeap()->undefined_value();

[fschneider, 2012/02/24 10:33:17]

return SetPropertyCallback(name, accessors, attributes);

would be more concise.

[Sven Panne, 2012/02/24 11:47:17]

Good point, I'll submit a separate CL for this mini-cleanup, although I think
that I'll have to change this code soon, anyway (to make setting of callbacks
more "atomic", in a way like Michael already has done it for data properties).

 }
 
 
 bool JSObject::CanSetCallback(String* name) {
   ASSERT(!IsAccessCheckNeeded() ||
          GetIsolate()->MayNamedAccess(this, name, v8::ACCESS_SET));
 
   // Check if there is an API defined callback object which prohibits
   // callback overwriting in this object or it's prototype chain.
   // This mechanism is needed for instance in a browser setting, where
@@ skipping 13 matching lines @@
   return true;
 }
 
 
 MaybeObject* JSObject::SetElementCallback(uint32_t index,
                                           Object* structure,
                                           PropertyAttributes attributes) {
   PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
 
   // Normalize elements to make this operation simple.
-  SeededNumberDictionary* dictionary = NULL;
-  { Object* result;
-    MaybeObject* maybe = NormalizeElements();
-    if (!maybe->ToObject(&result)) return maybe;
-    dictionary = SeededNumberDictionary::cast(result);
+  SeededNumberDictionary* dictionary;
+  { MaybeObject* maybe_dictionary = NormalizeElements();
+    if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
   }
   ASSERT(HasDictionaryElements() || HasDictionaryArgumentsElements());
 
   // Update the dictionary with the new CALLBACKS property.
-  { Object* result;
-    MaybeObject* maybe = dictionary->Set(index, structure, details);
-    if (!maybe->ToObject(&result)) return maybe;
-    dictionary = SeededNumberDictionary::cast(result);
+  { MaybeObject* maybe_dictionary = dictionary->Set(index, structure, details);
+    if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
   }
 
   dictionary->set_requires_slow_elements();
   // Update the dictionary backing store on the object.
   if (elements()->map() == GetHeap()->non_strict_arguments_elements_map()) {
     // Also delete any parameter alias.
     //
     // TODO(kmillikin): when deleting the last parameter alias we could
     // switch to a direct backing store without the parameter map.  This
     // would allow GC of the context.
     FixedArray* parameter_map = FixedArray::cast(elements());
-    uint32_t length = parameter_map->length();
-    if (index < length - 2) {
+    if (index < static_cast<uint32_t>(parameter_map->length()) - 2) {
       parameter_map->set(index + 2, GetHeap()->the_hole_value());
     }
     parameter_map->set(1, dictionary);
   } else {
     set_elements(dictionary);
   }
 
-  return structure;
+  return GetHeap()->undefined_value();
 }
 
 
 MaybeObject* JSObject::SetPropertyCallback(String* name,
                                            Object* structure,
                                            PropertyAttributes attributes) {
   PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
 
   bool convert_back_to_fast = HasFastProperties() &&
       (map()->instance_descriptors()->number_of_descriptors()
           < DescriptorArray::kMaxNumberOfDescriptors);
 
   // Normalize object to make this operation simple.
-  Object* ok;
   { MaybeObject* maybe_ok = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
-    if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
 
   // For the global object allocate a new map to invalidate the global inline
   // caches which have a global property cell reference directly in the code.
   if (IsGlobalObject()) {
-    Object* new_map;
+    Map* new_map;
     { MaybeObject* maybe_new_map = map()->CopyDropDescriptors();
-      if (!maybe_new_map->ToObject(&new_map)) return maybe_new_map;
+      if (!maybe_new_map->To(&new_map)) return maybe_new_map;
     }
-    set_map(Map::cast(new_map));
+    set_map(new_map);
     // When running crankshaft, changing the map is not enough. We
     // need to deoptimize all functions that rely on this global
     // object.
     Deoptimizer::DeoptimizeGlobalObject(this);
   }
 
   // Update the dictionary with the new CALLBACKS property.
-  Object* result;
-  { MaybeObject* maybe_result = SetNormalizedProperty(name, structure, details);
-    if (!maybe_result->ToObject(&result)) return maybe_result;
+  { MaybeObject* maybe_ok = SetNormalizedProperty(name, structure, details);
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
 
   if (convert_back_to_fast) {
-    { MaybeObject* maybe_ok = TransformToFastProperties(0);
-      if (!maybe_ok->ToObject(&ok)) return maybe_ok;
-    }
+    MaybeObject* maybe_ok = TransformToFastProperties(0);
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
-  return result;
+  return GetHeap()->undefined_value();
 }
 
 MaybeObject* JSObject::DefineAccessor(String* name,
                                       bool is_getter,
                                       Object* fun,
                                       PropertyAttributes attributes) {
   ASSERT(fun->IsSpecFunction() || fun->IsUndefined());
   Isolate* isolate = GetIsolate();
   // Check access rights if needed.
   if (IsAccessCheckNeeded() &&
       !isolate->MayNamedAccess(this, name, v8::ACCESS_SET)) {
     isolate->ReportFailedAccessCheck(this, v8::ACCESS_SET);
     return isolate->heap()->undefined_value();
   }
 
   if (IsJSGlobalProxy()) {
     Object* proto = GetPrototype();
     if (proto->IsNull()) return this;
     ASSERT(proto->IsJSGlobalObject());
     return JSObject::cast(proto)->DefineAccessor(name, is_getter,
                                                  fun, attributes);
   }
 
-  Object* accessors;
-  { MaybeObject* maybe_accessors = DefineGetterSetter(name, attributes);
-    if (!maybe_accessors->To<Object>(&accessors)) return maybe_accessors;
-  }
-  if (accessors->IsUndefined()) return accessors;
-  if (is_getter) {
-    AccessorPair::cast(accessors)->set_getter(fun);
-  } else {
-    AccessorPair::cast(accessors)->set_setter(fun);
-  }
-  return this;
+  // Make sure that the top context does not change when doing callbacks or
+  // interceptor calls.
+  AssertNoContextChange ncc;
+
+  // Try to flatten before operating on the string.
+  name->TryFlatten();
+
+  if (!CanSetCallback(name)) return isolate->heap()->undefined_value();
+
+  uint32_t index = 0;
+  return name->AsArrayIndex(&index) ?
+      DefineElementAccessor(index, is_getter, fun, attributes) :
+      DefinePropertyAccessor(name, is_getter, fun, attributes);
 }
 
 
 MaybeObject* JSObject::DefineAccessor(AccessorInfo* info) {
   Isolate* isolate = GetIsolate();
   String* name = String::cast(info->name());
   // Check access rights if needed.
   if (IsAccessCheckNeeded() &&
       !isolate->MayNamedAccess(this, name, v8::ACCESS_SET)) {
     isolate->ReportFailedAccessCheck(this, v8::ACCESS_SET);
@@ skipping 42 matching lines @@
         // Ignore getters and setters on pixel and external array
         // elements.
         return isolate->heap()->undefined_value();
       case DICTIONARY_ELEMENTS:
         break;
       case NON_STRICT_ARGUMENTS_ELEMENTS:
         UNIMPLEMENTED();
         break;
     }
 
-    Object* ok;
     { MaybeObject* maybe_ok =
           SetElementCallback(index, info, info->property_attributes());
-      if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+      if (maybe_ok->IsFailure()) return maybe_ok;
     }
   } else {
     // Lookup the name.
     LookupResult result(isolate);
     LocalLookup(name, &result);
     // ES5 forbids turning a property into an accessor if it's not
     // configurable (that is IsDontDelete in ES3 and v8), see 8.6.1 (Table 5).
     if (result.IsProperty() && (result.IsReadOnly() || result.IsDontDelete())) {
       return isolate->heap()->undefined_value();
     }
-    Object* ok;
     { MaybeObject* maybe_ok =
           SetPropertyCallback(name, info, info->property_attributes());
-      if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+      if (maybe_ok->IsFailure()) return maybe_ok;
     }
   }
 
   return this;
 }
 
 
 Object* JSObject::LookupAccessor(String* name, bool is_getter) {
   Heap* heap = GetHeap();
 
@@ skipping 7943 matching lines @@
         } else {
           int offset = current_offset - inobject_props;
           FixedArray::cast(fields)->set(offset, value);
         }
         FieldDescriptor d(String::cast(key),
                           current_offset++,
                           details.attributes(),
                           details.index());
         descriptors->Set(next_descriptor++, &d, witness);
       } else if (type == CALLBACKS) {
+        if (value->IsAccessorPair()) {
+          MaybeObject* maybe_copy =
+              AccessorPair::cast(value)->CopyWithoutTransitions();
+          if (!maybe_copy->To(&value)) return maybe_copy;
+        }
         CallbacksDescriptor d(String::cast(key),
                               value,
                               details.attributes(),
                               details.index());
         descriptors->Set(next_descriptor++, &d, witness);
       } else {
         UNREACHABLE();
       }
     }
   }
@@ skipping 390 matching lines @@
   if (break_point_objects()->IsUndefined()) return 0;
   // Single break point.
   if (!break_point_objects()->IsFixedArray()) return 1;
   // Multiple break points.
   return FixedArray::cast(break_point_objects())->length();
 }
 #endif  // ENABLE_DEBUGGER_SUPPORT
 
 
 } }  // namespace v8::internal