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 @@
     object = handle(JSGlobalObject::cast(*object)->global_receiver(), isolate);
   }
   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);
 }
 

[adamk, 2013/05/24 00:15:30]

v8 style nit: need two blank lines between functions.

[rafaelw, 2013/05/24 19:11:43]

Done.

+void JSObject::EnqueueSpliceRecord(Handle<JSArray> object,
+                                   uint32_t index,
+                                   Handle<JSArray> deleted,
+                                   uint32_t delete_count,

[adamk, 2013/05/24 00:15:30]

I know we discussed this before, but can you avoid passing delete_count around?
I don't think you even need to set length explicitly, since you're going through
SetElement() anyway (which will update the length appropriately).

[rafaelw, 2013/05/24 19:11:43]

I've put a todo to go back and remove it. There are now three CLs pending that
aren't dependent on each other. I prefer to land them and then go back and clean
this up.

On 2013/05/24 00:15:30, adamk wrote:

+                                   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(), 5, args,

[adamk, 2013/05/24 00:15:30]

I'd use ARRAY_SIZE(args) here, and below.

[rafaelw, 2013/05/24 19:11:43]

Done.

+                  &threw);
+  ASSERT(!threw);
+}
+

[adamk, 2013/05/24 00:15:30]

same whitespace nit, and so on below

[rafaelw, 2013/05/24 19:11:43]

Done.

+void JSObject::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(), 1, args,
+                  &threw);
+  ASSERT(!threw);
+}
+
+void JSObject::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(), 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 8889 matching lines @@
 }
 
 
 // Returns false if the passed-in index is marked non-configurable,
 // which will cause the ES5 truncation operation to halt, and thus
 // no further old values need be collected.
 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;
 }
 
 
 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> 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;
@@ skipping 19 matching lines @@
       }
     }
   }
 
   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;

[adamk, 2013/05/24 00:15:30]

v8 style puts this on the previous line

[rafaelw, 2013/05/24 19:11:43]

Done.

+
+  JSObject::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);
+
+  JSObject::EndPerformSplice(self);
+
+  uint32_t index = new_length > old_length ? old_length : new_length;

[adamk, 2013/05/24 00:15:30]

this would be slightly more readable as min(old_length, new_length)

[rafaelw, 2013/05/24 19:11:43]

Done.

+  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 < indices.length(); ++i)

[adamk, 2013/05/24 00:15:30]

Note that these indices are in descending order due to the way they were
generated. So you might be better off iterating back-to-front.

[rafaelw, 2013/05/24 19:11:43]

Done.

+      JSObject::SetElement(deleted, 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 1004 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));

[adamk, 2013/05/24 00:15:30]

We should really find a better way to avoid dropping CHECKs everywhere. Not in
this patch, though.

+      JSObject::BeginPerformSplice(Handle<JSArray>::cast(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(Handle<JSArray>::cast(self));
+      Handle<JSArray> deleted = isolate->factory()->NewJSArray(0);
+      JSObject::EnqueueSpliceRecord(Handle<JSArray>::cast(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 3599 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