8213791: StringTable: Use get and insert
authorrehn
Wed, 28 Nov 2018 11:06:27 +0100
changeset 52717 b22da519f2e3
parent 52716 877dd2b0f36c
child 52718 263c7685a22a
8213791: StringTable: Use get and insert Reviewed-by: eosterlund, gziemski
src/hotspot/share/classfile/stringTable.cpp
src/hotspot/share/classfile/stringTable.hpp
src/hotspot/share/utilities/concurrentHashTable.hpp
src/hotspot/share/utilities/concurrentHashTable.inline.hpp
--- a/src/hotspot/share/classfile/stringTable.cpp	Wed Nov 28 11:06:58 2018 +0100
+++ b/src/hotspot/share/classfile/stringTable.cpp	Wed Nov 28 11:06:27 2018 +0100
@@ -206,6 +206,12 @@
   _local_table = new StringTableHash(start_size_log_2, END_SIZE, REHASH_LEN);
 }
 
+void StringTable::update_needs_rehash(bool rehash) {
+  if (rehash) {
+    _needs_rehashing = true;
+  }
+}
+
 size_t StringTable::item_added() {
   return Atomic::add((size_t)1, &(the_table()->_items_count));
 }
@@ -281,9 +287,7 @@
   StringTableGet stg(thread);
   bool rehash_warning;
   _local_table->get(thread, lookup, stg, &rehash_warning);
-  if (rehash_warning) {
-    _needs_rehashing = true;
-  }
+  update_needs_rehash(rehash_warning);
   return stg.get_res_oop();
 }
 
@@ -334,30 +338,6 @@
                                              hash, CHECK_NULL);
 }
 
-class StringTableCreateEntry : public StackObj {
- private:
-   Thread* _thread;
-   Handle  _return;
-   Handle  _store;
- public:
-  StringTableCreateEntry(Thread* thread, Handle store)
-    : _thread(thread), _store(store) {}
-
-  WeakHandle<vm_string_table_data> operator()() { // No dups found
-    WeakHandle<vm_string_table_data> wh =
-      WeakHandle<vm_string_table_data>::create(_store);
-    return wh;
-  }
-  void operator()(bool inserted, WeakHandle<vm_string_table_data>* val) {
-    oop result = val->resolve();
-    assert(result != NULL, "Result should be reachable");
-    _return = Handle(_thread, result);
-  }
-  oop get_return() const {
-    return _return();
-  }
-};
-
 oop StringTable::do_intern(Handle string_or_null_h, const jchar* name,
                            int len, uintx hash, TRAPS) {
   HandleMark hm(THREAD);  // cleanup strings created
@@ -377,15 +357,23 @@
   assert(java_lang_String::equals(string_h(), name, len),
          "string must be properly initialized");
   assert(len == java_lang_String::length(string_h()), "Must be same length");
+
   StringTableLookupOop lookup(THREAD, hash, string_h);
-  StringTableCreateEntry stc(THREAD, string_h);
+  StringTableGet stg(THREAD);
 
   bool rehash_warning;
-  _local_table->get_insert_lazy(THREAD, lookup, stc, stc, &rehash_warning);
-  if (rehash_warning) {
-    _needs_rehashing = true;
-  }
-  return stc.get_return();
+  do {
+    if (_local_table->get(THREAD, lookup, stg, &rehash_warning)) {
+      update_needs_rehash(rehash_warning);
+      return stg.get_res_oop();
+    }
+    WeakHandle<vm_string_table_data> wh = WeakHandle<vm_string_table_data>::create(string_h);
+    // The hash table takes ownership of the WeakHandle, even if it's not inserted.
+    if (_local_table->insert(THREAD, lookup, wh, &rehash_warning)) {
+      update_needs_rehash(rehash_warning);
+      return wh.resolve();
+    }
+  } while(true);
 }
 
 // GC support
--- a/src/hotspot/share/classfile/stringTable.hpp	Wed Nov 28 11:06:58 2018 +0100
+++ b/src/hotspot/share/classfile/stringTable.hpp	Wed Nov 28 11:06:27 2018 +0100
@@ -93,6 +93,7 @@
 
   void try_rehash_table();
   bool do_rehash();
