Reland [Object.observe] Don't force normalization of elements for observed objects
authorrafaelw@chromium.org <rafaelw@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Thu, 14 Nov 2013 21:47:39 +0000 (21:47 +0000)
committerrafaelw@chromium.org <rafaelw@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Thu, 14 Nov 2013 21:47:39 +0000 (21:47 +0000)
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

14 files changed:
src/arm/ic-arm.cc
src/arm/stub-cache-arm.cc
src/builtins.cc
src/ia32/ic-ia32.cc
src/ia32/stub-cache-ia32.cc
src/mips/ic-mips.cc
src/mips/stub-cache-mips.cc
src/objects-debug.cc
src/objects-inl.h
src/objects.cc
src/objects.h
src/x64/ic-x64.cc
src/x64/stub-cache-x64.cc
test/mjsunit/harmony/object-observe.js

index 025a590..a3b2a6e 100644 (file)
@@ -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));
index 984c7d5..88e220e 100644 (file)
@@ -1717,8 +1717,12 @@ Handle<Code> 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<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 @@ Handle<Code> 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<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;
index 758967e..4077272 100644 (file)
@@ -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()) {
index 0b7c4a8..dab9dd7 100644 (file)
@@ -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);
index 61639a9..b839333 100644 (file)
@@ -1736,8 +1736,10 @@ Handle<Code> 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<JSArray>::cast(object)->map()->is_observed()) {
     return Handle<Code>::null();
   }
 
@@ -1995,8 +1997,10 @@ Handle<Code> 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<JSArray>::cast(object)->map()->is_observed()) {
     return Handle<Code>::null();
   }
 
index 0fe044a..c7e1a2a 100644 (file)
@@ -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));
index c1b3ed3..25edc6d 100644 (file)
@@ -1704,8 +1704,12 @@ Handle<Code> 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<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 @@ Handle<Code> 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<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;
index f59687a..ed93e1d 100644 (file)
@@ -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());
 }
 
 
index fced7a9..9358f42 100644 (file)
@@ -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());
 }
 
 
index 84dea05..671d06f 100644 (file)
@@ -5612,12 +5612,6 @@ void JSObject::SetObserved(Handle<JSObject> 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<JSObject> 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> Map::CopyForObserved(Handle<Map> 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<FixedArray> 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)) {
index 1c415d7..25c2210 100644 (file)
@@ -5715,7 +5715,7 @@ class Map: public HeapObject {
   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 @@ 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
index 721ae1d..fe8734c 100644 (file)
@@ -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);
index 3a1a179..edfb47b 100644 (file)
@@ -1660,8 +1660,12 @@ Handle<Code> 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<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 @@ Handle<Code> 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<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);
index 39bf6a5..72a9cad 100644 (file)
@@ -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() {}