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 ee0ff5123ab7839fced082347435621321d15f9d..6eb5c621faf872f041f63efc08328db48ca1250a 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -1989,6 +1989,64 @@ void JSObject::EnqueueChangeRecord(Handle<JSObject> object,
   ASSERT(!threw);
 }
 
+void JSObject::EnqueueSpliceRecord(Handle<JSObject> object,
+                                  uint32_t index,

[adamk, 2013/05/20 23:13:13]

Nit: indentation seems off here.

[rafaelw, 2013/05/20 23:52:06]

Done.

+                                  Handle<JSArray> deleted,
+                                  uint32_t delete_count,
+                                  uint32_t add_count) {
+  Isolate* isolate = object->GetIsolate();
+  HandleScope scope(isolate);
+  if (object->IsJSGlobalObject()) {

[adamk, 2013/05/20 23:13:13]

Can this even happen? Do we ever emit splices for non-arrays?

[rafaelw, 2013/05/20 23:52:06]

Replaced with assert.

On 2013/05/20 23:13:13, adamk wrote:

[adamk, 2013/05/21 19:11:11]

Now that you've added the assert I'm wondering what happens with:

var global = this;
global.length = 0;
Array.prototype.push.call(global, 75);

So perhaps I was wrong...

[rafaelw, 2013/05/23 18:19:58]

These only get called from within objects.cc, but I've made it so that all three
take Handle<JSArray>

On 2013/05/21 19:11:11, adamk wrote:

+    object = handle(JSGlobalObject::cast(*object)->global_receiver(), isolate);
+  }
+
+  Handle<Object> index_object = isolate->factory()->NewNumberFromUint(index);
+  Handle<Object> delete_count_object =

[adamk, 2013/05/20 23:13:13]

It's not obvious why we need the delete_count here...why don't we just pass the
correct deleted array?

[rafaelw, 2013/05/20 23:52:06]

You are correct. This is an artifact of having split the original (large) patch
into smaller ones. The receiver side of EnqueueSpliceRecord has already landed,
and there are two patches in flight that depend on that signature. I'll
follow-up with a clean-up and remove the delete_count argument.

On 2013/05/20 23:13:13, adamk wrote:

+      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(), 5, args,
+                  &threw);
+  ASSERT(!threw);
+}
+
+void JSObject::BeginPerformSplice(Handle<JSObject> object) {
+  Isolate* isolate = object->GetIsolate();
+  HandleScope scope(isolate);
+  if (object->IsJSGlobalObject()) {

[adamk, 2013/05/20 23:13:13]

Same question as above, can this happen?

[rafaelw, 2013/05/20 23:52:06]

Done.

+    object = handle(JSGlobalObject::cast(*object)->global_receiver(), isolate);
+  }
+
+  Handle<Object> args[] = { object };
+
+  bool threw;
+  Execution::Call(Handle<JSFunction>(isolate->observers_begin_perform_splice()),
+                  isolate->factory()->undefined_value(), 1, args,
+                  &threw);
+  ASSERT(!threw);
+}
+
+void JSObject::EndPerformSplice(Handle<JSObject> object) {
+  Isolate* isolate = object->GetIsolate();
+  HandleScope scope(isolate);
+  if (object->IsJSGlobalObject()) {

[adamk, 2013/05/20 23:13:13]

ditto

[rafaelw, 2013/05/20 23:52:06]

Done.

+    object = handle(JSGlobalObject::cast(*object)->global_receiver(), isolate);
+  }
+
+  Handle<Object> args[] = { object };
+
+  bool threw;
+  Execution::Call(Handle<JSFunction>(isolate->observers_end_perform_splice()),
+                  isolate->factory()->undefined_value(), 1, args,
+                  &threw);
+  ASSERT(!threw);
+}
 
 void JSObject::DeliverChangeRecords(Isolate* isolate) {
   ASSERT(isolate->observer_delivery_pending());
@@ -10729,6 +10787,7 @@ MaybeObject* JSArray::SetElementsLength(Object* len) {
   HandleScope scope(isolate);
   Handle<JSArray> self(this);
   List<Handle<String> > indices;
+  List<uint32_t> int_indices;
   List<Handle<Object> > old_values;
   Handle<Object> old_length_handle(self->length(), isolate);
   Handle<Object> new_length_handle(len, isolate);
@@ -10749,6 +10808,7 @@ MaybeObject* JSArray::SetElementsLength(Object* len) {
       // Simple case for arrays without holes.
       for (uint32_t i = old_length - 1; i + 1 > new_length; --i) {
         if (!GetOldValue(isolate, self, i, &old_values, &indices)) break;
+        int_indices.Add(i);

[adamk, 2013/05/20 23:13:13]

It seems odd to keep track of the indices twice. Any reason not to store them
only as ints and then generate strings below when needed?

[rafaelw, 2013/05/20 23:52:06]

Done.

       }
     } else {
       // For sparse arrays, only iterate over existing elements.
@@ -10758,6 +10818,7 @@ MaybeObject* JSArray::SetElementsLength(Object* len) {
         uint32_t index = NumberToUint32(keys->get(num_elements));
         if (index < new_length) break;
         if (!GetOldValue(isolate, self, index, &old_values, &indices)) break;
+        int_indices.Add(index);
       }
     }
   }
@@ -10768,15 +10829,33 @@ 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;
+
+  JSObject::BeginPerformSplice(self);
+
+  for (int i = 0; i < indices.length(); ++i) {
     JSObject::EnqueueChangeRecord(
-        self, "updated", isolate->factory()->length_string(),
-        old_length_handle);
+        self, "deleted", indices[i], old_values[i]);
+  }
+  JSObject::EnqueueChangeRecord(
+      self, "updated", isolate->factory()->length_string(),
+      old_length_handle);
+
+  JSObject::EndPerformSplice(self);
+
+  uint32_t index = new_length > old_length ? old_length : new_length;
+  uint32_t add_count = new_length > old_length ? new_length - old_length : 0;
+  uint32_t delete_count = new_length < old_length ? old_length - new_length : 0;
+  Handle<JSArray> deleted = isolate->factory()->NewJSArray(0);
+  if (delete_count) {
+    for (int i = 0; i < int_indices.length(); ++i)
+      JSObject::SetElement(deleted, int_indices[i] - index, old_values[i],
+                           NONE, kNonStrictMode);
   }
+
+  JSObject::EnqueueSpliceRecord(self, index, deleted, delete_count, add_count);
+
   return *hresult;
 }
 
@@ -11802,14 +11881,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;
+  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
@@ -11825,11 +11908,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;
     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));
+      JSObject::BeginPerformSplice(self);
+    }
+
+    EnqueueChangeRecord(self, "new", name, old_value);
+    if (array_extended) {
+      EnqueueChangeRecord(self, "updated", isolate->factory()->length_string(),
+                          old_length_handle);
+
+      JSObject::EndPerformSplice(self);
+      Handle<JSArray> deleted = isolate->factory()->NewJSArray(0);
+      JSObject::EnqueueSpliceRecord(self, old_length, deleted, 0,
+                                    new_length - old_length);
     }
   } else if (old_value->IsTheHole()) {
     EnqueueChangeRecord(self, "reconfigured", name, old_value);