Correct handling of arrays with callbacks in the prototype chain.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 414758f0e8350ec9c8be5c2cbd21453dca22a398..6527cae2cc2b76c8d6127fa12c43426c3682cf5e 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2948,6 +2948,30 @@ Handle<Object> JSReceiver::SetPropertyWithDefinedSetter(
 }
 
 
+bool Map::MayHaveIndexedCallbacksInPrototypeChain() {
+  Heap* heap = GetHeap();
+
+  if (has_element_callbacks()) {
+    return true;
+  }
+
+  for (Object* pt = prototype();

[danno, 2013/10/30 12:17:45]

don't use the abbreviation, use the variable name prototype and call the
function by specifying this->prototype()

[mvstanton, 2013/10/30 18:22:28]

Done.

+       pt != heap->null_value();
+       pt = pt->GetPrototype(GetIsolate())) {
+    if (pt->IsJSProxy()) {
+      // Be conservative, don't walk into proxies.
+      return true;
+    }
+
+    if (JSObject::cast(pt)->map()->has_element_callbacks()) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+
 MaybeObject* JSObject::SetElementWithCallbackSetterInPrototypes(
     uint32_t index,
     Object* value,
@@ -4478,9 +4502,10 @@ Handle<Map> NormalizedMapCache::Get(Handle<NormalizedMapCache> cache,
   if (result->IsMap() &&
       Handle<Map>::cast(result)->EquivalentToForNormalization(obj->map(),
                                                               mode)) {
+    Handle<Map> result_map = Handle<Map>::cast(result);
 #ifdef VERIFY_HEAP
     if (FLAG_verify_heap) {
-      Handle<Map>::cast(result)->SharedMapVerify();
+      result_map->SharedMapVerify();
     }
 #endif
 #ifdef ENABLE_SLOW_ASSERTS
@@ -4492,17 +4517,36 @@ Handle<Map> NormalizedMapCache::Get(Handle<NormalizedMapCache> cache,
                                               SHARED_NORMALIZED_MAP);
 
       ASSERT(memcmp(fresh->address(),
-                    Handle<Map>::cast(result)->address(),
-                    Map::kCodeCacheOffset) == 0);
+                    result_map->address(),
+                    Map::kTransitionsOrBackPointerOffset) == 0);
       STATIC_ASSERT(Map::kDependentCodeOffset ==
-                    Map::kCodeCacheOffset + kPointerSize);
-      int offset = Map::kDependentCodeOffset + kPointerSize;
-      ASSERT(memcmp(fresh->address() + offset,
-                    Handle<Map>::cast(result)->address() + offset,
-                    Map::kSize - offset) == 0);
+                    Map::kTransitionsOrBackPointerOffset + 3 * kPointerSize);
+
+      // The final fields to look at are flag fields 3 and 4. Field 3

[danno, 2013/10/30 12:17:45]

Is the logic needed? I thought the conclusion was other than the transitions
array, everything in this slow assert logic is OK. In that case, it's probably
better/safer to just use the original memcmp, since that will automatically
catch problems with new fields that are added to the Map without having to
explicitly add them in the code here.

+      // needs special attention.
+      STATIC_ASSERT(Map::kDependentCodeOffset + kPointerSize ==
+                    Map::kBitField3Offset);
+      STATIC_ASSERT(Map::kSize - (2 * kPointerSize) == Map::kBitField3Offset);
+      STATIC_ASSERT(Map::kSize - kPointerSize == Map::kBitField4Offset);
+
+      int mask = Map::EnumLengthBits::kMask |
+          Map::NumberOfOwnDescriptorsBits::kMask |
+          Map::IsShared::kMask |
+          Map::FunctionWithPrototype::kMask |
+          Map::DictionaryMap::kMask |
+          // OwnsDescriptors::kMask |
+          Map::IsObserved::kMask |
+          Map::Deprecated::kMask |
+          Map::IsFrozen::kMask |
+          Map::IsUnstable::kMask |
+          Map::IsMigrationTarget::kMask;
+      int fresh_field3 = fresh->bit_field3() & mask;
+      int result_field3 = result_map->bit_field3() & mask;
+      ASSERT(fresh_field3 == result_field3);
+      ASSERT(fresh->bit_field4() == result_map->bit_field4());
     }
 #endif
-    return Handle<Map>::cast(result);
+    return result_map;
   }
 
   Isolate* isolate = cache->GetIsolate();
