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

src/objects.cc

 // Copyright 2013 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 1971 matching lines @@
   }
   Handle<Object> args[] = { type, object, name, old_value };
   bool threw;
   Execution::Call(Handle<JSFunction>(isolate->observers_notify_change()),
                   isolate->factory()->undefined_value(),
                   old_value->IsTheHole() ? 3 : 4, args,
                   &threw);
   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());
   bool threw = false;
   Execution::Call(
       isolate->observers_deliver_changes(),
       isolate->factory()->undefined_value(),
       0,
       NULL,
       &threw);
@@ skipping 8720 matching lines @@
 MaybeObject* JSArray::SetElementsLength(Object* len) {
   // We should never end in here with a pixel or external array.
   ASSERT(AllowsSetElementsLength());
   if (!(FLAG_harmony_observation && map()->is_observed()))
     return GetElementsAccessor()->SetLength(this, len);
 
   Isolate* isolate = GetIsolate();
   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);
   uint32_t old_length = 0;
   CHECK(old_length_handle->ToArrayIndex(&old_length));
   uint32_t new_length = 0;
   if (!new_length_handle->ToArrayIndex(&new_length))
     return Failure::InternalError();
 
   // Observed arrays should always be in dictionary mode;
   // if they were in fast mode, the below is slower than necessary
   // as it iterates over the array backing store multiple times.
   ASSERT(self->HasDictionaryElements());
   static const PropertyAttributes kNoAttrFilter = NONE;
   int num_elements = self->NumberOfLocalElements(kNoAttrFilter);
   if (num_elements > 0) {
     if (old_length == static_cast<uint32_t>(num_elements)) {
       // 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.
       Handle<FixedArray> keys = isolate->factory()->NewFixedArray(num_elements);
       self->GetLocalElementKeys(*keys, kNoAttrFilter);
       while (num_elements-- > 0) {
         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);
       }
     }
   }
 
   MaybeObject* result =
       self->GetElementsAccessor()->SetLength(*self, *new_length_handle);
   Handle<Object> hresult;
   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;
 }
 
 
 Map* Map::GetPrototypeTransition(Object* prototype) {
   FixedArray* cache = GetPrototypeTransitions();
   int number_of_transitions = NumberOfProtoTransitions();
   const int proto_offset =
       kProtoTransitionHeaderSize + kProtoTransitionPrototypeOffset;
   const int map_offset = kProtoTransitionHeaderSize + kProtoTransitionMapOffset;
@@ skipping 1005 matching lines @@
           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<JSObject> self(this);
   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
   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, isolate)) return result;
 
   Handle<String> name = isolate->factory()->Uint32ToString(index);
   PropertyAttributes new_attributes = self->GetLocalElementAttribute(index);
   if (old_attributes == ABSENT) {
+    bool array_extended = false;
+    if (self->IsJSArray() &&
+        !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 (self->IsJSArray() &&
-        !old_length->SameValue(Handle<JSArray>::cast(self)->length())) {
-      EnqueueChangeRecord(
-          self, "updated", isolate->factory()->length_string(), old_length);
+    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);
   } else {
     Handle<Object> new_value = Object::GetElement(self, index);
     bool value_changed = !old_value->SameValue(*new_value);
     if (old_attributes != new_attributes) {
       if (!value_changed) old_value = isolate->factory()->the_hole_value();
       EnqueueChangeRecord(self, "reconfigured", name, old_value);
     } else if (value_changed) {
@@ skipping 3594 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