Restructure JSObject::SetElement for performance.

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 10249 matching lines @@
 }
 
 
 MaybeObject* JSObject::SetElement(uint32_t index,
                                   Object* value_raw,
                                   PropertyAttributes attributes,
                                   StrictModeFlag strict_mode,
                                   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,
                                              set_mode);
   }
 
   // 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);
   }
 
   // Normalize the elements to enable attributes on the property.
   if ((attributes & (DONT_DELETE | DONT_ENUM | READ_ONLY)) != 0) {
     SeededNumberDictionary* dictionary;
     MaybeObject* maybe_object = NormalizeElements();
     if (!maybe_object->To(&dictionary)) return maybe_object;
     // Make sure that we never go back to fast case.
     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
   MaybeObject* result = self->HasIndexedInterceptor()
     ? self->SetElementWithInterceptor(
         index, *value, attributes, strict_mode, check_prototype, set_mode)
     : self->SetElementWithoutInterceptor(
         index, *value, attributes, strict_mode, check_prototype, set_mode);
 
   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;
 }
 
 
 MaybeObject* JSObject::SetElementWithoutInterceptor(uint32_t index,
                                                     Object* value,
                                                     PropertyAttributes attr,
                                                     StrictModeFlag strict_mode,
@@ skipping 3494 matching lines @@
   set_year(Smi::FromInt(year), SKIP_WRITE_BARRIER);
   set_month(Smi::FromInt(month), SKIP_WRITE_BARRIER);
   set_day(Smi::FromInt(day), SKIP_WRITE_BARRIER);
   set_weekday(Smi::FromInt(weekday), SKIP_WRITE_BARRIER);
   set_hour(Smi::FromInt(hour), SKIP_WRITE_BARRIER);
   set_min(Smi::FromInt(min), SKIP_WRITE_BARRIER);
   set_sec(Smi::FromInt(sec), SKIP_WRITE_BARRIER);
 }
 
 } }  // namespace v8::internal