From de92d0b0e059ee916bab32a393b55c4b72831eb7 Mon Sep 17 00:00:00 2001 From: "adamk@chromium.org" Date: Tue, 4 Jun 2013 23:58:49 +0000 Subject: [PATCH] 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 . git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14944 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/bootstrapper.cc | 5 ++ src/contexts.h | 8 +++ src/objects.cc | 118 ++++++++++++++++++++++++++++----- src/v8natives.js | 53 ++++++++++----- test/mjsunit/harmony/object-observe.js | 76 ++++++++++++++++++--- 5 files changed, 221 insertions(+), 39 deletions(-) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index fb89452..7077241 100644 --- a/src/bootstrapper.cc +++ b/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); } diff --git a/src/contexts.h b/src/contexts.h index 86406e5..f04ccd1 100644 --- a/src/contexts.h +++ b/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, diff --git a/src/objects.cc b/src/objects.cc index 10971cf..c9cc5a4 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -10942,18 +10942,70 @@ static bool GetOldValue(Isolate* isolate, Handle object, uint32_t index, List >* old_values, - List >* indices) { + List* 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::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 object, + uint32_t index, + Handle deleted, + uint32_t delete_count, + uint32_t add_count) { + Isolate* isolate = object->GetIsolate(); + HandleScope scope(isolate); + Handle index_object = isolate->factory()->NewNumberFromUint(index); + Handle delete_count_object = + isolate->factory()->NewNumberFromUint(delete_count); + Handle add_count_object = + isolate->factory()->NewNumberFromUint(add_count); + + Handle args[] = + { object, index_object, deleted, delete_count_object, add_count_object }; + + bool threw; + Execution::Call(Handle(isolate->observers_enqueue_splice()), + isolate->factory()->undefined_value(), ARRAY_SIZE(args), args, + &threw); + ASSERT(!threw); +} + + +static void BeginPerformSplice(Handle object) { + Isolate* isolate = object->GetIsolate(); + HandleScope scope(isolate); + Handle args[] = { object }; + + bool threw; + Execution::Call(Handle(isolate->observers_begin_perform_splice()), + isolate->factory()->undefined_value(), ARRAY_SIZE(args), args, + &threw); + ASSERT(!threw); +} + + +static void EndPerformSplice(Handle object) { + Isolate* isolate = object->GetIsolate(); + HandleScope scope(isolate); + Handle args[] = { object }; + + bool threw; + Execution::Call(Handle(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 self(this); - List > indices; + List indices; List > old_values; Handle old_length_handle(self->length(), isolate); Handle 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 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 value(value_raw, isolate); PropertyAttributes old_attributes = self->GetLocalElementAttribute(index); Handle old_value = isolate->factory()->the_hole_value(); - Handle old_length; + Handle old_length_handle; + Handle 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::cast(self)->length(), isolate); + old_length_handle = handle(Handle::cast(self)->length(), isolate); } // Check for lookup interceptor @@ -12060,11 +12132,25 @@ MaybeObject* JSObject::SetElement(uint32_t index, Handle 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::cast(self)->length())) { - EnqueueChangeRecord( - self, "updated", isolate->factory()->length_string(), old_length); + !old_length_handle->SameValue(Handle::cast(self)->length())) { + new_length_handle = handle(Handle::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::cast(self)); + EnqueueChangeRecord(self, "new", name, old_value); + EnqueueChangeRecord(self, "updated", isolate->factory()->length_string(), + old_length_handle); + EndPerformSplice(Handle::cast(self)); + Handle deleted = isolate->factory()->NewJSArray(0); + EnqueueSpliceRecord(Handle::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); diff --git a/src/v8natives.js b/src/v8natives.js index f5ab122..d444613 100644 --- a/src/v8natives.js +++ b/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; } diff --git a/test/mjsunit/harmony/object-observe.js b/test/mjsunit/harmony/object-observe.js index 4c75e63..0434ccd 100644 --- a/test/mjsunit/harmony/object-observe.js +++ b/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 -- 2.7.4