Split Put into Put and Remove

No longer treat the hole as a removal. This removes one branch in
Put and cleans up the API.

BUG=None
LOG=Y
R=adamk@chromium.org, mstarzinger@chromium.org

Review URL: https://codereview.chromium.org/309663005

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21616 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/objects.cc

@@ -5214,7 +5214,8 @@ void JSObject::DeleteHiddenProperty(Handle<JSObject> object, Handle<Name> key) {
   if (inline_value->IsUndefined() || inline_value->IsSmi()) return;
 
   Handle<ObjectHashTable> hashtable(ObjectHashTable::cast(inline_value));
-  ObjectHashTable::Put(hashtable, key, isolate->factory()->the_hole_value());
+  bool was_present = false;
+  ObjectHashTable::Remove(hashtable, key, &was_present);
 }
 
 
@@ -16039,6 +16040,7 @@ Handle<ObjectHashTable> ObjectHashTable::Put(Handle<ObjectHashTable> table,
                                              Handle<Object> key,
                                              Handle<Object> value) {
   ASSERT(table->IsKey(*key));
+  ASSERT(!value->IsTheHole());
 
   Isolate* isolate = table->GetIsolate();
 
@@ -16047,13 +16049,6 @@ Handle<ObjectHashTable> ObjectHashTable::Put(Handle<ObjectHashTable> table,
 
   int entry = table->FindEntry(key);
 
-  // Check whether to perform removal operation.
-  if (value->IsTheHole()) {
-    if (entry == kNotFound) return table;
-    table->RemoveEntry(entry);
-    return Shrink(table, key);
-  }
-
   // Key is already in table, just overwrite value.
   if (entry != kNotFound) {
     table->set(EntryToIndex(entry) + 1, *value);
@@ -16069,6 +16064,29 @@ Handle<ObjectHashTable> ObjectHashTable::Put(Handle<ObjectHashTable> table,
 }
 
 
+Handle<ObjectHashTable> ObjectHashTable::Remove(Handle<ObjectHashTable> table,
+                                                Handle<Object> key,
+                                                bool* was_present) {
+  ASSERT(table->IsKey(*key));
+
+  Object* hash = key->GetHash();
+  if (hash->IsUndefined()) {
+    *was_present = false;
+    return table;
+  }
+
+  int entry = table->FindEntry(key);
+  if (entry == kNotFound) {
+    *was_present = false;
+    return table;
+  }
+
+  *was_present = true;
+  table->RemoveEntry(entry);
+  return Shrink(table, key);
+}
+
+
 void ObjectHashTable::AddEntry(int entry, Object* key, Object* value) {
   set(EntryToIndex(entry), key);
   set(EntryToIndex(entry) + 1, value);
@@ -16200,6 +16218,20 @@ Handle<Derived> OrderedHashTable<Derived, Iterator, entrysize>::Clear(
 }
 
 
+template<class Derived, class Iterator, int entrysize>
+Handle<Derived> OrderedHashTable<Derived, Iterator, entrysize>::Remove(
+    Handle<Derived> table, Handle<Object> key, bool* was_present) {
+  int entry = table->FindEntry(key);
+  if (entry == kNotFound) {
+    *was_present = false;
+    return table;
+  }
+  *was_present = true;
+  table->RemoveEntry(entry);
+  return Shrink(table);
+}
+
+
 template<class Derived, class Iterator, int entrysize>
 Handle<Derived> OrderedHashTable<Derived, Iterator, entrysize>::Rehash(
     Handle<Derived> table, int new_capacity) {
@@ -16309,6 +16341,10 @@ template Handle<OrderedHashSet>
 OrderedHashTable<OrderedHashSet, JSSetIterator, 1>::Clear(
     Handle<OrderedHashSet> table);
 
+template Handle<OrderedHashSet>
+OrderedHashTable<OrderedHashSet, JSSetIterator, 1>::Remove(
+    Handle<OrderedHashSet> table, Handle<Object> key, bool* was_present);
+
 template int
 OrderedHashTable<OrderedHashSet, JSSetIterator, 1>::FindEntry(
     Handle<Object> key);
@@ -16336,6 +16372,10 @@ template Handle<OrderedHashMap>
 OrderedHashTable<OrderedHashMap, JSMapIterator, 2>::Clear(
     Handle<OrderedHashMap> table);
 
+template Handle<OrderedHashMap>
+OrderedHashTable<OrderedHashMap, JSMapIterator, 2>::Remove(
+    Handle<OrderedHashMap> table, Handle<Object> key, bool* was_present);
+
 template int
 OrderedHashTable<OrderedHashMap, JSMapIterator, 2>::FindEntry(
     Handle<Object> key);
@@ -16365,20 +16405,6 @@ Handle<OrderedHashSet> OrderedHashSet::Add(Handle<OrderedHashSet> table,
 }
 
 
-Handle<OrderedHashSet> OrderedHashSet::Remove(Handle<OrderedHashSet> table,
-                                              Handle<Object> key,
-                                              bool* was_present) {
-  int entry = table->FindEntry(key);
-  if (entry == kNotFound) {
-    *was_present = false;
-    return table;
-  }
-  *was_present = true;
-  table->RemoveEntry(entry);
-  return Shrink(table);
-}
-
-
 Object* OrderedHashMap::Lookup(Handle<Object> key) {
   DisallowHeapAllocation no_gc;
   int entry = FindEntry(key);
@@ -16390,13 +16416,9 @@ Object* OrderedHashMap::Lookup(Handle<Object> key) {
 Handle<OrderedHashMap> OrderedHashMap::Put(Handle<OrderedHashMap> table,
                                            Handle<Object> key,
                                            Handle<Object> value) {
-  int entry = table->FindEntry(key);
+  ASSERT(!key->IsTheHole());
 
-  if (value->IsTheHole()) {
-    if (entry == kNotFound) return table;
-    table->RemoveEntry(entry);
-    return Shrink(table);
-  }
+  int entry = table->FindEntry(key);
 
   if (entry != kNotFound) {
     table->set(table->EntryToIndex(entry) + kValueOffset, *value);

src/objects.h

@@ -4133,12 +4133,16 @@ class ObjectHashTable: public HashTable<ObjectHashTable,
   // returned in case the key is not present.
   Object* Lookup(Handle<Object> key);
 
-  // Adds (or overwrites) the value associated with the given key. Mapping a
-  // key to the hole value causes removal of the whole entry.
+  // Adds (or overwrites) the value associated with the given key.
   static Handle<ObjectHashTable> Put(Handle<ObjectHashTable> table,
                                      Handle<Object> key,
                                      Handle<Object> value);
 
+  // Returns an ObjectHashTable (possibly |table|) where |key| has been removed.
+  static Handle<ObjectHashTable> Remove(Handle<ObjectHashTable> table,
+                                        Handle<Object> key,
+                                        bool* was_present);
+
  private:
   friend class MarkCompactCollector;
 
@@ -4203,10 +4207,15 @@ class OrderedHashTable: public FixedArray {
   // if possible.
   static Handle<Derived> Shrink(Handle<Derived> table);
 
-  // Returns a new empty OrderedHashTable and updates all the iterators to
-  // point to the new table.
+  // Returns a new empty OrderedHashTable and records the clearing so that
+  // exisiting iterators can be updated.
   static Handle<Derived> Clear(Handle<Derived> table);
 
+  // Returns an OrderedHashTable (possibly |table|) where |key| has been
+  // removed.
+  static Handle<Derived> Remove(Handle<Derived> table, Handle<Object> key,
+      bool* was_present);
+
   // Returns kNotFound if the key isn't present.
   int FindEntry(Handle<Object> key);
 
@@ -4330,8 +4339,6 @@ class OrderedHashSet: public OrderedHashTable<
   bool Contains(Handle<Object> key);
   static Handle<OrderedHashSet> Add(
       Handle<OrderedHashSet> table, Handle<Object> key);
-  static Handle<OrderedHashSet> Remove(
-      Handle<OrderedHashSet> table, Handle<Object> key, bool* was_present);
 };
 
 

src/runtime.cc

@@ -1646,11 +1646,11 @@ RUNTIME_FUNCTION(Runtime_MapDelete) {
   CONVERT_ARG_HANDLE_CHECKED(JSMap, holder, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
   Handle<OrderedHashMap> table(OrderedHashMap::cast(holder->table()));
-  Handle<Object> lookup(table->Lookup(key), isolate);
+  bool was_present = false;
   Handle<OrderedHashMap> new_table =
-      OrderedHashMap::Put(table, key, isolate->factory()->the_hole_value());
+      OrderedHashMap::Remove(table, key, &was_present);
   holder->set_table(*new_table);
-  return isolate->heap()->ToBoolean(!lookup->IsTheHole());
+  return isolate->heap()->ToBoolean(was_present);
 }
 
 
@@ -1767,11 +1767,11 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) {
   Handle<ObjectHashTable> table(ObjectHashTable::cast(
       weak_collection->table()));
   RUNTIME_ASSERT(table->IsKey(*key));
-  Handle<Object> lookup(table->Lookup(key), isolate);
+  bool was_present = false;
   Handle<ObjectHashTable> new_table =
-      ObjectHashTable::Put(table, key, isolate->factory()->the_hole_value());
+      ObjectHashTable::Remove(table, key, &was_present);
   weak_collection->set_table(*new_table);
-  return isolate->heap()->ToBoolean(!lookup->IsTheHole());
+  return isolate->heap()->ToBoolean(was_present);
 }
 
 

test/cctest/test-dictionary.cc

@@ -51,6 +51,7 @@ static void TestHashMap(Handle<HashMap> table) {
   table = HashMap::Put(table, a, b);
   CHECK_EQ(table->NumberOfElements(), 1);
   CHECK_EQ(table->Lookup(a), *b);
+  // When the key does not exist in the map, Lookup returns the hole.
   CHECK_EQ(table->Lookup(b), CcTest::heap()->the_hole_value());
 
   // Keys still have to be valid after objects were moved.
@@ -64,8 +65,10 @@ static void TestHashMap(Handle<HashMap> table) {
   CHECK_EQ(table->NumberOfElements(), 1);
   CHECK_NE(table->Lookup(a), *b);
 
-  // Keys mapped to the hole should be removed permanently.
-  table = HashMap::Put(table, a, factory->the_hole_value());
+  // Keys that have been removed are mapped to the hole.
+  bool was_present = false;
+  table = HashMap::Remove(table, a, &was_present);
+  CHECK(was_present);
   CHECK_EQ(table->NumberOfElements(), 0);
   CHECK_EQ(table->Lookup(a), CcTest::heap()->the_hole_value());
 

test/cctest/test-ordered-hash-table.cc

@@ -123,8 +123,9 @@ TEST(Map) {
   ordered_map = OrderedHashMap::Put(ordered_map, obj, val);
   CHECK_EQ(1, ordered_map->NumberOfElements());
   CHECK(ordered_map->Lookup(obj)->SameValue(*val));
-  ordered_map = OrderedHashMap::Put(
-      ordered_map, obj, factory->the_hole_value());
+  bool was_present = false;
+  ordered_map = OrderedHashMap::Remove(ordered_map, obj, &was_present);
+  CHECK(was_present);
   CHECK_EQ(0, ordered_map->NumberOfElements());
   CHECK(ordered_map->Lookup(obj)->IsTheHole());
 
@@ -157,14 +158,14 @@ TEST(Map) {
   CHECK_EQ(4, ordered_map->NumberOfBuckets());
 
   // Test shrinking
-  ordered_map = OrderedHashMap::Put(
-      ordered_map, obj, factory->the_hole_value());
-  ordered_map = OrderedHashMap::Put(
-      ordered_map, obj1, factory->the_hole_value());
-  ordered_map = OrderedHashMap::Put(
-      ordered_map, obj2, factory->the_hole_value());
-  ordered_map = OrderedHashMap::Put(
-      ordered_map, obj3, factory->the_hole_value());
+  ordered_map = OrderedHashMap::Remove(ordered_map, obj, &was_present);
+  CHECK(was_present);
+  ordered_map = OrderedHashMap::Remove(ordered_map, obj1, &was_present);
+  CHECK(was_present);
+  ordered_map = OrderedHashMap::Remove(ordered_map, obj2, &was_present);
+  CHECK(was_present);
+  ordered_map = OrderedHashMap::Remove(ordered_map, obj3, &was_present);
+  CHECK(was_present);
   CHECK_EQ(1, ordered_map->NumberOfElements());
   CHECK_EQ(2, ordered_map->NumberOfBuckets());
 }