Revision: 17769
Author: [email protected]
Date: Thu Nov 14 21:47:39 2013 UTC
Log: 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-*)
[email protected]
BUG=v8:2946
LOG=N
Review URL: https://codereview.chromium.org/68343016
http://code.google.com/p/v8/source/detail?r=17769
Modified:
/branches/bleeding_edge/src/arm/ic-arm.cc
/branches/bleeding_edge/src/arm/stub-cache-arm.cc
/branches/bleeding_edge/src/builtins.cc
/branches/bleeding_edge/src/ia32/ic-ia32.cc
/branches/bleeding_edge/src/ia32/stub-cache-ia32.cc
/branches/bleeding_edge/src/mips/ic-mips.cc
/branches/bleeding_edge/src/mips/stub-cache-mips.cc
/branches/bleeding_edge/src/objects-debug.cc
/branches/bleeding_edge/src/objects-inl.h
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/src/x64/ic-x64.cc
/branches/bleeding_edge/src/x64/stub-cache-x64.cc
/branches/bleeding_edge/test/mjsunit/harmony/object-observe.js
=======================================
--- /branches/bleeding_edge/src/arm/ic-arm.cc Fri Nov 8 19:33:05 2013 UTC
+++ /branches/bleeding_edge/src/arm/ic-arm.cc Thu Nov 14 21:47:39 2013 UTC
@@ -1432,10 +1432,10 @@
__ 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));
=======================================
--- /branches/bleeding_edge/src/arm/stub-cache-arm.cc Thu Nov 14 16:37:36
2013 UTC
+++ /branches/bleeding_edge/src/arm/stub-cache-arm.cc Thu Nov 14 21:47:39
2013 UTC
@@ -1717,8 +1717,12 @@
// -- sp[argc * 4] : receiver
// -----------------------------------
- // If object is not an array, bail out to regular call.
- if (!object->IsJSArray() || !cell.is_null()) return Handle<Code>::null();
+ // If object is not an array or is observed, bail out to regular call.
+ if (!object->IsJSArray() ||
+ !cell.is_null() ||
+ Handle<JSArray>::cast(object)->map()->is_observed()) {
+ return Handle<Code>::null();
+ }
Label miss;
GenerateNameCheck(name, &miss);
@@ -1971,8 +1975,12 @@
// -- sp[argc * 4] : receiver
// -----------------------------------
- // If object is not an array, bail out to regular call.
- if (!object->IsJSArray() || !cell.is_null()) return Handle<Code>::null();
+ // If object is not an array or is observed, bail out to regular call.
+ if (!object->IsJSArray() ||
+ !cell.is_null() ||
+ Handle<JSArray>::cast(object)->map()->is_observed()) {
+ return Handle<Code>::null();
+ }
Label miss, return_undefined, call_builtin;
Register receiver = r1;
=======================================
--- /branches/bleeding_edge/src/builtins.cc Fri Nov 8 19:33:05 2013 UTC
+++ /branches/bleeding_edge/src/builtins.cc Thu Nov 14 21:47:39 2013 UTC
@@ -311,6 +311,7 @@
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()) {
=======================================
--- /branches/bleeding_edge/src/ia32/ic-ia32.cc Fri Nov 8 19:33:05 2013 UTC
+++ /branches/bleeding_edge/src/ia32/ic-ia32.cc Thu Nov 14 21:47:39 2013 UTC
@@ -874,10 +874,10 @@
__ 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);
=======================================
--- /branches/bleeding_edge/src/ia32/stub-cache-ia32.cc Thu Nov 14 16:37:36
2013 UTC
+++ /branches/bleeding_edge/src/ia32/stub-cache-ia32.cc Thu Nov 14 21:47:39
2013 UTC
@@ -1736,8 +1736,10 @@
// -- 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<JSArray>::cast(object)->map()->is_observed()) {
return Handle<Code>::null();
}
@@ -1995,8 +1997,10 @@
// -- 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<JSArray>::cast(object)->map()->is_observed()) {
return Handle<Code>::null();
}
=======================================
--- /branches/bleeding_edge/src/mips/ic-mips.cc Fri Nov 8 19:33:05 2013 UTC
+++ /branches/bleeding_edge/src/mips/ic-mips.cc Thu Nov 14 21:47:39 2013 UTC
@@ -1354,10 +1354,11 @@
__ 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));
=======================================
--- /branches/bleeding_edge/src/mips/stub-cache-mips.cc Thu Nov 14 21:12:22
2013 UTC
+++ /branches/bleeding_edge/src/mips/stub-cache-mips.cc Thu Nov 14 21:47:39
2013 UTC
@@ -1704,8 +1704,12 @@
// -- sp[argc * 4] : receiver
// -----------------------------------
- // If object is not an array, bail out to regular call.
- if (!object->IsJSArray() || !cell.is_null()) return Handle<Code>::null();
+ // If object is not an array or is observed, bail out to regular call.
+ if (!object->IsJSArray() ||
+ !cell.is_null() ||
+ Handle<JSArray>::cast(object)->map()->is_observed()) {
+ return Handle<Code>::null();
+ }
Label miss;
@@ -1959,8 +1963,12 @@
// -- sp[argc * 4] : receiver
// -----------------------------------
- // If object is not an array, bail out to regular call.
- if (!object->IsJSArray() || !cell.is_null()) return Handle<Code>::null();
+ // If object is not an array or is observed, bail out to regular call.
+ if (!object->IsJSArray() ||
+ !cell.is_null() ||
+ Handle<JSArray>::cast(object)->map()->is_observed()) {
+ return Handle<Code>::null();
+ }
Label miss, return_undefined, call_builtin;
Register receiver = a1;
=======================================
--- /branches/bleeding_edge/src/objects-debug.cc Wed Nov 13 10:34:06 2013
UTC
+++ /branches/bleeding_edge/src/objects-debug.cc Thu Nov 14 21:47:39 2013
UTC
@@ -367,9 +367,6 @@
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());
}
=======================================
--- /branches/bleeding_edge/src/objects-inl.h Thu Nov 14 16:25:31 2013 UTC
+++ /branches/bleeding_edge/src/objects-inl.h Thu Nov 14 21:47:39 2013 UTC
@@ -3667,16 +3667,13 @@
}
-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());
}
=======================================
--- /branches/bleeding_edge/src/objects.cc Thu Nov 14 20:51:18 2013 UTC
+++ /branches/bleeding_edge/src/objects.cc Thu Nov 14 21:47:39 2013 UTC
@@ -5612,12 +5612,6 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
} 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<FixedArray> keys =
isolate->factory()->NewFixedArray(num_elements);
self->GetLocalElementKeys(*keys, kNoAttrFilter);
while (num_elements-- > 0) {
@@ -12890,7 +12880,6 @@
MaybeObject* JSObject::TransitionElementsKind(ElementsKind to_kind) {
- ASSERT(!map()->is_observed());
ElementsKind from_kind = map()->elements_kind();
if (IsFastHoleyElementsKind(from_kind)) {
=======================================
--- /branches/bleeding_edge/src/objects.h Thu Nov 14 17:30:48 2013 UTC
+++ /branches/bleeding_edge/src/objects.h Thu Nov 14 21:47:39 2013 UTC
@@ -5715,7 +5715,7 @@
class FunctionWithPrototype: public BitField<bool, 23, 1> {};
class DictionaryMap: public BitField<bool, 24, 1> {};
class OwnsDescriptors: public BitField<bool, 25, 1> {};
- class IsObserved: public BitField<bool, 26, 1> {};
+ class HasInstanceCallHandler: public BitField<bool, 26, 1> {};
class Deprecated: public BitField<bool, 27, 1> {};
class IsFrozen: public BitField<bool, 28, 1> {};
class IsUnstable: public BitField<bool, 29, 1> {};
@@ -5778,12 +5778,12 @@
}
// 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 @@
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 @@
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 @@
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
=======================================
--- /branches/bleeding_edge/src/x64/ic-x64.cc Fri Nov 8 19:33:05 2013 UTC
+++ /branches/bleeding_edge/src/x64/ic-x64.cc Thu Nov 14 21:47:39 2013 UTC
@@ -749,10 +749,10 @@
__ 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);
=======================================
--- /branches/bleeding_edge/src/x64/stub-cache-x64.cc Thu Nov 14 16:37:36
2013 UTC
+++ /branches/bleeding_edge/src/x64/stub-cache-x64.cc Thu Nov 14 21:47:39
2013 UTC
@@ -1660,8 +1660,12 @@
// -- rsp[(argc + 1) * 8] : receiver
// -----------------------------------
- // If object is not an array, bail out to regular call.
- if (!object->IsJSArray() || !cell.is_null()) return Handle<Code>::null();
+ // If object is not an array or is observed, bail out to regular call.
+ if (!object->IsJSArray() ||
+ !cell.is_null() ||
+ Handle<JSArray>::cast(object)->map()->is_observed()) {
+ return Handle<Code>::null();
+ }
Label miss;
GenerateNameCheck(name, &miss);
@@ -1911,8 +1915,12 @@
// -- rsp[(argc + 1) * 8] : receiver
// -----------------------------------
- // If object is not an array, bail out to regular call.
- if (!object->IsJSArray() || !cell.is_null()) return Handle<Code>::null();
+ // If object is not an array or is observed, bail out to regular call.
+ if (!object->IsJSArray() ||
+ !cell.is_null() ||
+ Handle<JSArray>::cast(object)->map()->is_observed()) {
+ return Handle<Code>::null();
+ }
Label miss, return_undefined, call_builtin;
GenerateNameCheck(name, &miss);
=======================================
--- /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Wed Nov
6 12:14:24 2013 UTC
+++ /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Thu Nov
14 21:47:39 2013 UTC
@@ -613,6 +613,69 @@
multiplied: 2
}
]);
+
+// 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();
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
