From: rafaelw@chromium.org Date: Thu, 14 Nov 2013 21:47:39 +0000 (+0000) Subject: Reland [Object.observe] Don't force normalization of elements for observed objects X-Git-Tag: upstream/4.7.83~11729 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=bdf78a7ad38d7942922a5b427888f6c929c70337;p=platform%2Fupstream%2Fv8.git Reland [Object.observe] Don't force normalization of elements for observed objects Original Issue: https://codereview.chromium.org/29353003/ Note that this version of the patch includes logic for bailing out of compiled ArrayPush/ArrayPop calls if the array is observed (see stub-cache-*) R=danno@chromium.org BUG=v8:2946 LOG=N Review URL: https://codereview.chromium.org/68343016 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@17769 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- diff --git a/src/arm/ic-arm.cc b/src/arm/ic-arm.cc index 025a590..a3b2a6e 100644 --- a/src/arm/ic-arm.cc +++ b/src/arm/ic-arm.cc @@ -1432,10 +1432,10 @@ void KeyedStoreIC::GenerateGeneric(MacroAssembler* masm, __ JumpIfSmi(receiver, &slow); // Get the map of the object. __ ldr(receiver_map, FieldMemOperand(receiver, HeapObject::kMapOffset)); - // Check that the receiver does not require access checks. We need - // to do this because this generic stub does not perform map checks. + // Check that the receiver does not require access checks and is not observed. + // The generic stub does not perform map checks or handle observed objects. __ ldrb(ip, FieldMemOperand(receiver_map, Map::kBitFieldOffset)); - __ tst(ip, Operand(1 << Map::kIsAccessCheckNeeded)); + __ tst(ip, Operand(1 << Map::kIsAccessCheckNeeded | 1 << Map::kIsObserved)); __ b(ne, &slow); // Check if the object is a JS array or not. __ ldrb(r4, FieldMemOperand(receiver_map, Map::kInstanceTypeOffset)); diff --git a/src/arm/stub-cache-arm.cc b/src/arm/stub-cache-arm.cc index 984c7d5..88e220e 100644 --- a/src/arm/stub-cache-arm.cc +++ b/src/arm/stub-cache-arm.cc @@ -1717,8 +1717,12 @@ Handle CallStubCompiler::CompileArrayPushCall( // -- sp[argc * 4] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) return Handle::null(); + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { + return Handle::null(); + } Label miss; GenerateNameCheck(name, &miss); @@ -1971,8 +1975,12 @@ Handle CallStubCompiler::CompileArrayPopCall( // -- sp[argc * 4] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) return Handle::null(); + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { + return Handle::null(); + } Label miss, return_undefined, call_builtin; Register receiver = r1; diff --git a/src/builtins.cc b/src/builtins.cc index 758967e..4077272 100644 --- a/src/builtins.cc +++ b/src/builtins.cc @@ -311,6 +311,7 @@ static inline MaybeObject* EnsureJSArrayWithWritableFastElements( Heap* heap, Object* receiver, Arguments* args, int first_added_arg) { if (!receiver->IsJSArray()) return NULL; JSArray* array = JSArray::cast(receiver); + if (array->map()->is_observed()) return NULL; HeapObject* elms = array->elements(); Map* map = elms->map(); if (map == heap->fixed_array_map()) { diff --git a/src/ia32/ic-ia32.cc b/src/ia32/ic-ia32.cc index 0b7c4a8..dab9dd7 100644 --- a/src/ia32/ic-ia32.cc +++ b/src/ia32/ic-ia32.cc @@ -874,10 +874,10 @@ void KeyedStoreIC::GenerateGeneric(MacroAssembler* masm, __ JumpIfSmi(edx, &slow); // Get the map from the receiver. __ mov(edi, FieldOperand(edx, HeapObject::kMapOffset)); - // Check that the receiver does not require access checks. We need - // to do this because this generic stub does not perform map checks. + // Check that the receiver does not require access checks and is not observed. + // The generic stub does not perform map checks or handle observed objects. __ test_b(FieldOperand(edi, Map::kBitFieldOffset), - 1 << Map::kIsAccessCheckNeeded); + 1 << Map::kIsAccessCheckNeeded | 1 << Map::kIsObserved); __ j(not_zero, &slow); // Check that the key is a smi. __ JumpIfNotSmi(ecx, &slow); diff --git a/src/ia32/stub-cache-ia32.cc b/src/ia32/stub-cache-ia32.cc index 61639a9..b839333 100644 --- a/src/ia32/stub-cache-ia32.cc +++ b/src/ia32/stub-cache-ia32.cc @@ -1736,8 +1736,10 @@ Handle CallStubCompiler::CompileArrayPushCall( // -- esp[(argc + 1) * 4] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) { + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { return Handle::null(); } @@ -1995,8 +1997,10 @@ Handle CallStubCompiler::CompileArrayPopCall( // -- esp[(argc + 1) * 4] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) { + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { return Handle::null(); } diff --git a/src/mips/ic-mips.cc b/src/mips/ic-mips.cc index 0fe044a..c7e1a2a 100644 --- a/src/mips/ic-mips.cc +++ b/src/mips/ic-mips.cc @@ -1354,10 +1354,11 @@ void KeyedStoreIC::GenerateGeneric(MacroAssembler* masm, __ JumpIfSmi(receiver, &slow); // Get the map of the object. __ lw(receiver_map, FieldMemOperand(receiver, HeapObject::kMapOffset)); - // Check that the receiver does not require access checks. We need - // to do this because this generic stub does not perform map checks. + // Check that the receiver does not require access checks and is not observed. + // The generic stub does not perform map checks or handle observed objects. __ lbu(t0, FieldMemOperand(receiver_map, Map::kBitFieldOffset)); - __ And(t0, t0, Operand(1 << Map::kIsAccessCheckNeeded)); + __ And(t0, t0, Operand(1 << Map::kIsAccessCheckNeeded | + 1 << Map::kIsObserved)); __ Branch(&slow, ne, t0, Operand(zero_reg)); // Check if the object is a JS array or not. __ lbu(t0, FieldMemOperand(receiver_map, Map::kInstanceTypeOffset)); diff --git a/src/mips/stub-cache-mips.cc b/src/mips/stub-cache-mips.cc index c1b3ed3..25edc6d 100644 --- a/src/mips/stub-cache-mips.cc +++ b/src/mips/stub-cache-mips.cc @@ -1704,8 +1704,12 @@ Handle CallStubCompiler::CompileArrayPushCall( // -- sp[argc * 4] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) return Handle::null(); + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { + return Handle::null(); + } Label miss; @@ -1959,8 +1963,12 @@ Handle CallStubCompiler::CompileArrayPopCall( // -- sp[argc * 4] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) return Handle::null(); + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { + return Handle::null(); + } Label miss, return_undefined, call_builtin; Register receiver = a1; diff --git a/src/objects-debug.cc b/src/objects-debug.cc index f59687a..ed93e1d 100644 --- a/src/objects-debug.cc +++ b/src/objects-debug.cc @@ -367,9 +367,6 @@ void Map::MapVerify() { SLOW_ASSERT(transitions()->IsSortedNoDuplicates()); SLOW_ASSERT(transitions()->IsConsistentWithBackPointers(this)); } - ASSERT(!is_observed() || instance_type() < FIRST_JS_OBJECT_TYPE || - instance_type() > LAST_JS_OBJECT_TYPE || - has_slow_elements_kind() || has_external_array_elements()); } diff --git a/src/objects-inl.h b/src/objects-inl.h index fced7a9..9358f42 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -3667,16 +3667,13 @@ bool Map::owns_descriptors() { } -void Map::set_is_observed(bool is_observed) { - ASSERT(instance_type() < FIRST_JS_OBJECT_TYPE || - instance_type() > LAST_JS_OBJECT_TYPE || - has_slow_elements_kind() || has_external_array_elements()); - set_bit_field3(IsObserved::update(bit_field3(), is_observed)); +void Map::set_has_instance_call_handler() { + set_bit_field3(HasInstanceCallHandler::update(bit_field3(), true)); } -bool Map::is_observed() { - return IsObserved::decode(bit_field3()); +bool Map::has_instance_call_handler() { + return HasInstanceCallHandler::decode(bit_field3()); } diff --git a/src/objects.cc b/src/objects.cc index 84dea05..671d06f 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -5612,12 +5612,6 @@ void JSObject::SetObserved(Handle object) { if (object->map()->is_observed()) return; - if (!object->HasExternalArrayElements()) { - // Go to dictionary mode, so that we don't skip map checks. - NormalizeElements(object); - ASSERT(!object->HasFastElements()); - } - LookupResult result(isolate); object->map()->LookupTransition(*object, isolate->heap()->observed_symbol(), @@ -5631,7 +5625,7 @@ void JSObject::SetObserved(Handle object) { new_map = Map::CopyForObserved(handle(object->map())); } else { new_map = Map::Copy(handle(object->map())); - new_map->set_is_observed(true); + new_map->set_is_observed(); } object->set_map(*new_map); } @@ -6968,7 +6962,7 @@ Handle Map::CopyForObserved(Handle map) { map->set_transitions(*transitions); - new_map->set_is_observed(true); + new_map->set_is_observed(); if (map->owns_descriptors()) { new_map->InitializeDescriptors(map->instance_descriptors()); @@ -11226,7 +11220,6 @@ MaybeObject* JSObject::SetFastElementsCapacityAndLength( Heap* heap = GetHeap(); // We should never end in here with a pixel or external array. ASSERT(!HasExternalArrayElements()); - ASSERT(!map()->is_observed()); // Allocate a new fast elements backing store. FixedArray* new_elements; @@ -11321,7 +11314,6 @@ MaybeObject* JSObject::SetFastDoubleElementsCapacityAndLength( Heap* heap = GetHeap(); // We should never end in here with a pixel or external array. ASSERT(!HasExternalArrayElements()); - ASSERT(!map()->is_observed()); FixedArrayBase* elems; { MaybeObject* maybe_obj = @@ -11470,10 +11462,6 @@ MaybeObject* JSArray::SetElementsLength(Object* len) { 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) { @@ -11484,6 +11472,8 @@ MaybeObject* JSArray::SetElementsLength(Object* len) { } } else { // For sparse arrays, only iterate over existing elements. + // TODO(rafaelw): For fast, sparse arrays, we can avoid iterating over + // the to-be-removed indices twice. Handle keys = isolate->factory()->NewFixedArray(num_elements); self->GetLocalElementKeys(*keys, kNoAttrFilter); while (num_elements-- > 0) { @@ -12890,7 +12880,6 @@ MaybeObject* JSObject::UpdateAllocationSite(ElementsKind to_kind) { MaybeObject* JSObject::TransitionElementsKind(ElementsKind to_kind) { - ASSERT(!map()->is_observed()); ElementsKind from_kind = map()->elements_kind(); if (IsFastHoleyElementsKind(from_kind)) { diff --git a/src/objects.h b/src/objects.h index 1c415d7..25c2210 100644 --- a/src/objects.h +++ b/src/objects.h @@ -5715,7 +5715,7 @@ class Map: public HeapObject { class FunctionWithPrototype: public BitField {}; class DictionaryMap: public BitField {}; class OwnsDescriptors: public BitField {}; - class IsObserved: public BitField {}; + class HasInstanceCallHandler: public BitField {}; class Deprecated: public BitField {}; class IsFrozen: public BitField {}; class IsUnstable: public BitField {}; @@ -5778,12 +5778,12 @@ class Map: public HeapObject { } // Tells whether the instance has a call-as-function handler. - inline void set_has_instance_call_handler() { - set_bit_field(bit_field() | (1 << kHasInstanceCallHandler)); + inline void set_is_observed() { + set_bit_field(bit_field() | (1 << kIsObserved)); } - inline bool has_instance_call_handler() { - return ((1 << kHasInstanceCallHandler) & bit_field()) != 0; + inline bool is_observed() { + return ((1 << kIsObserved) & bit_field()) != 0; } inline void set_is_extensible(bool value); @@ -5792,10 +5792,6 @@ class Map: public HeapObject { inline void set_elements_kind(ElementsKind elements_kind) { ASSERT(elements_kind < kElementsKindCount); ASSERT(kElementsKindCount <= (1 << kElementsKindBitCount)); - ASSERT(!is_observed() || - elements_kind == DICTIONARY_ELEMENTS || - elements_kind == NON_STRICT_ARGUMENTS_ELEMENTS || - IsExternalArrayElementsKind(elements_kind)); set_bit_field2((bit_field2() & ~kElementsKindMask) | (elements_kind << kElementsKindShift)); ASSERT(this->elements_kind() == elements_kind); @@ -6048,8 +6044,8 @@ class Map: public HeapObject { inline bool owns_descriptors(); inline void set_owns_descriptors(bool is_shared); - inline bool is_observed(); - inline void set_is_observed(bool is_observed); + inline bool has_instance_call_handler(); + inline void set_has_instance_call_handler(); inline void freeze(); inline bool is_frozen(); inline void mark_unstable(); @@ -6308,7 +6304,7 @@ class Map: public HeapObject { static const int kHasNamedInterceptor = 3; static const int kHasIndexedInterceptor = 4; static const int kIsUndetectable = 5; - static const int kHasInstanceCallHandler = 6; + static const int kIsObserved = 6; static const int kIsAccessCheckNeeded = 7; // Bit positions for bit field 2 diff --git a/src/x64/ic-x64.cc b/src/x64/ic-x64.cc index 721ae1d..fe8734c 100644 --- a/src/x64/ic-x64.cc +++ b/src/x64/ic-x64.cc @@ -749,10 +749,10 @@ void KeyedStoreIC::GenerateGeneric(MacroAssembler* masm, __ JumpIfSmi(rdx, &slow_with_tagged_index); // Get the map from the receiver. __ movq(r9, FieldOperand(rdx, HeapObject::kMapOffset)); - // Check that the receiver does not require access checks. We need - // to do this because this generic stub does not perform map checks. + // Check that the receiver does not require access checks and is not observed. + // The generic stub does not perform map checks or handle observed objects. __ testb(FieldOperand(r9, Map::kBitFieldOffset), - Immediate(1 << Map::kIsAccessCheckNeeded)); + Immediate(1 << Map::kIsAccessCheckNeeded | 1 << Map::kIsObserved)); __ j(not_zero, &slow_with_tagged_index); // Check that the key is a smi. __ JumpIfNotSmi(rcx, &slow_with_tagged_index); diff --git a/src/x64/stub-cache-x64.cc b/src/x64/stub-cache-x64.cc index 3a1a179..edfb47b 100644 --- a/src/x64/stub-cache-x64.cc +++ b/src/x64/stub-cache-x64.cc @@ -1660,8 +1660,12 @@ Handle CallStubCompiler::CompileArrayPushCall( // -- rsp[(argc + 1) * 8] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) return Handle::null(); + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { + return Handle::null(); + } Label miss; GenerateNameCheck(name, &miss); @@ -1911,8 +1915,12 @@ Handle CallStubCompiler::CompileArrayPopCall( // -- rsp[(argc + 1) * 8] : receiver // ----------------------------------- - // If object is not an array, bail out to regular call. - if (!object->IsJSArray() || !cell.is_null()) return Handle::null(); + // If object is not an array or is observed, bail out to regular call. + if (!object->IsJSArray() || + !cell.is_null() || + Handle::cast(object)->map()->is_observed()) { + return Handle::null(); + } Label miss, return_undefined, call_builtin; GenerateNameCheck(name, &miss); diff --git a/test/mjsunit/harmony/object-observe.js b/test/mjsunit/harmony/object-observe.js index 39bf6a5..72a9cad 100644 --- a/test/mjsunit/harmony/object-observe.js +++ b/test/mjsunit/harmony/object-observe.js @@ -614,6 +614,69 @@ observer2.assertCallbackRecords([ } ]); +// ArrayPush cached stub +reset(); + +function pushMultiple(arr) { + arr.push('a'); + arr.push('b'); + arr.push('c'); +} + +for (var i = 0; i < 5; i++) { + var arr = []; + pushMultiple(arr); +} + +for (var i = 0; i < 5; i++) { + reset(); + var arr = []; + Object.observe(arr, observer.callback); + pushMultiple(arr); + Object.unobserve(arr, observer.callback); + Object.deliverChangeRecords(observer.callback); + observer.assertCallbackRecords([ + { object: arr, type: 'add', name: '0' }, + { object: arr, type: 'update', name: 'length', oldValue: 0 }, + { object: arr, type: 'add', name: '1' }, + { object: arr, type: 'update', name: 'length', oldValue: 1 }, + { object: arr, type: 'add', name: '2' }, + { object: arr, type: 'update', name: 'length', oldValue: 2 }, + ]); +} + + +// ArrayPop cached stub +reset(); + +function popMultiple(arr) { + arr.pop(); + arr.pop(); + arr.pop(); +} + +for (var i = 0; i < 5; i++) { + var arr = ['a', 'b', 'c']; + popMultiple(arr); +} + +for (var i = 0; i < 5; i++) { + reset(); + var arr = ['a', 'b', 'c']; + Object.observe(arr, observer.callback); + popMultiple(arr); + Object.unobserve(arr, observer.callback); + Object.deliverChangeRecords(observer.callback); + observer.assertCallbackRecords([ + { object: arr, type: 'delete', name: '2', oldValue: 'c' }, + { object: arr, type: 'update', name: 'length', oldValue: 3 }, + { object: arr, type: 'delete', name: '1', oldValue: 'b' }, + { object: arr, type: 'update', name: 'length', oldValue: 2 }, + { object: arr, type: 'delete', name: '0', oldValue: 'a' }, + { object: arr, type: 'update', name: 'length', oldValue: 1 }, + ]); +} + reset(); function RecursiveThingy() {}