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

R=adamk@chromium.org, rossberg@chromium.org

Review URL: https://codereview.chromium.org/15504002

Patch from Rafael Weinstein <rafaelw@chromium.org>.

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14944 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/bootstrapper.cc

@@ -1576,6 +1576,11 @@ void Genesis::InstallExperimentalNativeFunctions() {
   }
   if (FLAG_harmony_observation) {
     INSTALL_NATIVE(JSFunction, "NotifyChange", observers_notify_change);
+    INSTALL_NATIVE(JSFunction, "EnqueueSpliceRecord", observers_enqueue_splice);
+    INSTALL_NATIVE(JSFunction, "BeginPerformSplice",
+                   observers_begin_perform_splice);
+    INSTALL_NATIVE(JSFunction, "EndPerformSplice",
+                   observers_end_perform_splice);
     INSTALL_NATIVE(JSFunction, "DeliverChangeRecords",
                    observers_deliver_changes);
   }

src/contexts.h

@@ -172,6 +172,11 @@ enum BindingFlags {
   V(DERIVED_SET_TRAP_INDEX, JSFunction, derived_set_trap) \
   V(PROXY_ENUMERATE_INDEX, JSFunction, proxy_enumerate) \
   V(OBSERVERS_NOTIFY_CHANGE_INDEX, JSFunction, observers_notify_change) \
+  V(OBSERVERS_ENQUEUE_SPLICE_INDEX, JSFunction, observers_enqueue_splice) \
+  V(OBSERVERS_BEGIN_SPLICE_INDEX, JSFunction, \
+    observers_begin_perform_splice) \
+  V(OBSERVERS_END_SPLICE_INDEX, JSFunction, \
+    observers_end_perform_splice) \
   V(OBSERVERS_DELIVER_CHANGES_INDEX, JSFunction, observers_deliver_changes) \
   V(GENERATOR_FUNCTION_MAP_INDEX, Map, generator_function_map) \
   V(STRICT_MODE_GENERATOR_FUNCTION_MAP_INDEX, Map, \
@@ -317,6 +322,9 @@ class Context: public FixedArray {
     DERIVED_SET_TRAP_INDEX,
     PROXY_ENUMERATE_INDEX,
     OBSERVERS_NOTIFY_CHANGE_INDEX,
+    OBSERVERS_ENQUEUE_SPLICE_INDEX,
+    OBSERVERS_BEGIN_SPLICE_INDEX,
+    OBSERVERS_END_SPLICE_INDEX,
     OBSERVERS_DELIVER_CHANGES_INDEX,
     GENERATOR_FUNCTION_MAP_INDEX,
     STRICT_MODE_GENERATOR_FUNCTION_MAP_INDEX,

src/objects.cc

@@ -10942,18 +10942,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());
@@ -10963,7 +11015,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);
@@ -11003,15 +11055,34 @@ 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;
+  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 = indices.length() - 1; i >= 0; i--) {
+      JSObject::SetElement(deleted, indices[i] - index, old_values[i], NONE,
+                           kNonStrictMode);
+    }
   }
+
+  EnqueueSpliceRecord(self, index, deleted, delete_count, add_count);
+
   return *hresult;
 }
 
@@ -12037,14 +12108,15 @@ 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;
 
   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 for lookup interceptor
