8215155: Remove get_insert() from concurrent hashtable and gtests
authorgziemski
Thu, 10 Jan 2019 11:16:17 -0600
changeset 53239 c024fcb88ede
parent 53238 38716f9d2239
child 53240 f6ab4cc4c70e
8215155: Remove get_insert() from concurrent hashtable and gtests Summary: Replaced get_insert() with get()/insert() in gtest, removed get_insert() API from cht implementation. Reviewed-by: coleenp, rehn
src/hotspot/share/utilities/concurrentHashTable.hpp
src/hotspot/share/utilities/concurrentHashTable.inline.hpp
test/hotspot/gtest/utilities/test_concurrentHashtable.cpp
--- a/src/hotspot/share/utilities/concurrentHashTable.hpp	Thu Jan 10 16:50:26 2019 +0100
+++ b/src/hotspot/share/utilities/concurrentHashTable.hpp	Thu Jan 10 11:16:17 2019 -0600
@@ -312,11 +312,6 @@
   VALUE* internal_get(Thread* thread, LOOKUP_FUNC& lookup_f,
                       bool* grow_hint = NULL);
 
-  // Insert which handles a number of cases.
-  template <typename LOOKUP_FUNC, typename VALUE_FUNC, typename CALLBACK_FUNC>
-  bool internal_get_insert(Thread* thread, LOOKUP_FUNC& lookup_f, VALUE_FUNC& value_f,
-                           CALLBACK_FUNC& callback_f, bool* grow_hint = NULL, bool* clean_hint = NULL);
-
   // Plain insert.
   template <typename LOOKUP_FUNC>
   bool internal_insert(Thread* thread, LOOKUP_FUNC& lookup_f, const VALUE& value,
@@ -402,37 +397,6 @@
   // methods on the table during a callback is not supported.Only MultiGetHandle
   // supports multiple gets.
 
-  // LOOKUP_FUNC is matching methods, VALUE_FUNC creates value to be inserted
-  // and CALLBACK_FUNC is called with new or old value. Returns true if the
-  // value already exists.
-  template <typename LOOKUP_FUNC, typename VALUE_FUNC, typename CALLBACK_FUNC>
-  bool get_insert_lazy(Thread* thread, LOOKUP_FUNC& lookup_f, VALUE_FUNC& val_f,
-                       CALLBACK_FUNC& callback_f, bool* grow_hint = NULL, bool* clean_hint = NULL) {
-    return !internal_get_insert(thread, lookup_f, val_f, callback_f, grow_hint, clean_hint);
-  }
-
-  // Same without CALLBACK_FUNC.
-  template <typename LOOKUP_FUNC, typename VALUE_FUNC>
-  bool get_insert_lazy(Thread* thread, LOOKUP_FUNC& lookup_f, VALUE_FUNC& val_f,
-                       bool* grow_hint = NULL) {
-    return get_insert_lazy(thread, lookup_f, val_f, noOp, grow_hint);
-  }
-
-  // Same without VALUE_FUNC.
-  template <typename LOOKUP_FUNC, typename CALLBACK_FUNC>
-  bool get_insert(Thread* thread, LOOKUP_FUNC& lookup_f, const VALUE& value,
-                  CALLBACK_FUNC& callback_f, bool* grow_hint = NULL) {
-    LazyValueRetrieve vp(value);
-    return get_insert_lazy(thread, lookup_f, vp, callback_f, grow_hint);
-  }
-
-  // Same without CALLBACK_FUNC and VALUE_FUNC.
-  template <typename LOOKUP_FUNC>
-  bool get_insert(Thread* thread, LOOKUP_FUNC& lookup_f, const VALUE& value,
-                  bool* grow_hint = NULL) {
-    return get_insert(thread, lookup_f, value, noOp, grow_hint);
-  }
-
   // Get methods return true on found item with LOOKUP_FUNC and FOUND_FUNC is
   // called.
   template <typename LOOKUP_FUNC, typename FOUND_FUNC>
--- a/src/hotspot/share/utilities/concurrentHashTable.inline.hpp	Thu Jan 10 16:50:26 2019 +0100
+++ b/src/hotspot/share/utilities/concurrentHashTable.inline.hpp	Thu Jan 10 11:16:17 2019 -0600
@@ -874,78 +874,6 @@
 }
 
 template <typename VALUE, typename CONFIG, MEMFLAGS F>