@@ -4703,39 +4747,15 @@ Handle<SeededNumberDictionary> JSObject::NormalizeElements(
 
 
 MaybeObject* JSObject::NormalizeElements() {
-  ASSERT(!HasExternalArrayElements());
-
-  // Find the backing store.
-  FixedArrayBase* array = FixedArrayBase::cast(elements());
-  Map* old_map = array->map();
-  bool is_arguments =
-      (old_map == old_map->GetHeap()->non_strict_arguments_elements_map());
-  if (is_arguments) {
-    array = FixedArrayBase::cast(FixedArray::cast(array)->get(1));
-  }
-  if (array->IsDictionary()) return array;
-
-  ASSERT(HasFastSmiOrObjectElements() ||
-         HasFastDoubleElements() ||
-         HasFastArgumentsElements());
-  // Compute the effective length and allocate a new backing store.
-  int length = IsJSArray()
-      ? Smi::cast(JSArray::cast(this)->length())->value()
-      : array->length();
-  int old_capacity = 0;
-  int used_elements = 0;
-  GetElementsCapacityAndUsage(&old_capacity, &used_elements);
   SeededNumberDictionary* dictionary;
-  MaybeObject* maybe_dictionary =
-      SeededNumberDictionary::Allocate(GetHeap(), used_elements);
-  if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
+  FixedArrayBase* array = GetElements();

[danno, 2013/10/30 12:17:45]

I'd call this GetElementBackingStore(), and you might want to find the places in
the code that can use it (look for "get(1)" in this file).

+  if (array->IsDictionary()) return array;
 
-  maybe_dictionary = CopyFastElementsToDictionary(
-      GetIsolate(), array, length, dictionary);
+  MaybeObject* maybe_dictionary = CreateNormalizedElements();
   if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
 
   // Switch to using the dictionary as the backing storage for elements.
-  if (is_arguments) {
+  if (HasNonStrictArgumentsElementsMap()) {
     FixedArray::cast(elements())->set(1, dictionary);
   } else {
     // Set the new map first to satify the elements type assert in
@@ -4748,8 +4768,7 @@ MaybeObject* JSObject::NormalizeElements() {
     set_elements(dictionary);
   }
 
-  old_map->GetHeap()->isolate()->counters()->elements_to_dictionary()->
-      Increment();
+  GetHeap()->isolate()->counters()->elements_to_dictionary()->Increment();
 
 #ifdef DEBUG
   if (FLAG_trace_normalization) {
@@ -4763,6 +4782,44 @@ MaybeObject* JSObject::NormalizeElements() {
 }
 
 
+Handle<SeededNumberDictionary> JSObject::CreateNormalizedElements(
+    Handle<JSObject> object) {
+  CALL_HEAP_FUNCTION(object->GetIsolate(),
+                     object->CreateNormalizedElements(),
+                     SeededNumberDictionary);
+}
+
+
+MaybeObject* JSObject::CreateNormalizedElements() {
+  ASSERT(!HasExternalArrayElements());
+
+  // Find the backing store.
+  FixedArrayBase* array = GetElements();
+  ASSERT(!array->IsDictionary());
+
+  ASSERT(HasFastSmiOrObjectElements() ||
+         HasFastDoubleElements() ||
+         HasFastArgumentsElements());
+  // Compute the effective length and allocate a new backing store.
+  int length = IsJSArray()
+      ? Smi::cast(JSArray::cast(this)->length())->value()
+      : array->length();
+  int old_capacity = 0;
+  int used_elements = 0;
+  GetElementsCapacityAndUsage(&old_capacity, &used_elements);
+  SeededNumberDictionary* dictionary;
+  MaybeObject* maybe_dictionary =
+      SeededNumberDictionary::Allocate(GetHeap(), used_elements);
+  if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
+
+  maybe_dictionary = CopyFastElementsToDictionary(
+      GetIsolate(), array, length, dictionary);
+  if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
+
+  return dictionary;
+}
+
+
 Smi* JSReceiver::GenerateIdentityHash() {
   Isolate* isolate = GetIsolate();
 
@@ -5565,7 +5622,7 @@ Handle<Object> JSObject::Freeze(Handle<JSObject> object) {
     Map* transition_map = result.GetTransitionTarget();
     ASSERT(transition_map->has_dictionary_elements());
     ASSERT(transition_map->is_frozen());
-    ASSERT(!transition_map->is_extensible());
+     ASSERT(!transition_map->is_extensible());

[danno, 2013/10/30 12:17:45]

nit: remove stray whitespace change.

[mvstanton, 2013/10/30 18:22:28]

Done.

     object->set_map(transition_map);
   } else if (object->HasFastProperties() && old_map->CanHaveMoreTransitions()) {
     // Create a new descriptor array with fully-frozen properties
@@ -6226,11 +6283,56 @@ void JSObject::SetElementCallback(Handle<JSObject> object,
                                   uint32_t index,
                                   Handle<Object> structure,
                                   PropertyAttributes attributes) {
+  Isolate* isolate = object->GetIsolate();
   Heap* heap = object->GetHeap();
   PropertyDetails details = PropertyDetails(attributes, CALLBACKS, 0);
+  bool is_arguments = object->HasNonStrictArgumentsElements();
+
+  // Do we need to create a new dictionary?
+  Handle<SeededNumberDictionary> new_element_dictionary;

[danno, 2013/10/30 12:17:45]

I think NormalizeElements will do the right thing without all of this extra
logic, including gcreating a transition from the fast elements kind map to the
new dictionary map if you need one, or just leaving the object in dictionary
mode and not changing the map if you are already here.

[mvstanton, 2013/10/30 18:22:28]

Yep, I thought combining the move to dictionary and setting the
HasElementCallbacks bit in one transition would be better, but it sure does
simplify things if I let go of that!

+  FixedArrayBase* array = object->GetElements();
+  if (!array->IsDictionary()) {
+    new_element_dictionary = CreateNormalizedElements(object);
+  }
+
+  if (!object->map()->has_element_callbacks()) {
+    LookupResult result(isolate);
+    Handle<Map> old_map(object->map());
+    old_map->LookupTransition(*object,

[danno, 2013/10/30 12:17:45]

It looks like JSObject::SetObserve does almost exactly the same logic as you
have here. Maybe it makes sense to have a SetHasElementsCallback or
PrepareForSetElementCallback that works just like SetObserve (including the
logic to to ignore external array elements, e.g. TypedArrays). Sorry, I should
have found that code for your sooner.

[mvstanton, 2013/10/30 18:22:28]

addressed.

+                              heap->element_callbacks_symbol(),
+                              &result);
+    if (result.IsTransition()) {
+      object->set_map(result.GetTransitionTarget());
+    } else {
+      Handle<Map> new_map = Map::CopyAsElementCallbacksTransition(old_map,
+                                                             !is_arguments);
+      object->set_map(*new_map);
+    }
+
+    ASSERT(object->map()->has_element_callbacks());
+
+    // Install the dictionary if required without doing an extra transition.
+    if (!new_element_dictionary.is_null()) {
+      if (is_arguments) {
+        FixedArray::cast(object->elements())->set(1, *new_element_dictionary);
+      } else {
+        object->set_elements(*new_element_dictionary);
+      }
+    }
+
+    // KeyedStoreICs (at least the non-generic ones) need a reset.
+    // I *could* look at the maps and only reset the poly/mono ones that

[danno, 2013/10/30 12:17:45]

nit: we try to avoid using "I" or "we" in comments. Passive voice is your enemy
in writing a novel, but your friend here :-p

[mvstanton, 2013/10/30 18:22:28]

:) done.

+    // depend on a map in this prototype chain?
+    heap->ClearAllICsByKind(Code::KEYED_STORE_IC);
+  } else {
+    // If our map is already flagged for element callbacks, then we should
+    // not have had to create a new element dictionary.
+    ASSERT(new_element_dictionary.is_null());
+  }
+
+  Handle<SeededNumberDictionary> dictionary = Handle<SeededNumberDictionary>(
+      SeededNumberDictionary::cast(object->GetElements()), isolate);
 
-  // Normalize elements to make this operation simple.
-  Handle<SeededNumberDictionary> dictionary = NormalizeElements(object);
   ASSERT(object->HasDictionaryElements() ||
          object->HasDictionaryArgumentsElements());
 
@@ -6240,7 +6342,7 @@ void JSObject::SetElementCallback(Handle<JSObject> object,
   dictionary->set_requires_slow_elements();
 
   // Update the dictionary backing store on the object.
-  if (object->elements()->map() == heap->non_strict_arguments_elements_map()) {
+  if (is_arguments) {
     // Also delete any parameter alias.
     //
     // TODO(kmillikin): when deleting the last parameter alias we could
@@ -6672,6 +6774,7 @@ MaybeObject* Map::RawCopy(int instance_size) {
   new_bit_field3 = Deprecated::update(new_bit_field3, false);
   new_bit_field3 = IsUnstable::update(new_bit_field3, false);
   result->set_bit_field3(new_bit_field3);
+  result->set_bit_field4(bit_field4());
   return result;
 }
 
@@ -6923,6 +7026,60 @@ MaybeObject* Map::CopyAsElementsKind(ElementsKind kind, TransitionFlag flag) {
 }
 
 
+MaybeObject* Map::CopyAsElementCallbacksTransition(

[danno, 2013/10/30 12:17:45]

I think with a little work, you can 100% share this code with CopyForObserved,
refactoring to call it CopyForTransitionedMap, and just pustting the logic to
set either "set_is_observed" or "set_has_elements_callback" either in wrapper
functions CopyForObserved/CopyForElementsCallback, or just directly in
SetObserved/SetElementCallback.

[mvstanton, 2013/10/30 18:22:28]

I this this in a basic way, with a protected function that does a "meaningless"
transition, then the caller can set a map bit on the new map. Let me know if it
should be fancier (there could be a macro with a code snippet, for example).

+    bool with_dictionary_elements) {
+  ASSERT(!HasElementCallbacksTransition());
+
+  if (owns_descriptors()) {
+    // In case the map owned its own descriptors, share the descriptors and
+    // transfer ownership to the new map.
+    Map* new_map;
+    MaybeObject* maybe_new_map = CopyDropDescriptors();
+    if (!maybe_new_map->To(&new_map)) return maybe_new_map;
+
+    MaybeObject* added_elements = set_element_callbacks_map(new_map);
+    if (added_elements->IsFailure()) return added_elements;
+
+    new_map->InitializeDescriptors(instance_descriptors());
+    new_map->set_has_element_callbacks(true);
+    if (with_dictionary_elements) {
+      new_map->set_elements_kind(DICTIONARY_ELEMENTS);
+    }
+    new_map->SetBackPointer(this);
+    set_owns_descriptors(false);
+    return new_map;
+  }
+
+  // In case the map did not own its own descriptors, a split is forced by
+  // copying the map; creating a new descriptor array cell.
+  // Create a new free-floating map only if we are not allowed to store it.
+  Map* new_map;
+  MaybeObject* maybe_new_map = Copy();
+  if (!maybe_new_map->To(&new_map)) return maybe_new_map;
+  new_map->set_has_element_callbacks(true);
+  if (with_dictionary_elements) {
+    new_map->set_elements_kind(DICTIONARY_ELEMENTS);
+  }
+
+  MaybeObject* added_elements = set_element_callbacks_map(new_map);
+  if (added_elements->IsFailure()) return added_elements;
+  new_map->SetBackPointer(this);
+
+  return new_map;
+}
+
+
+Handle<Map> Map::CopyAsElementCallbacksTransition(
+    Handle<Map> map,
+    bool with_dictionary_elements) {
+  Isolate* isolate = map->GetIsolate();
+  CALL_HEAP_FUNCTION(isolate,
+                     map->CopyAsElementCallbacksTransition(
+                         with_dictionary_elements),
+                     Map);
+}
+
+
 Handle<Map> Map::CopyForObserved(Handle<Map> map) {
   ASSERT(!map->is_observed());
 
@@ -9431,6 +9588,7 @@ static bool CheckEquivalent(Map* first, Map* second) {
     first->instance_type() == second->instance_type() &&
     first->bit_field() == second->bit_field() &&
     first->bit_field2() == second->bit_field2() &&
+    first->bit_field4() == second->bit_field4() &&
     first->is_observed() == second->is_observed() &&
     first->function_with_prototype() == second->function_with_prototype();
 }
@@ -10565,6 +10723,16 @@ void Code::ReplaceNthCell(int n, Cell* replace_with) {
 
 
 void Code::ClearInlineCaches() {
+  ClearInlineCaches(NULL);
+}
+
+
+void Code::ClearInlineCaches(Code::Kind kind) {
+  ClearInlineCaches(&kind);
+}
+
+
+void Code::ClearInlineCaches(Code::Kind* kind) {
   int mask = RelocInfo::ModeMask(RelocInfo::CODE_TARGET) |
              RelocInfo::ModeMask(RelocInfo::CONSTRUCT_CALL) |
              RelocInfo::ModeMask(RelocInfo::CODE_TARGET_WITH_ID) |
@@ -10573,7 +10741,9 @@ void Code::ClearInlineCaches() {
     RelocInfo* info = it.rinfo();
     Code* target(Code::GetCodeFromTargetAddress(info->target_address()));
     if (target->is_inline_cache_stub()) {
-      IC::Clear(this->GetIsolate(), info->pc());
+      if (kind == NULL || *kind == target->kind()) {
+        IC::Clear(this->GetIsolate(), info->pc());
+      }
     }
   }
 }
@@ -11746,6 +11916,8 @@ Handle<Object> JSObject::SetPrototype(Handle<JSObject> object,
     }
   }
 
+  bool has_element_callbacks_in_chain =
+      object->map()->MayHaveIndexedCallbacksInPrototypeChain();
   Handle<JSObject> real_receiver = object;
 
   if (skip_hidden_prototypes) {
@@ -11778,6 +11950,13 @@ Handle<Object> JSObject::SetPrototype(Handle<JSObject> object,
   ASSERT(new_map->prototype() == *value);
   real_receiver->set_map(*new_map);
 
+  if (!has_element_callbacks_in_chain &&
+      new_map->MayHaveIndexedCallbacksInPrototypeChain()) {
+    // If our prototype chain didn't have element callbacks, and now we do
+    // have them, then we need to clear KeyedStoreICs.
+    object->GetHeap()->ClearAllICsByKind(Code::KEYED_STORE_IC);
+  }
+
   heap->ClearInstanceofCache();
   ASSERT(size == object->Size());
   return value;
@@ -12767,8 +12946,11 @@ MaybeObject* JSObject::TransitionElementsKind(ElementsKind to_kind) {
 
   if (from_kind == to_kind) return this;
 
-  MaybeObject* maybe_failure = UpdateAllocationSite(to_kind);
-  if (maybe_failure->IsFailure()) return maybe_failure;
+  // Don't update the site if to_kind isn't fast
+  if (IsFastElementsKind(to_kind)) {
+    MaybeObject* maybe_failure = UpdateAllocationSite(to_kind);
+    if (maybe_failure->IsFailure()) return maybe_failure;
+  }
 
   Isolate* isolate = GetIsolate();
   if (elements() == isolate->heap()->empty_fixed_array() ||