[v8] r12905 committed - Handle Object.observe notifications for setting Array.length...

[codesite...@google.com]

Revision: 12905
Author: ross...@chromium.org
Date: Thu Nov 8 08:12:12 2012
Log: Handle Object.observe notifications for setting Array.length

Also handles notification of deleted properties when an array
is truncated by setting length.

Review URL: https://codereview.chromium.org/11338048
Patch from Adam Klein <ad...@chromium.org>.
http://code.google.com/p/v8/source/detail?r=12905

Modified:
/branches/bleeding_edge/src/accessors.cc
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/test/mjsunit/harmony/object-observe.js

=======================================
--- /branches/bleeding_edge/src/accessors.cc Mon Oct 8 05:58:46 2012
+++ /branches/bleeding_edge/src/accessors.cc Thu Nov 8 08:12:12 2012
@@ -93,6 +93,47 @@
if (wrapper->map() == number_map) return wrapper->value();
return value;
}
+
+
+static MaybeObject* ArraySetLengthObserved(Isolate* isolate,
+ Handle<JSArray> array,
+ Handle<Object>
new_length_handle) {
+ List<Handle<String> > indices;
+ List<Handle<Object> > old_values;
+ Handle<Object> old_length_handle(array->length(), isolate);
+ uint32_t old_length = 0;
+ CHECK(old_length_handle->ToArrayIndex(&old_length));
+ uint32_t new_length = 0;
+ CHECK(new_length_handle->ToArrayIndex(&new_length));
+ // TODO(adamk): This loop can be very slow for arrays in dictionary mode.
+ // Find another way to iterate over arrays with dictionary elements.
+ for (uint32_t i = old_length - 1; i + 1 > new_length; --i) {
+ PropertyAttributes attributes = array->GetLocalElementAttribute(i);
+ if (attributes == ABSENT) continue;
+ // A non-configurable property will cause the truncation operation to
+ // stop at this index.
+ if (attributes == DONT_DELETE) break;
+ // TODO(adamk): Don't fetch the old value if it's an accessor.
+ old_values.Add(Object::GetElement(array, i));
+ indices.Add(isolate->factory()->Uint32ToString(i));
+ }
+
+ MaybeObject* result = array->SetElementsLength(*new_length_handle);
+ Handle<Object> hresult;
+ if (!result->ToHandle(&hresult)) return result;
+
+ CHECK(array->length()->ToArrayIndex(&new_length));
+ if (old_length != new_length) {
+ for (int i = 0; i < indices.length(); ++i) {
+ JSObject::EnqueueChangeRecord(
+ array, "deleted", indices[i], old_values[i]);
+ }
+ JSObject::EnqueueChangeRecord(
+ array, "updated", isolate->factory()->length_symbol(),
+ old_length_handle);
+ }
+ return *hresult;
+}


