[v8-dev] Teach OrderedHashSet::Remove to report whether it actually removed anything (issue 290733008)

Wed, 21 May 2014 02:50:58 -0700 

Reviewers: arv, Michael Starzinger,

Description:
Teach OrderedHashSet::Remove to report whether it actually removed anything

This avoids an unnecessary runtime call from Set.prototype.delete().

Please review this at https://codereview.chromium.org/290733008/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files (+27, -16 lines):
  M src/collection.js
  M src/objects.h
  M src/objects.cc
  M src/runtime.cc
  M test/cctest/test-ordered-hash-table.cc


Index: src/collection.js
diff --git a/src/collection.js b/src/collection.js
index 94125fb89956caf6c7eb0b3f69060962c20fcb8f..e2cda595d845e0dc89a58e7e11a560f901c6821c 100644 
--- a/src/collection.js
+++ b/src/collection.js
@@ -47,12 +47,7 @@ function SetDeleteJS(key) {
     throw MakeTypeError('incompatible_method_receiver',
                         ['Set.prototype.delete', this]);
   }
-  if (%SetHas(this, key)) {
-    %SetDelete(this, key);
-    return true;
-  } else {
-    return false;
-  }
+  return %SetDelete(this, key);
 }


Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index a1ca34ead8dc83ec02d448eeb23d0953409df07a..769d5fc8b61b89e3554a45220654e3c8ea7577e0 100644 
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -16456,9 +16456,14 @@ Handle<OrderedHashSet> OrderedHashSet::Add(Handle<OrderedHashSet> table, 


 Handle<OrderedHashSet> OrderedHashSet::Remove(Handle<OrderedHashSet> table,
-                                              Handle<Object> key) {
+                                              Handle<Object> key,
+                                              bool* was_present) {
   int entry = table->FindEntry(key);
-  if (entry == kNotFound) return table;
+  if (entry == kNotFound) {
+    *was_present = false;
+    return table;
+  }
+  *was_present = true;
   table->RemoveEntry(entry);
   return Shrink(table);
 }
Index: src/objects.h
diff --git a/src/objects.h b/src/objects.h
index 80848f882bd5e38dd185a4d6d4f9bf0a474f4a67..944efb9d5e70b5c3289424db51b747469812f9c4 100644 
--- a/src/objects.h
+++ b/src/objects.h
@@ -4323,7 +4323,7 @@ class OrderedHashSet: public OrderedHashTable<
   static Handle<OrderedHashSet> Add(
       Handle<OrderedHashSet> table, Handle<Object> key);
   static Handle<OrderedHashSet> Remove(
-      Handle<OrderedHashSet> table, Handle<Object> key);
+      Handle<OrderedHashSet> table, Handle<Object> key, bool* was_present);
 };


Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index c3610fbe3ffb6df22c959dd2384d79759cc3011c..568ce3158089929004a5803de899f7588e30620e 100644 
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -1554,9 +1554,10 @@ RUNTIME_FUNCTION(Runtime_SetDelete) {
   CONVERT_ARG_HANDLE_CHECKED(JSSet, holder, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, key, 1);
   Handle<OrderedHashSet> table(OrderedHashSet::cast(holder->table()));
-  table = OrderedHashSet::Remove(table, key);
+  bool was_present = false;
+  table = OrderedHashSet::Remove(table, key, &was_present);
   holder->set_table(*table);
-  return isolate->heap()->undefined_value();
+  return isolate->heap()->ToBoolean(was_present);
 }


Index: test/cctest/test-ordered-hash-table.cc
diff --git a/test/cctest/test-ordered-hash-table.cc b/test/cctest/test-ordered-hash-table.cc index 48a457f5eec61c521ae3060a0b0168109d539737..1614e4b27e208afdbc5fdfe923f853852c7cf610 100644 
--- a/test/cctest/test-ordered-hash-table.cc
+++ b/test/cctest/test-ordered-hash-table.cc
@@ -75,10 +75,16 @@ TEST(Set) {
   ordered_set = OrderedHashSet::Add(ordered_set, obj);
   CHECK_EQ(1, ordered_set->NumberOfElements());
   CHECK(ordered_set->Contains(obj));
-  ordered_set = OrderedHashSet::Remove(ordered_set, obj);
+  bool was_present = false;
+  ordered_set = OrderedHashSet::Remove(ordered_set, obj, &was_present);
+  CHECK(was_present);
   CHECK_EQ(0, ordered_set->NumberOfElements());
   CHECK(!ordered_set->Contains(obj));

+  // Removing a not-present object should set was_present to false.
+  ordered_set = OrderedHashSet::Remove(ordered_set, obj, &was_present);
+  CHECK(!was_present);
+
   // Test for collisions/chaining
   Handle<JSObject> obj1 = factory->NewJSObjectFromMap(map);
   ordered_set = OrderedHashSet::Add(ordered_set, obj1);
@@ -133,10 +139,14 @@ TEST(Set) {
                         true);

   // Test shrinking
-  ordered_set = OrderedHashSet::Remove(ordered_set, obj);
-  ordered_set = OrderedHashSet::Remove(ordered_set, obj1);
-  ordered_set = OrderedHashSet::Remove(ordered_set, obj2);
-  ordered_set = OrderedHashSet::Remove(ordered_set, obj3);
+  ordered_set = OrderedHashSet::Remove(ordered_set, obj, &was_present);
+  CHECK(was_present);
+  ordered_set = OrderedHashSet::Remove(ordered_set, obj1, &was_present);
+  CHECK(was_present);
+  ordered_set = OrderedHashSet::Remove(ordered_set, obj2, &was_present);
+  CHECK(was_present);
+  ordered_set = OrderedHashSet::Remove(ordered_set, obj3, &was_present);
+  CHECK(was_present);
   CHECK_EQ(1, ordered_set->NumberOfElements());
   CHECK_EQ(2, ordered_set->NumberOfBuckets());
 }


--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
--- You received this message because you are subscribed to the Google Groups "v8-dev" group. 
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to