+  inline void update_needs_rehash(bool rehash);
 
  public:
   // The string table
--- a/src/hotspot/share/utilities/concurrentHashTable.hpp	Wed Nov 28 11:06:58 2018 +0100
+++ b/src/hotspot/share/utilities/concurrentHashTable.hpp	Wed Nov 28 11:06:27 2018 +0100
@@ -313,8 +313,13 @@
 
   // Insert which handles a number of cases.
   template <typename LOOKUP_FUNC, typename VALUE_FUNC, typename CALLBACK_FUNC>
-  bool internal_insert(Thread* thread, LOOKUP_FUNC& lookup_f, VALUE_FUNC& value_f,
-                       CALLBACK_FUNC& callback, bool* grow_hint = NULL, bool* clean_hint = NULL);
+  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,
+                       bool* grow_hint, bool* clean_hint);
 
   // Returns true if an item matching LOOKUP_FUNC is removed.
   // Calls DELETE_FUNC before destroying the node.
@@ -402,7 +407,7 @@
   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_insert(thread, lookup_f, val_f, callback_f, grow_hint, clean_hint);
+    return !internal_get_insert(thread, lookup_f, val_f, callback_f, grow_hint, clean_hint);
   }
 
   // Same without CALLBACK_FUNC.
@@ -442,8 +447,7 @@
   template <typename LOOKUP_FUNC>
   bool insert(Thread* thread, LOOKUP_FUNC& lookup_f, const VALUE& value,
               bool* grow_hint = NULL, bool* clean_hint = NULL) {
-    LazyValueRetrieve vp(value);
-    return internal_insert(thread, lookup_f, vp, noOp, grow_hint, clean_hint);
+    return internal_insert(thread, lookup_f, value, grow_hint, clean_hint);
   }
 
   // This does a fast unsafe insert and can thus only be used when there is no
--- a/src/hotspot/share/utilities/concurrentHashTable.inline.hpp	Wed Nov 28 11:06:58 2018 +0100
+++ b/src/hotspot/share/utilities/concurrentHashTable.inline.hpp	Wed Nov 28 11:06:27 2018 +0100
@@ -876,8 +876,8 @@
 template <typename VALUE, typename CONFIG, MEMFLAGS F>
 template <typename LOOKUP_FUNC, typename VALUE_FUNC, typename CALLBACK_FUNC>
 inline bool ConcurrentHashTable<VALUE, CONFIG, F>::
-  internal_insert(Thread* thread, LOOKUP_FUNC& lookup_f, VALUE_FUNC& value_f,
-                  CALLBACK_FUNC& callback, bool* grow_hint, bool* clean_hint)
+  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;
@@ -901,7 +901,7 @@
           new_node->set_next(first_at_start);
         }
         if (bucket->cas_first(new_node, first_at_start)) {
-          callback(true, new_node->value());
+          callback_f(true, new_node->value());
           new_node = NULL;
           ret = true;
           break; /* leave critical section */
@@ -910,7 +910,7 @@
         locked = bucket->is_locked();
       } else {
         // There is a duplicate.
-        callback(false, old->value());
+        callback_f(false, old->value());
         break; /* leave critical section */
       }
     } /* leave critical section */
@@ -946,6 +946,70 @@
 }
 
 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,
+                  bool* grow_hint, bool* clean_hint)
+{
+  bool ret = false;
+  bool clean = false;
+  bool locked;
+  size_t loops = 0;
+  size_t i = 0;
+  uintx hash = lookup_f.get_hash();
+  Node* new_node = Node::create_node(value, NULL);
+
+  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) {
+        new_node->set_next(first_at_start);
+        if (bucket->cas_first(new_node, first_at_start)) {
+          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.
+        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 FUNC>
 inline bool ConcurrentHashTable<VALUE, CONFIG, F>::
   visit_nodes(Bucket* bucket, FUNC& visitor_f)