[ic] Implement TransitionArray lookup in CSA.

Drive-by cleanup: remove megamorphic stub cache lookup support from generic property
store code. This lookup is no longer necessary because
1) fast stores to existing properties get all the information from the map,
2) transitioning store targets are taken directly from the transition array,
so in both cases there's no point in doing a store handler lookup.

Bug: v8:5988
Change-Id: I95c0a08e7d1a76bb0f4475a9bd685e4e11e16a48
Reviewed-on: https://chromium-review.googlesource.com/983921
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52349}

src/code-stub-assembler.cc

@@ -6754,11 +6754,25 @@ TNode<Uint32T> CodeStubAssembler::NumberOfEntries<DescriptorArray>(
       descriptors, IntPtrConstant(DescriptorArray::kDescriptorLengthIndex)));
 }
 
+template <>
+TNode<Uint32T> CodeStubAssembler::NumberOfEntries<TransitionArray>(
+    TNode<TransitionArray> transitions) {
+  TNode<IntPtrT> length = LoadAndUntagFixedArrayBaseLength(transitions);
+  return Select<Uint32T>(
+      UintPtrLessThan(length, IntPtrConstant(TransitionArray::kFirstIndex)),
+      [=] { return Unsigned(Int32Constant(0)); },
+      [=] {
+        return Unsigned(LoadAndUntagToWord32FixedArrayElement(
+            transitions,
+            IntPtrConstant(TransitionArray::kTransitionLengthIndex)));
+      });
+}
+
 template <typename Array>
 TNode<IntPtrT> CodeStubAssembler::EntryIndexToIndex(
     TNode<Uint32T> entry_index) {
-  TNode<Int32T> descriptor_size = Int32Constant(Array::kEntrySize);
-  TNode<Word32T> index = Int32Mul(entry_index, descriptor_size);
+  TNode<Int32T> entry_size = Int32Constant(Array::kEntrySize);
+  TNode<Word32T> index = Int32Mul(entry_index, entry_size);
   return ChangeInt32ToIntPtr(index);
 }
 
