Restructure JSObject::SetElement for performance.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index f8d080d676399d0eea6aebff3fc8ef0f0744d8ae..a03054653aa16cf014d8ffd83783a7ccd4d7c223 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -10267,25 +10267,21 @@ MaybeObject* JSObject::SetElement(uint32_t index,
                                   bool check_prototype,
                                   SetPropertyMode set_mode) {
   Isolate* isolate = GetIsolate();
-  HandleScope scope(isolate);
-  Handle<JSObject> self(this);
-  Handle<Object> value(value_raw);
 
   // Check access rights if needed.
   if (IsAccessCheckNeeded()) {
-    Heap* heap = GetHeap();
-    if (!heap->isolate()->MayIndexedAccess(*self, index, v8::ACCESS_SET)) {
-      heap->isolate()->ReportFailedAccessCheck(*self, v8::ACCESS_SET);
-      return *value;
+    if (!isolate->MayIndexedAccess(this, index, v8::ACCESS_SET)) {
+      isolate->ReportFailedAccessCheck(this, v8::ACCESS_SET);
+      return value_raw;
     }
   }
 
   if (IsJSGlobalProxy()) {
     Object* proto = GetPrototype();
-    if (proto->IsNull()) return *value;
+    if (proto->IsNull()) return value_raw;
     ASSERT(proto->IsJSGlobalObject());
     return JSObject::cast(proto)->SetElement(index,
-                                             *value,
+                                             value_raw,
                                              attributes,
                                              strict_mode,
                                              check_prototype,
@@ -10295,7 +10291,7 @@ MaybeObject* JSObject::SetElement(uint32_t index,
   // Don't allow element properties to be redefined for external arrays.
   if (HasExternalArrayElements() && set_mode == DEFINE_PROPERTY) {
     Handle<Object> number = isolate->factory()->NewNumberFromUint(index);
-    Handle<Object> args[] = { self, number };
+    Handle<Object> args[] = { handle(this), number };
     Handle<Object> error = isolate->factory()->NewTypeError(
         "redef_external_array_element", HandleVector(args, ARRAY_SIZE(args)));
     return isolate->Throw(*error);
@@ -10310,23 +10306,27 @@ MaybeObject* JSObject::SetElement(uint32_t index,
     dictionary->set_requires_slow_elements();
   }
 
+  if (!(FLAG_harmony_observation && map()->is_observed())) {
+    return HasIndexedInterceptor()

[rafaelw, 2012/11/13 13:18:44]

consider factoring out into inline static?

[Toon Verwaest, 2012/11/13 13:38:08]

+1
On 2012/11/13 13:18:44, rafaelw wrote:

[rossberg, 2012/11/13 13:47:44]

Note that one version is handlified while the other is not, which is a central
point of the whole exercise.

[rafaelw, 2012/11/13 14:53:01]

I'm not understanding why it wouldn't work, but I defer to your judgement here.

On 2012/11/13 13:47:44, rossberg wrote:

[rossberg, 2012/11/13 15:33:13]

Thing is, I could only factor it out in form of a handlified version, but then I
likely lose part of the performance I just tried to regain on the "normal" path.

+      ? SetElementWithInterceptor(
+          index, value_raw, attributes, strict_mode, check_prototype, set_mode)
+      : SetElementWithoutInterceptor(
+          index, value_raw, attributes, strict_mode, check_prototype, set_mode);
+  }
+
   // From here on, everything has to be handlified.
-  Handle<String> name;
-  Handle<Object> old_value(isolate->heap()->the_hole_value());
-  Handle<Object> old_array_length;
-  PropertyAttributes old_attributes = ABSENT;
-  bool preexists = false;
-  if (FLAG_harmony_observation && map()->is_observed()) {
-    name = isolate->factory()->Uint32ToString(index);
-    preexists = self->HasLocalElement(index);
-    if (preexists) {
-      old_attributes = self->GetLocalPropertyAttribute(*name);
-      // TODO(observe): only read & set old_value if we have a data property
-      old_value = Object::GetElement(self, index);
-    } else if (self->IsJSArray()) {
-      // Store old array length in case adding an element grows the array.
-      old_array_length = handle(Handle<JSArray>::cast(self)->length());
-    }
+  Handle<JSObject> self(this);
+  Handle<Object> value(value_raw);
+  PropertyAttributes old_attributes = self->GetLocalElementAttribute(index);
+  Handle<Object> old_value = isolate->factory()->the_hole_value();
+  Handle<Object> old_length;
+
+  if (old_attributes != ABSENT) {
+    // TODO(observe): only read & set old_value if we have a data property
+    old_value = Object::GetElement(self, index);
+  } else if (self->IsJSArray()) {
+    // Store old array length in case adding an element grows the array.
+    old_length = handle(Handle<JSArray>::cast(self)->length());
   }
 
   // Check for lookup interceptor
@@ -10339,23 +10339,19 @@ MaybeObject* JSObject::SetElement(uint32_t index,
   Handle<Object> hresult;
   if (!result->ToHandle(&hresult)) return result;
 
-  if (FLAG_harmony_observation && map()->is_observed()) {
-    PropertyAttributes new_attributes = self->GetLocalPropertyAttribute(*name);
-    if (!preexists) {
-      EnqueueChangeRecord(self, "new", name, old_value);
-      if (self->IsJSArray() &&
-          !old_array_length->SameValue(Handle<JSArray>::cast(self)->length())) {
-        EnqueueChangeRecord(self, "updated",
-                            isolate->factory()->length_symbol(),
-                            old_array_length);
-      }
-    } else if (new_attributes != old_attributes || old_value->IsTheHole()) {
-      EnqueueChangeRecord(self, "reconfigured", name, old_value);
-    } else {
-      Handle<Object> new_value = Object::GetElement(self, index);
-      if (!new_value->SameValue(*old_value))
-        EnqueueChangeRecord(self, "updated", name, old_value);
-    }
+  Handle<String> name = isolate->factory()->Uint32ToString(index);
+  PropertyAttributes new_attributes = self->GetLocalElementAttribute(index);
+  if (old_attributes == ABSENT) {

[rafaelw, 2012/11/13 13:18:44]

nit: return early here. e.g.

if (old_attributes != ABSENT)
  return *hresult;

EnqueueChangeRecord...

[Toon Verwaest, 2012/11/13 13:38:08]

This doesn't seem possible, given that the value is possibly updated, or the
property was reconfigured.

[rafaelw, 2012/11/13 13:41:25]

You are correct. Sorry. Misread the code.

On 2012/11/13 13:38:08, Toon Verwaest wrote:

+    EnqueueChangeRecord(self, "new", name, old_value);
+    if (self->IsJSArray() &&
+        !old_length->SameValue(Handle<JSArray>::cast(self)->length())) {
+      EnqueueChangeRecord(
+          self, "updated", isolate->factory()->length_symbol(), old_length);
+    }
+  } else if (old_attributes != new_attributes || old_value->IsTheHole()) {
+    EnqueueChangeRecord(self, "reconfigured", name, old_value);
+  } else if (!old_value->SameValue(*Object::GetElement(self, index))) {
+    EnqueueChangeRecord(self, "updated", name, old_value);
   }
 
   return *hresult;