MaybeObject* Accessors::ArraySetLength(JSObject* object, Object* value,
void*) {
@@ -112,7 +153,7 @@
HandleScope scope(isolate);

// Protect raw pointers.
- Handle<JSObject> object_handle(object, isolate);
+ Handle<JSArray> array_handle(JSArray::cast(object), isolate);
Handle<Object> value_handle(value, isolate);

bool has_exception;
@@ -122,7 +163,11 @@
if (has_exception) return Failure::Exception();

if (uint32_v->Number() == number_v->Number()) {
- return
Handle<JSArray>::cast(object_handle)->SetElementsLength(*uint32_v);
+ if (FLAG_harmony_observation && array_handle->map()->is_observed()) {
+ return ArraySetLengthObserved(isolate, array_handle, uint32_v);
+ } else {
+ return array_handle->SetElementsLength(*uint32_v);
+ }
}
return isolate->Throw(
*isolate->factory()->NewRangeError("invalid_array_length",
=======================================
--- /branches/bleeding_edge/src/objects.cc Thu Nov 8 05:44:59 2012
+++ /branches/bleeding_edge/src/objects.cc Thu Nov 8 08:12:12 2012
@@ -1717,20 +1717,21 @@
if (!result->ToHandle(&hresult)) return result;

if (FLAG_harmony_observation && map()->is_observed()) {
- this->EnqueueChangeRecord(
- "new", handle(name), handle(heap->the_hole_value()));
+ EnqueueChangeRecord(handle(this), "new", handle(name),
+ handle(heap->the_hole_value()));
}

return *hresult;
}


-void JSObject::EnqueueChangeRecord(
- const char* type_str, Handle<String> name, Handle<Object> old_value) {
- Isolate* isolate = GetIsolate();
+void JSObject::EnqueueChangeRecord(Handle<JSObject> object,
+ const char* type_str,
+ Handle<String> name,
+ Handle<Object> old_value) {
+ Isolate* isolate = object->GetIsolate();
HandleScope scope;
Handle<String> type = isolate->factory()->LookupAsciiSymbol(type_str);
- Handle<JSObject> object(this);
Handle<Object> args[] = { type, object, name, old_value };
bool threw;
Execution::Call(Handle<JSFunction>(isolate->observers_notify_change()),
@@ -2998,13 +2999,13 @@

if (FLAG_harmony_observation && map()->is_observed()) {
if (lookup->IsTransition()) {
- self->EnqueueChangeRecord("new", name, old_value);
+ EnqueueChangeRecord(self, "new", name, old_value);
} else {
LookupResult new_lookup(self->GetIsolate());
self->LocalLookup(*name, &new_lookup);
ASSERT(!new_lookup.GetLazyValue()->IsTheHole());
if (!new_lookup.GetLazyValue()->SameValue(*old_value)) {
- self->EnqueueChangeRecord("updated", name, old_value);
+ EnqueueChangeRecord(self, "updated", name, old_value);
}
}
}
@@ -3146,16 +3147,16 @@

if (FLAG_harmony_observation && map()->is_observed()) {
if (lookup.IsTransition()) {
- self->EnqueueChangeRecord("new", name, old_value);
+ EnqueueChangeRecord(self, "new", name, old_value);
} else {
LookupResult new_lookup(isolate);
self->LocalLookup(*name, &new_lookup);
ASSERT(!new_lookup.GetLazyValue()->IsTheHole());
if (old_value->IsTheHole() ||
new_lookup.GetAttributes() != old_attributes) {
- self->EnqueueChangeRecord("reconfigured", name, old_value);
+ EnqueueChangeRecord(self, "reconfigured", name, old_value);
} else if (!new_lookup.GetLazyValue()->SameValue(*old_value)) {
- self->EnqueueChangeRecord("updated", name, old_value);
+ EnqueueChangeRecord(self, "updated", name, old_value);
}
}
}
@@ -4152,7 +4153,7 @@

if (FLAG_harmony_observation && map()->is_observed()) {
if (preexists && !self->HasLocalElement(index))
- self->EnqueueChangeRecord("deleted", name, old_value);
+ EnqueueChangeRecord(self, "deleted", name, old_value);
}

return *hresult;
@@ -4239,7 +4240,7 @@

if (FLAG_harmony_observation && map()->is_observed()) {
if (!self->HasLocalProperty(*hname))
- self->EnqueueChangeRecord("deleted", hname, old_value);
+ EnqueueChangeRecord(self, "deleted", hname, old_value);
}

return *hresult;
@@ -4906,7 +4907,7 @@

if (FLAG_harmony_observation && map()->is_observed()) {
const char* type = preexists ? "reconfigured" : "new";
- self->EnqueueChangeRecord(type, name, old_value);
+ EnqueueChangeRecord(self, type, name, old_value);
}

return *hresult;
@@ -10331,13 +10332,13 @@
if (FLAG_harmony_observation && map()->is_observed()) {
PropertyAttributes new_attributes =
self->GetLocalPropertyAttribute(*name);
if (!preexists) {
- self->EnqueueChangeRecord("new", name, old_value);
+ EnqueueChangeRecord(self, "new", name, old_value);
} else if (new_attributes != old_attributes || old_value->IsTheHole())
{
- self->EnqueueChangeRecord("reconfigured", name, old_value);
+ EnqueueChangeRecord(self, "reconfigured", name, old_value);
} else {
Handle<Object> newValue = Object::GetElement(self, index);
if (!newValue->SameValue(*old_value))
- self->EnqueueChangeRecord("updated", name, old_value);
+ EnqueueChangeRecord(self, "updated", name, old_value);
}
}

=======================================
--- /branches/bleeding_edge/src/objects.h Thu Nov 8 05:44:59 2012
+++ /branches/bleeding_edge/src/objects.h Thu Nov 8 08:12:12 2012
@@ -2208,6 +2208,12 @@
static inline int SizeOf(Map* map, HeapObject* object);
};

+ // Enqueue change record for Object.observe. May cause GC.
+ static void EnqueueChangeRecord(Handle<JSObject> object,
+ const char* type,
+ Handle<String> name,
+ Handle<Object> old_value);
+
// Deliver change records to observers. May cause GC.
static void DeliverChangeRecords(Isolate* isolate);

@@ -2316,11 +2322,6 @@
MUST_USE_RESULT MaybeObject* SetHiddenPropertiesHashTable(
Object* value);

- // Enqueue change record for Object.observe. May cause GC.
- void EnqueueChangeRecord(const char* type,
- Handle<String> name,
- Handle<Object> old_value);
-
DISALLOW_IMPLICIT_CONSTRUCTORS(JSObject);
};

=======================================
--- /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Thu Nov
8 05:15:54 2012
+++ /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Thu Nov
8 08:12:12 2012
@@ -333,6 +333,41 @@
{ object: obj, name: "1", type: "new" },
]);

+// Observing array length (including truncation)
+reset();
+var arr = ['a', 'b', 'c', 'd'];
+var arr2 = ['alpha', 'beta'];
+var arr3 = ['hello'];
+// TODO(adamk): Enable this test case when it can run in a reasonable
+// amount of time.
+//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);
+Object.observe(arr2, observer.callback);
+Object.observe(arr3, observer.callback);
+arr.length = 2;
+arr.length = 0;
+arr.length = 10;
+arr2.length = 0;
+arr2.length = 1; // no change expected
+arr3.length = 0;
+Object.deliverChangeRecords(observer.callback);
+observer.assertCallbackRecords([
+ { object: arr, name: '3', type: 'deleted', oldValue: 'd' },
+ // TODO(adamk): oldValue should not be present below
+ { object: arr, name: '2', type: 'deleted', oldValue: undefined },
+ { object: arr, name: 'length', type: 'updated', oldValue: 4 },
+ { object: arr, name: '1', type: 'deleted', oldValue: 'b' },
+ { object: arr, name: 'length', type: 'updated', oldValue: 2 },
+ { object: arr, name: 'length', type: 'updated', oldValue: 1 },
+ { object: arr2, name: '1', type: 'deleted', oldValue: 'beta' },
+ { object: arr2, name: 'length', type: 'updated', oldValue: 2 },
+ { object: arr3, name: '0', type: 'deleted', oldValue: 'hello' },
+ { object: arr3, name: 'length', type: 'updated', oldValue: 1 },
+]);

// Assignments in loops (checking different IC states).
reset();