[runtime] Support Symbols in KeyAccumulator

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 7422aec4e6603ddbbf52a3295a9fea13e8da9b6b..167a053051706cbac6e261343b6a9dc0a675b22c 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -7566,19 +7566,23 @@ Handle<FixedArray> KeyAccumulator::GetKeys(GetKeysConversion convert) {
   // in |elements_| to deduce the number of keys to take from the |properties_|
   // set.
   Handle<FixedArray> result = isolate_->factory()->NewFixedArray(length_);
-  int index = 0;
-  int properties_index = 0;
-  for (size_t level = 0; level < levelLengths_.size(); level++) {
-    int num_total = levelLengths_[level];
-    int num_elements = 0;
-    if (num_total < 0) {
-      // If the total is negative, the current level contains properties from a
-      // proxy, hence we skip the integer keys in |elements_| since proxies
-      // define the complete ordering.
-      num_total = -num_total;
+  int insertion_index = 0;
+  int string_properties_index = 0;
+  int symbol_properties_index = 0;
+  // String and Symbol lengths always come in pairs:
+  size_t max_level = level_lengths_.size() / 2;
+  for (size_t level = 0; level < max_level; level++) {
+    int num_string_properties = level_lengths_[level * 2];
+    int num_symbol_properties = level_lengths_[level * 2 + 1];
+    if (num_string_properties < 0) {
+      // If the num_string_properties is negative, the current level contains
+      // properties from a proxy, hence we skip the integer keys in |elements_|
+      // since proxies define the complete ordering.
+      num_string_properties = -num_string_properties;
     } else if (level < elements_.size()) {
+      // Add the element indices for this prototype level.
       std::vector<uint32_t>* elements = elements_[level];
-      num_elements = static_cast<int>(elements->size());
+      int num_elements = static_cast<int>(elements->size());
       for (int i = 0; i < num_elements; i++) {
         Handle<Object> key;
         if (convert == KEEP_NUMBERS) {
@@ -7586,25 +7590,34 @@ Handle<FixedArray> KeyAccumulator::GetKeys(GetKeysConversion convert) {
         } else {
           key = isolate_->factory()->Uint32ToString(elements->at(i));
         }
-        result->set(index, *key);
-        index++;
+        result->set(insertion_index, *key);
+        insertion_index++;
       }
     }
-    // Add the property keys for this prototype level.
-    int num_properties = num_total - num_elements;
-    for (int i = 0; i < num_properties; i++) {
-      Object* key = properties_->KeyAt(properties_index);
-      result->set(index, key);
-      index++;
-      properties_index++;
+    // Add the string property keys for this prototype level.
+    for (int i = 0; i < num_string_properties; i++) {
+      Object* key = string_properties_->KeyAt(string_properties_index);
+      result->set(insertion_index, key);
+      insertion_index++;
+      string_properties_index++;
+    }
+    // Add the symbol property keys for this prototype level.
+    for (int i = 0; i < num_symbol_properties; i++) {
+      Object* key = symbol_properties_->KeyAt(symbol_properties_index);
+      result->set(insertion_index, key);
+      insertion_index++;
+      symbol_properties_index++;
     }
   }
 
-  DCHECK_EQ(index, length_);
+  DCHECK_EQ(insertion_index, length_);
   return result;
 }
 
 
+int KeyAccumulator::GetLength() { return length_; }

[Jakob Kummerow, 2015/11/03 15:33:37]

nit: a simple getter like this should be called just "length()" and be defined
inline in the header file (unless you have a reason to deviate from that rule of
thumb).

+
+
 namespace {
 
 bool AccumulatorHasKey(std::vector<uint32_t>* sub_elements, uint32_t key) {
@@ -7617,7 +7630,7 @@ bool AccumulatorHasKey(std::vector<uint32_t>* sub_elements, uint32_t key) {
 bool KeyAccumulator::AddKey(uint32_t key) {
   // Make sure we do not add keys to a proxy-level (see AddKeysFromProxy).
   // We mark proxy-levels with a negative length
-  DCHECK_LE(0, levelLength_);
+  DCHECK_LE(0, level_string_length_);
   // Binary search over all but the last level. The last one might not be
   // sorted yet.
   for (size_t i = 1; i < elements_.size(); i++) {
@@ -7625,7 +7638,6 @@ bool KeyAccumulator::AddKey(uint32_t key) {
   }
   elements_.back()->push_back(key);
   length_++;
-  levelLength_++;
   return true;
 }
 
@@ -7636,11 +7648,12 @@ bool KeyAccumulator::AddKey(Object* key, AddKeyConversion convert) {
 
 
 bool KeyAccumulator::AddKey(Handle<Object> key, AddKeyConversion convert) {
-  if (filter_ == SKIP_SYMBOLS && key->IsSymbol()) {
-    return false;
+  if (key->IsSymbol()) {
+    if (filter_ == SKIP_SYMBOLS) return false;
+    return AddSymbolKey(key);
   }
   // Make sure we do not add keys to a proxy-level (see AddKeysFromProxy).
-  DCHECK_LE(0, levelLength_);
+  DCHECK_LE(0, level_string_length_);
   // In some cases (e.g. proxies) we might get in String-converted ints which
   // should be added to the elements list instead of the properties. For
   // proxies we have to convert as well but also respect the original order.
@@ -7648,7 +7661,7 @@ bool KeyAccumulator::AddKey(Handle<Object> key, AddKeyConversion convert) {
   if (convert == CONVERT_TO_ARRAY_INDEX || convert == PROXY_MAGIC) {
     uint32_t index = 0;
     int prev_length = length_;
-    int prev_proto = levelLength_;
+    int prev_proto = level_string_length_;
     bool was_array_index = false;
     bool key_was_added = false;
     if ((key->IsString() && Handle<String>::cast(key)->AsArrayIndex(&index)) ||
@@ -7665,24 +7678,48 @@ bool KeyAccumulator::AddKey(Handle<Object> key, AddKeyConversion convert) {
         return false;
       }
       length_ = prev_length;
-      levelLength_ = prev_proto;
+      level_string_length_ = prev_proto;
     }
   }
-  if (properties_.is_null()) {
-    properties_ = OrderedHashSet::Allocate(isolate_, 16);
+  return AddStringKey(key, convert);
+}
+
+
+bool KeyAccumulator::AddStringKey(Handle<Object> key,
+                                  AddKeyConversion convert) {
+  if (string_properties_.is_null()) {
+    string_properties_ = OrderedHashSet::Allocate(isolate_, 16);
   }
   // TODO(cbruni): remove this conversion once we throw the correct TypeError
   // for non-string/symbol elements returned by proxies
   if (convert == PROXY_MAGIC && key->IsNumber()) {
     key = isolate_->factory()->NumberToString(key);
   }
-  int prev_size = properties_->NumberOfElements();
-  properties_ = OrderedHashSet::Add(properties_, key);
-  if (prev_size < properties_->NumberOfElements()) {
+  int prev_size = string_properties_->NumberOfElements();
+  string_properties_ = OrderedHashSet::Add(string_properties_, key);
+  if (prev_size < string_properties_->NumberOfElements()) {
+    length_++;
+    level_string_length_++;
+    return true;
+  } else {
+    return false;
+  }
+}
+
+
+bool KeyAccumulator::AddSymbolKey(Handle<Object> key) {
+  if (symbol_properties_.is_null()) {
+    symbol_properties_ = OrderedHashSet::Allocate(isolate_, 16);
+  }
+  int prev_size = symbol_properties_->NumberOfElements();
+  symbol_properties_ = OrderedHashSet::Add(symbol_properties_, key);
+  if (prev_size < symbol_properties_->NumberOfElements()) {
     length_++;
-    levelLength_++;
+    level_symbol_length_++;
+    return true;
+  } else {
+    return false;
   }
-  return true;
 }
 
 
@@ -7692,7 +7729,7 @@ void KeyAccumulator::AddKeys(Handle<FixedArray> array,
   if (add_length == 0) return;
   for (int i = 0; i < add_length; i++) {
     Handle<Object> current(array->get(i), isolate_);
-    AddKey(current);
+    AddKey(current, convert);
   }
 }
 
@@ -7713,7 +7750,7 @@ void KeyAccumulator::AddKeysFromProxy(Handle<JSObject> array_like) {
   // Invert the current length to indicate a present proxy, so we can ignore
   // element keys for this level. Otherwise we would not fully respect the order
   // given by the proxy.
-  levelLength_ = -levelLength_;
+  level_string_length_ = -level_string_length_;
 }
 
 
@@ -7727,16 +7764,15 @@ void KeyAccumulator::AddElementKeysFromInterceptor(
 
 
 void KeyAccumulator::SortCurrentElementsListRemoveDuplicates() {
-  // Sort and remove duplciated from the current elements level and adjust
+  // Sort and remove duplicated from the current elements level and adjust.

[Jakob Kummerow, 2015/11/03 15:33:37]

nit: s/.//, also s/duplicated/duplicate entries/?

   // the lengths accordingly.
   auto last_level = elements_.back();
   size_t nof_removed_keys = last_level->size();
   std::sort(last_level->begin(), last_level->end());
   last_level->erase(std::unique(last_level->begin(), last_level->end()),
                     last_level->end());
-  // Adjust total length / level length by the number of removed duplicates
+  // Adjust total length by the number of removed duplicates.
   nof_removed_keys -= last_level->size();
-  levelLength_ -= static_cast<int>(nof_removed_keys);
   length_ -= static_cast<int>(nof_removed_keys);
 }
 
@@ -7751,10 +7787,12 @@ void KeyAccumulator::SortCurrentElementsList() {
 void KeyAccumulator::NextPrototype() {
   // Store the protoLength on the first call of this method.
   if (!elements_.empty()) {
-    levelLengths_.push_back(levelLength_);
+    level_lengths_.push_back(level_string_length_);
+    level_lengths_.push_back(level_symbol_length_);
   }
   elements_.push_back(new std::vector<uint32_t>());
-  levelLength_ = 0;
+  level_string_length_ = 0;
+  level_symbol_length_ = 0;
 }
 
 
@@ -7840,10 +7878,7 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
       DCHECK(filter == INCLUDE_SYMBOLS);
       PropertyAttributes attr_filter =
           static_cast<PropertyAttributes>(DONT_ENUM | PRIVATE_SYMBOL);
-      Handle<FixedArray> property_keys = isolate->factory()->NewFixedArray(
-          current->NumberOfOwnProperties(attr_filter));
-      current->GetOwnPropertyNames(*property_keys, 0, attr_filter);
-      accumulator.AddKeys(property_keys);
+      current->CollectOwnPropertyNames(&accumulator, attr_filter);
     }
 
     // Add the property keys from the interceptor.
@@ -15020,6 +15055,27 @@ int JSObject::GetOwnPropertyNames(FixedArray* storage, int index,
 }
 
 
+int JSObject::CollectOwnPropertyNames(KeyAccumulator* keys,
+                                      PropertyAttributes filter) {
+  if (HasFastProperties()) {
+    int nof_keys = keys->GetLength();
+    int real_size = map()->NumberOfOwnDescriptors();
+    Handle<DescriptorArray> descs(map()->instance_descriptors());
+    for (int i = 0; i < real_size; i++) {
+      if ((descs->GetDetails(i).attributes() & filter) != 0) continue;
+      Name* key = descs->GetKey(i);
+      if (key->FilterKey(filter)) continue;
+      keys->AddKey(key);
+    }
+    return nof_keys - keys->GetLength();
+  } else if (IsJSGlobalObject()) {
+    return global_dictionary()->CollectKeysTo(keys, filter);
+  } else {
+    return property_dictionary()->CollectKeysTo(keys, filter);
+  }
+}
+
+
 int JSObject::NumberOfOwnElements(PropertyAttributes filter) {
   // Fast case for objects with no elements.
   if (!IsJSValue() && HasFastElements()) {
@@ -16791,12 +16847,12 @@ int Dictionary<Derived, Shape, Key>::CopyKeysTo(
   int capacity = this->Capacity();
   for (int i = 0; i < capacity; i++) {
     Object* k = this->KeyAt(i);
-    if (this->IsKey(k) && !k->FilterKey(filter)) {
-      if (this->IsDeleted(i)) continue;
-      PropertyDetails details = this->DetailsAt(i);
-      PropertyAttributes attr = details.attributes();
-      if ((attr & filter) == 0) storage->set(index++, k);
-    }
+    if (!this->IsKey(k) || k->FilterKey(filter)) continue;
+    if (this->IsDeleted(i)) continue;
+    PropertyDetails details = this->DetailsAt(i);
+    PropertyAttributes attr = details.attributes();
+    if ((attr & filter) != 0) continue;
+    storage->set(index++, k);
   }
   if (sort_mode == Dictionary::SORTED) {
     storage->SortPairs(storage, index);
@@ -16806,6 +16862,24 @@ int Dictionary<Derived, Shape, Key>::CopyKeysTo(
 }
 
 
+template <typename Derived, typename Shape, typename Key>
+int Dictionary<Derived, Shape, Key>::CollectKeysTo(KeyAccumulator* keys,
+                                                   PropertyAttributes filter) {
+  int capacity = this->Capacity();
+  int keyLength = keys->GetLength();
+  for (int i = 0; i < capacity; i++) {
+    Object* k = this->KeyAt(i);
+    if (!this->IsKey(k) || k->FilterKey(filter)) continue;
+    if (this->IsDeleted(i)) continue;
+    PropertyDetails details = this->DetailsAt(i);
+    PropertyAttributes attr = details.attributes();
+    if ((attr & filter) != 0) continue;
+    keys->AddKey(k);
+  }
+  return keyLength - keys->GetLength();
+}
+
+
 // Backwards lookup (slow).
 template<typename Derived, typename Shape, typename Key>
 Object* Dictionary<Derived, Shape, Key>::SlowReverseLookup(Object* value) {