@@ -12060,11 +12132,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);
     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())) {
+      new_length_handle = handle(Handle<JSArray>::cast(self)->length(),
+                                 isolate);
+      uint32_t old_length = 0;
+      uint32_t new_length = 0;
+      CHECK(old_length_handle->ToArrayIndex(&old_length));
+      CHECK(new_length_handle->ToArrayIndex(&new_length));
+
+      BeginPerformSplice(Handle<JSArray>::cast(self));
+      EnqueueChangeRecord(self, "new", name, old_value);
+      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 {
+      EnqueueChangeRecord(self, "new", name, old_value);
     }
   } else if (old_value->IsTheHole()) {
     EnqueueChangeRecord(self, "reconfigured", name, old_value);

src/v8natives.js

@@ -873,6 +873,7 @@ function DefineArrayProperty(obj, p, desc, should_throw) {
   // Step 3 - Special handling for length property.
   if (p === "length") {
     var length = obj.length;
+    var old_length = length;
     if (!desc.hasValue()) {
       return DefineObjectProperty(obj, "length", desc, should_throw);
     }
@@ -889,8 +890,24 @@ function DefineArrayProperty(obj, p, desc, should_throw) {
       }
     }
     var threw = false;
+
+    var emit_splice = %IsObserved(obj) && new_length !== old_length;
+    var removed;
+    if (emit_splice) {
+      BeginPerformSplice(obj);
+      removed = [];
+      if (new_length < old_length)
+        removed.length = old_length - new_length;
+    }
+
     while (new_length < length--) {
-      if (!Delete(obj, ToString(length), false)) {
+      var index = ToString(length);
+      if (emit_splice) {
+        var deletedDesc = GetOwnProperty(obj, index);
+        if (deletedDesc && deletedDesc.hasValue())
+          removed[length - new_length] = deletedDesc.getValue();
+      }
+      if (!Delete(obj, index, false)) {
         new_length = length + 1;
         threw = true;
         break;
@@ -902,13 +919,18 @@ function DefineArrayProperty(obj, p, desc, should_throw) {
     // respective TODO in Runtime_DefineOrRedefineDataProperty.
     // For the time being, we need a hack to prevent Object.observe from
     // generating two change records.
-    var isObserved = %IsObserved(obj);
-    if (isObserved) %SetIsObserved(obj, false);
     obj.length = new_length;
     desc.value_ = void 0;
     desc.hasValue_ = false;
     threw = !DefineObjectProperty(obj, "length", desc, should_throw) || threw;
-    if (isObserved) %SetIsObserved(obj, true);
+    if (emit_splice) {
+      EndPerformSplice(obj);
+      EnqueueSpliceRecord(obj,
+          new_length < old_length ? new_length : old_length,
+          removed,
+          removed.length,
+          new_length > old_length ? new_length - old_length : 0);
+    }
     if (threw) {
       if (should_throw) {
         throw MakeTypeError("redefine_disallowed", [p]);
@@ -916,27 +938,24 @@ function DefineArrayProperty(obj, p, desc, should_throw) {
         return false;
       }
     }
-    if (isObserved) {
-      var new_desc = GetOwnProperty(obj, "length");
-      var updated = length_desc.value_ !== new_desc.value_;
-      var reconfigured = length_desc.writable_ !== new_desc.writable_ ||
-          length_desc.configurable_ !== new_desc.configurable_ ||
-          length_desc.enumerable_ !== new_desc.configurable_;
-      if (updated || reconfigured) {
-        NotifyChange(reconfigured ? "reconfigured" : "updated",
-            obj, "length", length_desc.value_);
-      }
-    }
     return true;
   }
 
   // Step 4 - Special handling for array index.
   var index = ToUint32(p);
+  var emit_splice = false;
   if (ToString(index) == p && index != 4294967295) {
     var length = obj.length;
+    if (index >= length && %IsObserved(obj)) {
+      emit_splice = true;
+      BeginPerformSplice(obj);
+    }
+
     var length_desc = GetOwnProperty(obj, "length");
     if ((index >= length && !length_desc.isWritable()) ||
         !DefineObjectProperty(obj, p, desc, true)) {
+      if (emit_splice)
+        EndPerformSplice(obj);
       if (should_throw) {
         throw MakeTypeError("define_disallowed", [p]);
       } else {
@@ -946,6 +965,10 @@ function DefineArrayProperty(obj, p, desc, should_throw) {
     if (index >= length) {
       obj.length = index + 1;
     }
+    if (emit_splice) {
+      EndPerformSplice(obj);
+      EnqueueSpliceRecord(obj, length, [], 0, index + 1 - length);
+    }
     return true;
   }
 

test/mjsunit/harmony/object-observe.js

@@ -957,15 +957,15 @@ var arr2 = ['alpha', 'beta'];
 var arr3 = ['hello'];
 arr3[2] = 'goodbye';
 arr3.length = 6;
-var slow_arr = new Array(1000000000);
-slow_arr[500000000] = 'hello';
 Object.defineProperty(arr, '0', {configurable: false});
 Object.defineProperty(arr, '2', {get: function(){}});
 Object.defineProperty(arr2, '0', {get: function(){}, configurable: false});
 Object.observe(arr, observer.callback);
+Array.observe(arr, observer2.callback);
 Object.observe(arr2, observer.callback);
+Array.observe(arr2, observer2.callback);
 Object.observe(arr3, observer.callback);
-Object.observe(slow_arr, observer.callback);
+Array.observe(arr3, observer2.callback);
 arr.length = 2;
 arr.length = 0;
 arr.length = 10;
@@ -978,8 +978,8 @@ arr3.length = 0;
 arr3.length++;
 arr3.length /= 2;
 Object.defineProperty(arr3, 'length', {value: 5});
-Object.defineProperty(arr3, 'length', {value: 10, writable: false});
-slow_arr.length = 100;
+arr3[4] = 5;
+Object.defineProperty(arr3, 'length', {value: 1, writable: false});
 Object.deliverChangeRecords(observer.callback);
 observer.assertCallbackRecords([
   { object: arr, name: '3', type: 'deleted', oldValue: 'd' },
@@ -991,7 +991,7 @@ observer.assertCallbackRecords([
   { object: arr, name: 'length', type: 'reconfigured' },
   { object: arr2, name: '1', type: 'deleted', oldValue: 'beta' },
   { object: arr2, name: 'length', type: 'updated', oldValue: 2 },
-  { object: arr2, name: 'length', type: 'reconfigured', oldValue: 1 },
+  { object: arr2, name: 'length', type: 'reconfigured' },
   { object: arr3, name: '2', type: 'deleted', oldValue: 'goodbye' },
   { object: arr3, name: '0', type: 'deleted', oldValue: 'hello' },
   { object: arr3, name: 'length', type: 'updated', oldValue: 6 },
@@ -999,10 +999,60 @@ observer.assertCallbackRecords([
   { object: arr3, name: 'length', type: 'updated', oldValue: 1 },
   { object: arr3, name: 'length', type: 'updated', oldValue: 2 },
   { object: arr3, name: 'length', type: 'updated', oldValue: 1 },
-  { object: arr3, name: 'length', type: 'reconfigured', oldValue: 5 },
+  { object: arr3, name: '4', type: 'new' },
+  { object: arr3, name: '4', type: 'deleted', oldValue: 5 },
+  // TODO(rafaelw): It breaks spec compliance to get two records here.
+  // When the TODO in v8natives.js::DefineArrayProperty is addressed
+  // which prevents DefineProperty from over-writing the magic length
+  // property, these will collapse into a single record.
+  { object: arr3, name: 'length', type: 'updated', oldValue: 5 },
+  { object: arr3, name: 'length', type: 'reconfigured' }
+]);
+Object.deliverChangeRecords(observer2.callback);
+observer2.assertCallbackRecords([
+  { object: arr, type: 'splice', index: 2, removed: [, 'd'], addedCount: 0 },
+  { object: arr, type: 'splice', index: 1, removed: ['b'], addedCount: 0 },
+  { object: arr, type: 'splice', index: 1, removed: [], addedCount: 9 },
+  { object: arr2, type: 'splice', index: 1, removed: ['beta'], addedCount: 0 },
+  { object: arr3, type: 'splice', index: 0, removed: ['hello',, 'goodbye',,,,], addedCount: 0 },
+  { object: arr3, type: 'splice', index: 0, removed: [], addedCount: 1 },
+  { object: arr3, type: 'splice', index: 1, removed: [], addedCount: 1 },
+  { object: arr3, type: 'splice', index: 1, removed: [,], addedCount: 0 },
+  { object: arr3, type: 'splice', index: 1, removed: [], addedCount: 4 },
+  { object: arr3, name: '4', type: 'new' },
+  { object: arr3, type: 'splice', index: 1, removed: [,,,5], addedCount: 0 }
+]);
+
+
+// Updating length on large (slow) array
+reset();
+var slow_arr = new Array(1000000000);
+slow_arr[500000000] = 'hello';
+Object.observe(slow_arr, observer.callback);
+var spliceRecords;
+function slowSpliceCallback(records) {
+  spliceRecords = records;
+}
+Array.observe(slow_arr, slowSpliceCallback);
+slow_arr.length = 100;
+Object.deliverChangeRecords(observer.callback);
+observer.assertCallbackRecords([
   { object: slow_arr, name: '500000000', type: 'deleted', oldValue: 'hello' },
   { object: slow_arr, name: 'length', type: 'updated', oldValue: 1000000000 },
 ]);
+Object.deliverChangeRecords(slowSpliceCallback);
+assertEquals(spliceRecords.length, 1);
+// Have to custom assert this splice record because the removed array is huge.
+var splice = spliceRecords[0];
+assertSame(splice.object, slow_arr);
+assertEquals(splice.type, 'splice');
+assertEquals(splice.index, 100);
+assertEquals(splice.addedCount, 0);
+var array_keys = %GetArrayKeys(splice.removed, splice.removed.length);
+assertEquals(array_keys.length, 1);
+assertEquals(array_keys[0], 499999900);
+assertEquals(splice.removed[499999900], 'hello');
+assertEquals(splice.removed.length, 999999900);
 
 
 // Assignments in loops (checking different IC states).
@@ -1037,10 +1087,12 @@ observer.assertCallbackRecords([
 ]);
 
 
-// Adding elements past the end of an array should notify on length
+// Adding elements past the end of an array should notify on length for
+// Object.observe and emit "splices" for Array.observe.
 reset();
 var arr = [1, 2, 3];
 Object.observe(arr, observer.callback);
+Array.observe(arr, observer2.callback);
 arr[3] = 10;
 arr[100] = 20;
 Object.defineProperty(arr, '200', {value: 7});
@@ -1058,6 +1110,14 @@ observer.assertCallbackRecords([
   { object: arr, name: 'length', type: 'updated', oldValue: 201 },
   { object: arr, name: '50', type: 'new' },
 ]);
+Object.deliverChangeRecords(observer2.callback);
+observer2.assertCallbackRecords([
+  { object: arr, type: 'splice', index: 3, removed: [], addedCount: 1 },
+  { object: arr, type: 'splice', index: 4, removed: [], addedCount: 97 },
+  { object: arr, type: 'splice', index: 101, removed: [], addedCount: 100 },
+  { object: arr, type: 'splice', index: 201, removed: [], addedCount: 200 },
+  { object: arr, type: 'new', name: '50' },
+]);
 
 
 // Tests for array methods, first on arrays and then on plain objects