-template <typename LOOKUP_FUNC, typename VALUE_FUNC, typename CALLBACK_FUNC>
-inline bool ConcurrentHashTable<VALUE, CONFIG, F>::
-  internal_get_insert(Thread* thread, LOOKUP_FUNC& lookup_f, VALUE_FUNC& value_f,
-                      CALLBACK_FUNC& callback_f, bool* grow_hint, bool* clean_hint)
-{
-  bool ret = false;
-  bool clean = false;
-  bool locked;
-  size_t loops = 0;
-  size_t i = 0;
-  Node* new_node = NULL;
-  uintx hash = lookup_f.get_hash();
-  while (true) {
-    {
-      ScopedCS cs(thread, this); /* protected the table/bucket */
-      Bucket* bucket = get_bucket(hash);
-
-      Node* first_at_start = bucket->first();
-      Node* old = get_node(bucket, lookup_f, &clean, &loops);
-      if (old == NULL) {
-        // No duplicate found.
-        if (new_node == NULL) {
-          new_node = Node::create_node(value_f(), first_at_start);
-        } else {
-          new_node->set_next(first_at_start);
-        }
-        if (bucket->cas_first(new_node, first_at_start)) {
-          callback_f(true, new_node->value());
-          new_node = NULL;
-          ret = true;
-          break; /* leave critical section */
-        }
-        // CAS failed we must leave critical section and retry.
-        locked = bucket->is_locked();
-      } else {
-        // There is a duplicate.
-        callback_f(false, old->value());
-        break; /* leave critical section */
-      }
-    } /* leave critical section */
-    i++;
-    if (locked) {
-      os::naked_yield();
-    } else {
-      SpinPause();
-    }
-  }
-
-  if (new_node != NULL) {
-    // CAS failed and a duplicate was inserted, we must free this node.
-    Node::destroy_node(new_node);
-  } else if (i == 0 && clean) {
-    // We only do cleaning on fast inserts.
-    Bucket* bucket = get_bucket_locked(thread, lookup_f.get_hash());
-    delete_in_bucket(thread, bucket, lookup_f);
-    bucket->unlock();
-
-    clean = false;
-  }
-
-  if (grow_hint != NULL) {
-    *grow_hint = loops > _grow_hint;
-  }
-
-  if (clean_hint != NULL) {
-    *clean_hint = clean;
-  }
-
-  return ret;
-}
-
-template <typename VALUE, typename CONFIG, MEMFLAGS F>
 template <typename LOOKUP_FUNC>
 inline bool ConcurrentHashTable<VALUE, CONFIG, F>::
   internal_insert(Thread* thread, LOOKUP_FUNC& lookup_f, const VALUE& value,
--- a/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp	Thu Jan 10 16:50:26 2019 +0100
+++ b/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp	Thu Jan 10 11:16:17 2019 -0600
@@ -81,39 +81,34 @@
   delete cht;
 }
 
