Array.observe emit splices for array length change and update index >= length

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 99fd8e21f0a6dfd0ba2019289caf06c770f241a0..4919a9dd7bad511a3202c5a4809a843a56f72518 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -10899,18 +10899,70 @@ static bool GetOldValue(Isolate* isolate,
                         Handle<JSObject> object,
                         uint32_t index,
                         List<Handle<Object> >* old_values,
-                        List<Handle<String> >* indices) {
+                        List<uint32_t>* indices) {
   PropertyAttributes attributes = object->GetLocalElementAttribute(index);
   ASSERT(attributes != ABSENT);
   if (attributes == DONT_DELETE) return false;
   old_values->Add(object->GetLocalElementAccessorPair(index) == NULL
       ? Object::GetElement(object, index)
       : Handle<Object>::cast(isolate->factory()->the_hole_value()));
-  indices->Add(isolate->factory()->Uint32ToString(index));
+  indices->Add(index);
   return true;
 }
 
 
+// TODO(rafaelw): Remove |delete_count| argument and rely on the length of
+// of |deleted|.
+static void EnqueueSpliceRecord(Handle<JSArray> object,
+                                uint32_t index,
+                                Handle<JSArray> deleted,
+                                uint32_t delete_count,
+                                uint32_t add_count) {
+  Isolate* isolate = object->GetIsolate();
+  HandleScope scope(isolate);
+  Handle<Object> index_object = isolate->factory()->NewNumberFromUint(index);
+  Handle<Object> delete_count_object =
+      isolate->factory()->NewNumberFromUint(delete_count);
+  Handle<Object> add_count_object =
+      isolate->factory()->NewNumberFromUint(add_count);
+
+  Handle<Object> args[] =
+      { object, index_object, deleted, delete_count_object, add_count_object };
+
+  bool threw;
+  Execution::Call(Handle<JSFunction>(isolate->observers_enqueue_splice()),
+                  isolate->factory()->undefined_value(), ARRAY_SIZE(args), args,
+                  &threw);
+  ASSERT(!threw);
+}
+
+
+static void BeginPerformSplice(Handle<JSArray> object) {
+  Isolate* isolate = object->GetIsolate();
+  HandleScope scope(isolate);
+  Handle<Object> args[] = { object };
+
+  bool threw;
+  Execution::Call(Handle<JSFunction>(isolate->observers_begin_perform_splice()),
+                  isolate->factory()->undefined_value(), ARRAY_SIZE(args), args,
+                  &threw);
+  ASSERT(!threw);
+}
+
+
+static void EndPerformSplice(Handle<JSArray> object) {
+  Isolate* isolate = object->GetIsolate();
+  HandleScope scope(isolate);
+  Handle<Object> args[] = { object };
+
+  bool threw;
+  Execution::Call(Handle<JSFunction>(isolate->observers_end_perform_splice()),
+                  isolate->factory()->undefined_value(), ARRAY_SIZE(args), args,
+                  &threw);
+  ASSERT(!threw);
+}
+
+
 MaybeObject* JSArray::SetElementsLength(Object* len) {
   // We should never end in here with a pixel or external array.
   ASSERT(AllowsSetElementsLength());
@@ -10920,7 +10972,7 @@ MaybeObject* JSArray::SetElementsLength(Object* len) {
   Isolate* isolate = GetIsolate();
   HandleScope scope(isolate);
   Handle<JSArray> self(this);
-  List<Handle<String> > indices;
+  List<uint32_t> indices;
   List<Handle<Object> > old_values;
   Handle<Object> old_length_handle(self->length(), isolate);
   Handle<Object> new_length_handle(len, isolate);
@@ -10960,15 +11012,35 @@ MaybeObject* JSArray::SetElementsLength(Object* len) {
   if (!result->ToHandle(&hresult, isolate)) return result;
 
   CHECK(self->length()->ToArrayIndex(&new_length));
-  if (old_length != new_length) {
-    for (int i = 0; i < indices.length(); ++i) {
-      JSObject::EnqueueChangeRecord(
-          self, "deleted", indices[i], old_values[i]);
-    }
+  if (old_length == new_length) return *hresult;
+
+  BeginPerformSplice(self);
+
+  for (int i = 0; i < indices.length(); ++i) {
     JSObject::EnqueueChangeRecord(
-        self, "updated", isolate->factory()->length_string(),
-        old_length_handle);
+        self, "deleted", isolate->factory()->Uint32ToString(indices[i]),
+        old_values[i]);
   }
+  JSObject::EnqueueChangeRecord(
+      self, "updated", isolate->factory()->length_string(),
+      old_length_handle);
+
+  EndPerformSplice(self);
+
+  uint32_t index = Min(old_length, new_length);
+  uint32_t add_count = new_length > old_length ? new_length - old_length : 0;

[rossberg, 2013/05/27 14:48:41]

Max(new_length - old_length, 0)

[rafaelw, 2013/05/28 11:38:53]

new_length and old_length are both uint so, without casting, (new_length -
old_length) is never going to be less than 0. I could cast to ints, but I'm not
sure that buys much since Max is just a macro.

Leaving for now. Please clarify if you want something else.

On 2013/05/27 14:48:41, rossberg wrote:

[rossberg, 2013/06/04 10:56:58]

OK, I see.

+  uint32_t delete_count = new_length < old_length ? old_length - new_length : 0;

[rossberg, 2013/05/27 14:48:41]

Max(old_length - new_length, 0)

[rafaelw, 2013/05/28 11:38:53]

Same here.

On 2013/05/27 14:48:41, rossberg wrote:

+  Handle<JSArray> deleted = isolate->factory()->NewJSArray(0);
+  if (delete_count) {

[rossberg, 2013/05/27 14:48:41]

Nit: delete_count > 0

[rafaelw, 2013/05/28 11:38:53]

Done.

+    int i = indices.length();
+    while (i--) {

[rossberg, 2013/05/27 14:48:41]

Can we make this a for loop?

[rafaelw, 2013/05/28 11:38:53]

I'm curious why you prefer the for loop, but I've changed it for you.

On 2013/05/27 14:48:41, rossberg wrote:

[rossberg, 2013/06/04 10:56:58]

I'm curious why you're curious. ;) It is just an indexed loop from i-1 to 0,
isn't it? Why would it be preferable not to express this as a for loop?

+      JSObject::SetElement(deleted, indices[i] - index, old_values[i], NONE,
+                           kNonStrictMode);
+    }
+  }
+
+  EnqueueSpliceRecord(self, index, deleted, delete_count, add_count);
+
   return *hresult;
 }
 
@@ -11993,14 +12065,18 @@ MaybeObject* JSObject::SetElement(uint32_t index,
   Handle<Object> value(value_raw, isolate);
   PropertyAttributes old_attributes = self->GetLocalElementAttribute(index);
   Handle<Object> old_value = isolate->factory()->the_hole_value();
-  Handle<Object> old_length;
+  Handle<Object> old_length_handle;
+  Handle<Object> new_length_handle;

[rossberg, 2013/05/27 14:48:41]

Move this and new_length into conditional below, to avoid additional register
pressure on ia32.

[rafaelw, 2013/05/28 11:38:53]

Done.

+  uint32_t old_length = 0;
+  uint32_t new_length = 0;
 
   if (old_attributes != ABSENT) {
     if (self->GetLocalElementAccessorPair(index) == NULL)
       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(), isolate);
+    old_length_handle = handle(Handle<JSArray>::cast(self)->length(), isolate);
+    CHECK(old_length_handle->ToArrayIndex(&old_length));
   }
 
   // Check for lookup interceptor
@@ -12016,11 +12092,25 @@ MaybeObject* JSObject::SetElement(uint32_t index,
   Handle<String> name = isolate->factory()->Uint32ToString(index);
   PropertyAttributes new_attributes = self->GetLocalElementAttribute(index);
   if (old_attributes == ABSENT) {
-    EnqueueChangeRecord(self, "new", name, old_value);
+    bool array_extended = false;

[rossberg, 2013/05/27 14:48:41]

Can we initialize this properly instead of mutating later, i.e.:

bool array_extended = self->IsJSArray() && ...;

But actually, I'd simplify control flow by making it one 'if/else' and duplicate
the single EnqueueChangeRecord call.

[rafaelw, 2013/05/28 11:38:53]

Done.

     if (self->IsJSArray() &&
-        !old_length->SameValue(Handle<JSArray>::cast(self)->length())) {
-      EnqueueChangeRecord(
-          self, "updated", isolate->factory()->length_string(), old_length);
+        !old_length_handle->SameValue(Handle<JSArray>::cast(self)->length())) {
+      array_extended = true;
+      new_length_handle = handle(Handle<JSArray>::cast(self)->length(),
+                                 isolate);
+      CHECK(new_length_handle->ToArrayIndex(&new_length));
+      BeginPerformSplice(Handle<JSArray>::cast(self));
+    }
+
+    EnqueueChangeRecord(self, "new", name, old_value);
+    if (array_extended) {
+      EnqueueChangeRecord(self, "updated", isolate->factory()->length_string(),
+                          old_length_handle);
+
+      EndPerformSplice(Handle<JSArray>::cast(self));
+      Handle<JSArray> deleted = isolate->factory()->NewJSArray(0);
+      EnqueueSpliceRecord(Handle<JSArray>::cast(self), old_length, deleted, 0,
+                          new_length - old_length);
     }
   } else if (old_value->IsTheHole()) {
     EnqueueChangeRecord(self, "reconfigured", name, old_value);