@@ -6770,6 +6784,8 @@ TNode<IntPtrT> CodeStubAssembler::ToKeyIndex(TNode<Uint32T> entry_index) {
 
 template TNode<IntPtrT> CodeStubAssembler::ToKeyIndex<DescriptorArray>(
     TNode<Uint32T>);
+template TNode<IntPtrT> CodeStubAssembler::ToKeyIndex<TransitionArray>(
+    TNode<Uint32T>);
 
 template <>
 TNode<Uint32T> CodeStubAssembler::GetSortedKeyIndex<DescriptorArray>(
@@ -6795,6 +6811,8 @@ TNode<Name> CodeStubAssembler::GetKey(TNode<Array> array,
 
 template TNode<Name> CodeStubAssembler::GetKey<DescriptorArray>(
     TNode<DescriptorArray>, TNode<Uint32T>);
+template TNode<Name> CodeStubAssembler::GetKey<TransitionArray>(
+    TNode<TransitionArray>, TNode<Uint32T>);
 
 TNode<Uint32T> CodeStubAssembler::DescriptorArrayGetDetails(
     TNode<DescriptorArray> descriptors, TNode<Uint32T> descriptor_number) {
@@ -6886,6 +6904,16 @@ void CodeStubAssembler::DescriptorLookup(
                           var_name_index, if_not_found);
 }
 
+void CodeStubAssembler::TransitionLookup(
+    SloppyTNode<Name> unique_name, SloppyTNode<TransitionArray> transitions,
+    Label* if_found, TVariable<IntPtrT>* var_name_index, Label* if_not_found) {
+  Comment("TransitionArrayLookup");
+  TNode<Uint32T> number_of_valid_transitions =
+      NumberOfEntries<TransitionArray>(transitions);
+  Lookup<TransitionArray>(unique_name, transitions, number_of_valid_transitions,
+                          if_found, var_name_index, if_not_found);
+}
+
 template <typename Array>
 void CodeStubAssembler::Lookup(TNode<Name> unique_name, TNode<Array> array,
                                TNode<Uint32T> number_of_valid_entries,

src/code-stub-assembler.h

@@ -60,6 +60,7 @@ enum class PrimitiveType { kBoolean, kNumber, kString, kSymbol };
   V(StoreHandler0Map, store_handler0_map, StoreHandler0Map)              \
   V(SymbolMap, symbol_map, SymbolMap)                                    \
   V(TheHoleValue, the_hole_value, TheHole)                               \
+  V(TransitionArrayMap, transition_array_map, TransitionArrayMap)        \
   V(TrueValue, true_value, True)                                         \
   V(Tuple2Map, tuple2_map, Tuple2Map)                                    \
   V(Tuple3Map, tuple3_map, Tuple3Map)                                    \
@@ -1520,7 +1521,8 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
     const int kKeyToValueOffset =
         (ContainerType::kEntryValueIndex - ContainerType::kEntryKeyIndex) *
         kPointerSize;
-    return LoadFixedArrayElement(container, key_index, kKeyToValueOffset);
+    return UncheckedCast<Object>(
+        LoadFixedArrayElement(container, key_index, kKeyToValueOffset));
   }
 
   // Stores the details for the entry with the given key_index.
@@ -2018,6 +2020,14 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
                         TVariable<IntPtrT>* var_name_index,
                         Label* if_not_found);
 
+  // Implements TransitionArray::SearchName() - searches for first transition
+  // entry with given name (note that there could be multiple entries with
+  // the same name).
+  void TransitionLookup(SloppyTNode<Name> unique_name,
+                        SloppyTNode<TransitionArray> transitions,
+                        Label* if_found, TVariable<IntPtrT>* var_name_index,
+                        Label* if_not_found);
+
   // Implements generic search procedure like i::Search<Array>().
   template <typename Array>
   void Lookup(TNode<Name> unique_name, TNode<Array> array,

src/heap/mark-compact.cc

@@ -2683,8 +2683,7 @@ bool MarkCompactCollector::CompactTransitionArray(
   // array disappeared during GC.
   int trim = transitions->Capacity() - transition_index;
   if (trim > 0) {
-    heap_->RightTrimFixedArray(transitions,
-                               trim * TransitionArray::kTransitionSize);
+    heap_->RightTrimFixedArray(transitions, trim * TransitionArray::kEntrySize);
     transitions->SetNumberOfTransitions(transition_index);
   }
   return descriptors_owner_died;

src/ic/keyed-store-generic.cc

@@ -41,8 +41,7 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
                                Node* value, Node* context, Label* slow);
 
   void EmitGenericPropertyStore(Node* receiver, Node* receiver_map,
-                                const StoreICParameters* p, Label* slow,
-                                UseStubCache use_stub_cache = kUseStubCache);
+                                const StoreICParameters* p, Label* slow);
 
   void BranchIfPrototypesHaveNonFastElements(Node* receiver_map,
                                              Label* non_fast_elements,
@@ -611,8 +610,8 @@ void KeyedStoreGenericAssembler::LookupPropertyOnPrototypeChain(
 }
 
 void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
-    Node* receiver, Node* receiver_map, const StoreICParameters* p, Label* slow,
-    UseStubCache use_stub_cache) {
+    Node* receiver, Node* receiver_map, const StoreICParameters* p,
+    Label* slow) {
   VARIABLE(var_accessor_pair, MachineRepresentation::kTagged);
   VARIABLE(var_accessor_holder, MachineRepresentation::kTagged);
   Label stub_cache(this), fast_properties(this), dictionary_properties(this),
@@ -627,7 +626,6 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
     Node* descriptors = LoadMapDescriptors(receiver_map);
     Label descriptor_found(this), lookup_transition(this);
     TVARIABLE(IntPtrT, var_name_index);
-    Label* notfound = use_stub_cache == kUseStubCache ? &stub_cache : slow;
     DescriptorLookup(p->name, descriptors, bitfield3, &descriptor_found,
                      &var_name_index, &lookup_transition);
 
@@ -659,19 +657,57 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
     BIND(&lookup_transition);
     {
       Comment("lookup transition");
-      Label found_simple_transition_handler(this);
-      Node* maybe_handler =
+      TVARIABLE(WeakCell, var_transition_map_weak_cell);
+      Label simple_transition(this), found_handler_candidate(this);
+      TNode<Object> maybe_handler =
           LoadObjectField(receiver_map, Map::kTransitionsOrPrototypeInfoOffset);
-      GotoIf(TaggedIsSmi(maybe_handler), notfound);
-      GotoIf(IsWeakCell(maybe_handler), &found_simple_transition_handler);
+      GotoIf(TaggedIsSmi(maybe_handler), slow);
+      TNode<Map> maybe_handler_map = LoadMap(CAST(maybe_handler));
+      GotoIf(IsWeakCellMap(maybe_handler_map), &simple_transition);
+      GotoIfNot(IsTransitionArrayMap(maybe_handler_map), slow);
 
-      // TODO(jkummerow): Consider implementing TransitionArray search.
-      Goto(notfound);
+      {
+        TVARIABLE(IntPtrT, var_name_index);
+        Label if_found_candidate(this);
+        TNode<TransitionArray> transitions = CAST(maybe_handler);
+        TransitionLookup(p->name, transitions, &if_found_candidate,
+                         &var_name_index, slow);
+
+        BIND(&if_found_candidate);
+        {
+          // Given that
+          // 1) transitions with the same name are ordered in the transition
+          //    array by PropertyKind and then by PropertyAttributes values,
+          // 2) kData < kAccessor,
+          // 3) NONE == 0,
+          // 4) properties with private symbol names are guaranteed to be
+          //    non-enumerable (so DONT_ENUM bit in attributes is always set),
+          // the resulting map of transitioning store if it exists in the
+          // transition array is expected to be the first among the transitions
+          // with the same name.
+          // See TransitionArray::CompareDetails() for details.
+          STATIC_ASSERT(kData == 0);
+          STATIC_ASSERT(NONE == 0);
+          const int kKeyToTargetOffset = (TransitionArray::kEntryTargetIndex -
+                                          TransitionArray::kEntryKeyIndex) *
+                                         kPointerSize;
+          var_transition_map_weak_cell = CAST(LoadFixedArrayElement(
+              transitions, var_name_index.value(), kKeyToTargetOffset));
+          Goto(&found_handler_candidate);
+        }
+      }
+
+      BIND(&simple_transition);
+      {
+        var_transition_map_weak_cell = CAST(maybe_handler);
+        Goto(&found_handler_candidate);
+      }
 
-      BIND(&found_simple_transition_handler);
+      BIND(&found_handler_candidate);
       {
-        Node* transition_cell = maybe_handler;
-        Node* transition_map = LoadWeakCellValue(transition_cell, slow);
+        TNode<Map> transition_map =
+            CAST(LoadWeakCellValue(var_transition_map_weak_cell.value(), slow));
+        // Validate the transition handler candidate and apply the transition.
         HandleStoreICTransitionMapHandlerCase(p, transition_map, slow, true);
       }
     }
@@ -777,27 +813,6 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
                      p->name, type, p->receiver);
     }
   }