-struct ValVerify {
-  uintptr_t _val;
-  bool called_get;
-  bool called_insert;
-  ValVerify(uintptr_t val) : _val(val), called_get(false), called_insert(false) {}
-  void operator()(bool inserted, uintptr_t* val) {
-    EXPECT_EQ(_val, *val) << "The value inserted is not correct.";
-    if (inserted) {
-      called_insert = true;
-    } else {
-      called_get = true;
-    }
+struct ValueGet {
+  uintptr_t _return;
+  ValueGet() : _return(0) {}
+  void operator()(uintptr_t* value) {
+    EXPECT_NE(value, (uintptr_t*)NULL) << "expected valid value";
+    _return = *value;
   }
-  void verify(bool get, bool insert) {
-    EXPECT_EQ(called_get, get) << "Get unexpected";
-    EXPECT_EQ(called_insert, insert) << "Insert unexpected";
+  uintptr_t get_value() const {
+    return _return;
   }
 };
 
-static void cht_get_insert_helper(Thread* thr, SimpleTestTable* cht, uintptr_t val) {
+static void cht_get_helper(Thread* thr, SimpleTestTable* cht, uintptr_t val) {
   {
     SimpleTestLookup stl(val);
-    ValVerify vv(val);
-    EXPECT_EQ(cht->get_insert(thr, stl, val, vv), false) << "Inserting an unique value failed.";
-    vv.verify(false, true);
+    ValueGet vg;
+    EXPECT_EQ(cht->get(thr, stl, vg), true) << "Getting an old value failed.";
+    EXPECT_EQ(val, vg.get_value()) << "Getting an old value failed.";
+  }
+}
+
+static void cht_insert_helper(Thread* thr, SimpleTestTable* cht, uintptr_t val) {
+  {
+    SimpleTestLookup stl(val);
+    EXPECT_EQ(cht->insert(thr, stl, val), true) << "Inserting an unique value failed.";
   }
 
-  {
-    SimpleTestLookup stl(val);
-    ValVerify vv(val);
-    EXPECT_EQ(cht->get_insert(thr, stl, val, vv), true) << "Getting an old value failed.";
-    vv.verify(true, false);
-  }
+  cht_get_helper(thr, cht, val);
 }
 
 static void cht_get_insert(Thread* thr) {
@@ -123,7 +118,7 @@
 
   {
     SCOPED_TRACE("First");
-    cht_get_insert_helper(thr, cht, val);
+    cht_insert_helper(thr, cht, val);
   }
   EXPECT_EQ(cht->get_copy(thr, stl), val) << "Get an old value failed";
   EXPECT_TRUE(cht->remove(thr, stl)) << "Removing existing value failed.";
@@ -131,7 +126,7 @@
 
   {
     SCOPED_TRACE("Second");
-    cht_get_insert_helper(thr, cht, val);
+    cht_insert_helper(thr, cht, val);
   }
 
   delete cht;
@@ -148,10 +143,13 @@
 
 static void cht_getinsert_bulkdelete_insert_verified(Thread* thr, SimpleTestTable* cht, uintptr_t val,
                                                      bool verify_expect_get, bool verify_expect_inserted) {
-  ValVerify vv(val);
   SimpleTestLookup stl(val);
-  EXPECT_EQ(cht->get_insert(thr, stl, val, vv), verify_expect_get) << "Inserting an unique value failed.";
-  vv.verify(verify_expect_get, verify_expect_inserted);
+  if (verify_expect_inserted) {
+    cht_insert_helper(thr, cht, val);
+  }
+  if (verify_expect_get) {
+    cht_get_helper(thr, cht, val);
+  }
 }
 
 static void cht_getinsert_bulkdelete(Thread* thr) {
@@ -856,10 +854,19 @@
     bool grow;
     MyDel del(_br);
     for (uintptr_t v = _start; v <= _stop; v++) {
-      ValVerify vv(v);
-      TestLookup tl(v);
-      _cht->get_insert(this, tl, v, vv, &grow);
-      EXPECT_NE(vv.called_get, vv.called_insert) << "Non or both callbacks was called.";
+      {
+        TestLookup tl(v);
+        ValueGet vg;
+        do {
+          if (_cht->get(this, tl, vg, &grow)) {
+            EXPECT_EQ(v, vg.get_value()) << "Getting an old value failed.";
+            break;
+          }
+          if (_cht->insert(this, tl, v, &grow)) {
+            break;
+          }
+        } while(true);
+      }
       if (grow && !_shrink) {
         _cht->grow(this);
       }