-
-  if (use_stub_cache == kUseStubCache) {
-    BIND(&stub_cache);
-    Comment("stub cache probe");
-    VARIABLE(var_handler, MachineRepresentation::kTagged);
-    Label found_handler(this, &var_handler), stub_cache_miss(this);
-    TryProbeStubCache(isolate()->store_stub_cache(), receiver, p->name,
-                      &found_handler, &var_handler, &stub_cache_miss);
-    BIND(&found_handler);
-    {
-      Comment("KeyedStoreGeneric found handler");
-      HandleStoreICHandlerCase(p, var_handler.value(), &stub_cache_miss,
-                               ICMode::kNonGlobalIC);
-    }
-    BIND(&stub_cache_miss);
-    {
-      Comment("KeyedStoreGeneric_miss");
-      TailCallRuntime(Runtime::kKeyedStoreIC_Miss, p->context, p->value,
-                      p->slot, p->vector, p->receiver, p->name);
-    }
-  }
 }
 
 void KeyedStoreGenericAssembler::KeyedStoreGeneric() {
@@ -893,8 +908,7 @@ void KeyedStoreGenericAssembler::StoreIC_Uninitialized() {
                           SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
 
   StoreICParameters p(context, receiver, name, value, slot, vector);
-  EmitGenericPropertyStore(receiver, receiver_map, &p, &miss,
-                           kDontUseStubCache);
+  EmitGenericPropertyStore(receiver, receiver_map, &p, &miss);
 
   BIND(&miss);
   {

src/objects-debug.cc

@@ -595,15 +595,29 @@ void FeedbackMetadata::FeedbackMetadataVerify() {
 
 void DescriptorArray::DescriptorArrayVerify() {
   FixedArrayVerify();
+  int nof_descriptors = number_of_descriptors();
   if (number_of_descriptors_storage() == 0) {
     Heap* heap = GetHeap();
     CHECK_EQ(heap->empty_descriptor_array(), this);
     CHECK_EQ(2, length());
-    CHECK_EQ(0, number_of_descriptors());
+    CHECK_EQ(0, nof_descriptors);
     CHECK_EQ(heap->empty_enum_cache(), GetEnumCache());
   } else {
     CHECK_LT(2, length());
-    CHECK_LE(LengthFor(number_of_descriptors()), length());
+    CHECK_LE(LengthFor(nof_descriptors), length());
+
+    Isolate* isolate = GetIsolate();
+    // Check that properties with private symbols names are non-enumerable.
+    for (int descriptor = 0; descriptor < nof_descriptors; descriptor++) {
+      Object* key = get(ToKeyIndex(descriptor));
+      // number_of_descriptors() may be out of sync with the actual descriptors
+      // written during descriptor array construction.
+      if (key->IsUndefined(isolate)) continue;
+      if (Name::cast(key)->IsPrivate()) {
+        PropertyDetails details = GetDetails(descriptor);
+        CHECK_NE(details.attributes() & DONT_ENUM, 0);
+      }
+    }
   }
 }
 

src/transitions-inl.h

@@ -202,7 +202,7 @@ void TransitionArray::Set(int transition_number, Name* key, Object* target) {
 
 int TransitionArray::Capacity() {
   if (length() <= kFirstIndex) return 0;
-  return (length() - kFirstIndex) / kTransitionSize;
+  return (length() - kFirstIndex) / kEntrySize;
 }
 
 void TransitionArray::SetNumberOfTransitions(int number_of_transitions) {

src/transitions.h

@@ -247,12 +247,34 @@ class TransitionArray : public FixedArray {
   DECL_PRINTER(TransitionArray)
   DECL_VERIFIER(TransitionArray)
 
+  // Layout for full transition arrays.
+  static const int kPrototypeTransitionsIndex = 0;
+  static const int kTransitionLengthIndex = 1;
+  static const int kFirstIndex = 2;
+
+  // Layout of map transition entries in full transition arrays.
+  static const int kEntryKeyIndex = 0;
+  static const int kEntryTargetIndex = 1;
+  static const int kEntrySize = 2;
+
+  // Conversion from transition number to array indices.
+  static int ToKeyIndex(int transition_number) {
+    return kFirstIndex + (transition_number * kEntrySize) + kEntryKeyIndex;
+  }
+
+  static int ToTargetIndex(int transition_number) {
+    return kFirstIndex + (transition_number * kEntrySize) + kEntryTargetIndex;
+  }
+
+  inline int SearchNameForTesting(Name* name,
+                                  int* out_insertion_index = nullptr) {
+    return SearchName(name, out_insertion_index);
+  }
+
  private:
   friend class MarkCompactCollector;
   friend class TransitionsAccessor;
 
-  static const int kTransitionSize = 2;
-
   inline void SetNumberOfTransitions(int number_of_transitions);
 
   inline int Capacity();
@@ -274,32 +296,9 @@ class TransitionArray : public FixedArray {
   static void SetNumberOfPrototypeTransitions(FixedArray* proto_transitions,
                                               int value);
 
-  // Layout for full transition arrays.
-  static const int kPrototypeTransitionsIndex = 0;
-  static const int kTransitionLengthIndex = 1;
-  static const int kFirstIndex = 2;
-
-  // Layout of map transition entries in full transition arrays.
-  static const int kTransitionKey = 0;
-  static const int kTransitionTarget = 1;
-  STATIC_ASSERT(kTransitionSize == 2);
-
   static const int kProtoTransitionNumberOfEntriesOffset = 0;
   STATIC_ASSERT(kProtoTransitionHeaderSize == 1);
 
-  // Conversion from transition number to array indices.
-  static int ToKeyIndex(int transition_number) {
-    return kFirstIndex +
-           (transition_number * kTransitionSize) +
-           kTransitionKey;
-  }
-
-  static int ToTargetIndex(int transition_number) {
-    return kFirstIndex +
-           (transition_number * kTransitionSize) +
-           kTransitionTarget;
-  }
-
   // Returns the fixed array length required to hold number_of_transitions
   // transitions.
   static int LengthFor(int number_of_transitions) {

test/cctest/test-code-stub-assembler.cc

@@ -28,6 +28,18 @@ namespace {
 typedef CodeAssemblerLabel Label;
 typedef CodeAssemblerVariable Variable;
 
+Handle<String> MakeString(const char* str) {
+  Isolate* isolate = CcTest::i_isolate();
+  Factory* factory = isolate->factory();
+  return factory->InternalizeUtf8String(str);
+}
+
+Handle<String> MakeName(const char* str, int suffix) {
+  EmbeddedVector<char, 128> buffer;
+  SNPrintF(buffer, "%s%d", str, suffix);
+  return MakeString(buffer.start());
+}
+
 int sum9(int a0, int a1, int a2, int a3, int a4, int a5, int a6, int a7,
          int a8) {
   return a0 + a1 + a2 + a3 + a4 + a5 + a6 + a7 + a8;
@@ -743,6 +755,136 @@ TEST(NumberDictionaryLookup) {
   }
 }
 
+TEST(TransitionLookup) {
+  Isolate* isolate(CcTest::InitIsolateOnce());
+
+  const int kNumParams = 4;
+  CodeAssemblerTester asm_tester(isolate, kNumParams);
+
+  enum Result { kFound, kNotFound };
+
+  class TempAssembler : public CodeStubAssembler {
+   public:
+    explicit TempAssembler(compiler::CodeAssemblerState* state)
+        : CodeStubAssembler(state) {}
+
+    void Generate() {
+      TNode<TransitionArray> transitions = CAST(Parameter(0));
+      TNode<Name> name = CAST(Parameter(1));
+      TNode<Smi> expected_result = CAST(Parameter(2));
+      TNode<Object> expected_arg = CAST(Parameter(3));
+
+      Label passed(this), failed(this);
+      Label if_found(this), if_not_found(this);
+      TVARIABLE(IntPtrT, var_transition_index);
+
+      TransitionLookup(name, transitions, &if_found, &var_transition_index,
+                       &if_not_found);
+
+      BIND(&if_found);
+      GotoIfNot(WordEqual(expected_result, SmiConstant(kFound)), &failed);
+      Branch(WordEqual(expected_arg, SmiTag(var_transition_index.value())),
+             &passed, &failed);
+
+      BIND(&if_not_found);
+      Branch(WordEqual(expected_result, SmiConstant(kNotFound)), &passed,
+             &failed);
+
+      BIND(&passed);
+      Return(BooleanConstant(true));
+
+      BIND(&failed);
+      Return(BooleanConstant(false));
+    }
+  };
+  TempAssembler(asm_tester.state()).Generate();
+
+  FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
+
+  Handle<Object> expect_found(Smi::FromInt(kFound), isolate);
+  Handle<Object> expect_not_found(Smi::FromInt(kNotFound), isolate);
+
+  const int ATTRS_COUNT = (READ_ONLY | DONT_ENUM | DONT_DELETE) + 1;
+  STATIC_ASSERT(ATTRS_COUNT == 8);
+
+  const int kKeysCount = 300;
+  Handle<Map> root_map = Map::Create(isolate, 0);
+  Handle<Name> keys[kKeysCount];
+
+  base::RandomNumberGenerator rand_gen(FLAG_random_seed);
+
+  Factory* factory = isolate->factory();
+  Handle<FieldType> any = FieldType::Any(isolate);
+
+  for (int i = 0; i < kKeysCount; i++) {
+    Handle<Name> name;
+    if (i % 30 == 0) {
+      name = factory->NewPrivateSymbol();
+    } else if (i % 10 == 0) {
+      name = factory->NewSymbol();
+    } else {
+      int random_key = rand_gen.NextInt(Smi::kMaxValue);
+      name = MakeName("p", random_key);
+    }
+    keys[i] = name;
+
+    bool is_private = name->IsPrivate();
+    PropertyAttributes base_attributes = is_private ? DONT_ENUM : NONE;
+
+    // Ensure that all the combinations of cases are covered:
+    // 1) there is a "base" attributes transition
+    // 2) there are other non-base attributes transitions
+    if ((i & 1) == 0) {
+      CHECK(!Map::CopyWithField(root_map, name, any, base_attributes, kMutable,
+                                Representation::Tagged(), INSERT_TRANSITION)
+                 .is_null());
+    }
+
+    if ((i & 2) == 0) {
+      for (int j = 0; j < ATTRS_COUNT; j++) {
+        PropertyAttributes attributes = static_cast<PropertyAttributes>(j);
+        if (attributes == base_attributes) continue;
+        // Don't add private symbols with enumerable attributes.
+        if (is_private && ((attributes & DONT_ENUM) == 0)) continue;
+        CHECK(!Map::CopyWithField(root_map, name, any, attributes, kMutable,
+                                  Representation::Tagged(), INSERT_TRANSITION)
+                   .is_null());
+      }
+    }
+  }
+
+  CHECK(root_map->raw_transitions()->IsTransitionArray());
+  Handle<TransitionArray> transitions(
+      TransitionArray::cast(root_map->raw_transitions()));
+  DCHECK(transitions->IsSortedNoDuplicates());
+
+  // Ensure we didn't overflow transition array and therefore all the
+  // combinations of cases are covered.
+  CHECK(TransitionsAccessor(root_map).CanHaveMoreTransitions());
+
+  // Now try querying keys.
+  bool positive_lookup_tested = false;
+  bool negative_lookup_tested = false;
+  for (int i = 0; i < kKeysCount; i++) {
+    Handle<Name> name = keys[i];
+
+    int transition_number = transitions->SearchNameForTesting(*name);
+
+    if (transition_number != TransitionArray::kNotFound) {
+      Handle<Smi> expected_value(
+          Smi::FromInt(TransitionArray::ToKeyIndex(transition_number)),
+          isolate);
+      ft.CheckTrue(transitions, name, expect_found, expected_value);
+      positive_lookup_tested = true;
+    } else {
+      ft.CheckTrue(transitions, name, expect_not_found);
+      negative_lookup_tested = true;
+    }
+  }
+  CHECK(positive_lookup_tested);
+  CHECK(negative_lookup_tested);
+}
+
 namespace {
 
 void AddProperties(Handle<JSObject> object, Handle<Name> names[],