changeset 59373:0c21a8e4fe47

8244733: Add ResourceHashtable::xxx_if_absent Reviewed-by: coleenp, iklam, rehn, dholmes
author stuefe
date Wed, 20 May 2020 15:56:39 +0200
parents 3df43128536e
children a0d9d2e73ef2 35f88d70cb09
files src/hotspot/share/classfile/bytecodeAssembler.cpp src/hotspot/share/classfile/classLoaderStats.cpp src/hotspot/share/classfile/classLoaderStats.hpp src/hotspot/share/classfile/systemDictionaryShared.cpp src/hotspot/share/jfr/periodic/jfrPeriodic.cpp src/hotspot/share/utilities/resourceHash.hpp test/hotspot/gtest/utilities/test_resourceHash.cpp
diffstat 7 files changed, 127 insertions(+), 56 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/classfile/bytecodeAssembler.cpp	Wed May 20 15:24:32 2020 +0200
+++ b/src/hotspot/share/classfile/bytecodeAssembler.cpp	Wed May 20 15:56:39 2020 +0200
@@ -32,12 +32,12 @@
 #include "utilities/bytes.hpp"
 
 u2 BytecodeConstantPool::find_or_add(BytecodeCPEntry const& bcpe) {
-  u2 index;
-  u2* probe = _indices.get(bcpe);
-  if (probe == NULL) {
-    index = _entries.length();
+
+  u2 index = _entries.length();
+  bool created = false;
+  u2* probe = _indices.put_if_absent(bcpe, index, &created);
+  if (created) {
     _entries.append(bcpe);
-    _indices.put(bcpe, index);
   } else {
     index = *probe;
   }
--- a/src/hotspot/share/classfile/classLoaderStats.cpp	Wed May 20 15:24:32 2020 +0200
+++ b/src/hotspot/share/classfile/classLoaderStats.cpp	Wed May 20 15:56:39 2020 +0200
@@ -44,27 +44,23 @@
   }
 };
 
-
 void ClassLoaderStatsClosure::do_cld(ClassLoaderData* cld) {
   oop cl = cld->class_loader();
-  ClassLoaderStats* cls;
 
   // The hashtable key is the ClassLoader oop since we want to account
   // for "real" classes and anonymous classes together
-  ClassLoaderStats** cls_ptr = _stats->get(cl);
-  if (cls_ptr == NULL) {
-    cls = new ClassLoaderStats();
-    _stats->put(cl, cls);
+  bool added = false;
+  ClassLoaderStats* cls = _stats->put_if_absent(cl, &added);
+  if (added) {
+    cls->_class_loader = cl;
     _total_loaders++;
-  } else {
-    cls = *cls_ptr;
   }
+  assert(cls->_class_loader == cl, "Sanity");
 
   if (!cld->has_class_mirror_holder()) {
     cls->_cld = cld;
   }
 
-  cls->_class_loader = cl;
   if (cl != NULL) {
     cls->_parent = java_lang_ClassLoader::parent(cl);
     addEmptyParents(cls->_parent);
@@ -105,25 +101,25 @@
 #endif
 
 
-bool ClassLoaderStatsClosure::do_entry(oop const& key, ClassLoaderStats* const& cls) {
-  Klass* class_loader_klass = (cls->_class_loader == NULL ? NULL : cls->_class_loader->klass());
-  Klass* parent_klass = (cls->_parent == NULL ? NULL : cls->_parent->klass());
+bool ClassLoaderStatsClosure::do_entry(oop const& key, ClassLoaderStats const& cls) {
+  Klass* class_loader_klass = (cls._class_loader == NULL ? NULL : cls._class_loader->klass());
+  Klass* parent_klass = (cls._parent == NULL ? NULL : cls._parent->klass());
 
   _out->print(INTPTR_FORMAT "  " INTPTR_FORMAT "  " INTPTR_FORMAT "  " UINTX_FORMAT_W(6) "  " SIZE_FORMAT_W(8) "  " SIZE_FORMAT_W(8) "  ",
-      p2i(class_loader_klass), p2i(parent_klass), p2i(cls->_cld),
-      cls->_classes_count,
-      cls->_chunk_sz, cls->_block_sz);
+      p2i(class_loader_klass), p2i(parent_klass), p2i(cls._cld),
+      cls._classes_count,
+      cls._chunk_sz, cls._block_sz);
   if (class_loader_klass != NULL) {
     _out->print("%s", class_loader_klass->external_name());
   } else {
     _out->print("<boot class loader>");
   }
   _out->cr();
-  if (cls->_hidden_classes_count > 0) {
+  if (cls._hidden_classes_count > 0) {
     _out->print_cr(SPACE SPACE SPACE "                                    " UINTX_FORMAT_W(6) "  " SIZE_FORMAT_W(8) "  " SIZE_FORMAT_W(8) "   + hidden classes",
         "", "", "",
-        cls->_hidden_classes_count,
-        cls->_hidden_chunk_sz, cls->_hidden_block_sz);
+        cls._hidden_classes_count,
+        cls._hidden_chunk_sz, cls._hidden_block_sz);
   }
   return true;
 }
@@ -146,15 +142,14 @@
 void ClassLoaderStatsClosure::addEmptyParents(oop cl) {
   while (cl != NULL && java_lang_ClassLoader::loader_data_acquire(cl) == NULL) {
     // This classloader has not loaded any classes
-    ClassLoaderStats** cls_ptr = _stats->get(cl);
-    if (cls_ptr == NULL) {
-      // It does not exist in our table - add it
-      ClassLoaderStats* cls = new ClassLoaderStats();
+    bool added = false;
+    ClassLoaderStats* cls = _stats->put_if_absent(cl, &added);
+    if (added) {
       cls->_class_loader = cl;
       cls->_parent = java_lang_ClassLoader::parent(cl);
-      _stats->put(cl, cls);
       _total_loaders++;
     }
+    assert(cls->_class_loader == cl, "Sanity");
 
     cl = java_lang_ClassLoader::parent(cl);
   }
--- a/src/hotspot/share/classfile/classLoaderStats.hpp	Wed May 20 15:24:32 2020 +0200
+++ b/src/hotspot/share/classfile/classLoaderStats.hpp	Wed May 20 15:56:39 2020 +0200
@@ -115,7 +115,7 @@
     return hash;
   }
 
-  typedef ResourceHashtable<oop, ClassLoaderStats*,
+  typedef ResourceHashtable<oop, ClassLoaderStats,
       ClassLoaderStatsClosure::oop_hash, ClassLoaderStatsClosure::oop_equals> StatsTable;
 
   outputStream* _out;
@@ -136,7 +136,7 @@
   }
 
   virtual void do_cld(ClassLoaderData* cld);
-  virtual bool do_entry(oop const& key, ClassLoaderStats* const& cls);
+  virtual bool do_entry(oop const& key, ClassLoaderStats const& cls);
   void print();
 
 private:
--- a/src/hotspot/share/classfile/systemDictionaryShared.cpp	Wed May 20 15:24:32 2020 +0200
+++ b/src/hotspot/share/classfile/systemDictionaryShared.cpp	Wed May 20 15:56:39 2020 +0200
@@ -198,14 +198,14 @@
   int _unregistered_count;
 public:
   DumpTimeSharedClassInfo* find_or_allocate_info_for(InstanceKlass* k) {
-    DumpTimeSharedClassInfo* p = get(k);
-    if (p == NULL) {
+    bool created = false;
+    DumpTimeSharedClassInfo* p = put_if_absent(k, &created);
+    if (created) {
       assert(!SystemDictionaryShared::no_class_loading_should_happen(),
              "no new classes can be loaded while dumping archive");
-      put(k, DumpTimeSharedClassInfo());
-      p = get(k);
-      assert(p != NULL, "sanity");
       p->_klass = k;
+    } else {
+      assert(p->_klass == k, "Sanity");
     }
     return p;
   }
@@ -1041,19 +1041,16 @@
   ResourceObj::C_HEAP> _loaded_unregistered_classes;
 
 bool SystemDictionaryShared::add_unregistered_class(InstanceKlass* k, TRAPS) {
+  // We don't allow duplicated unregistered classes of the same name.
   assert(DumpSharedSpaces, "only when dumping");
-
   Symbol* name = k->name();
-  if (_loaded_unregistered_classes.get(name) != NULL) {
-    // We don't allow duplicated unregistered classes of the same name.
-    return false;
-  } else {
-    bool isnew = _loaded_unregistered_classes.put(name, true);
-    assert(isnew, "sanity");
+  bool created = false;
+  _loaded_unregistered_classes.put_if_absent(name, true, &created);
+  if (created) {
     MutexLocker mu_r(THREAD, Compile_lock); // add_to_hierarchy asserts this.
     SystemDictionary::add_to_hierarchy(k, CHECK_false);
-    return true;
   }
+  return created;
 }
 
 // This function is called to resolve the super/interfaces of shared classes for
--- a/src/hotspot/share/jfr/periodic/jfrPeriodic.cpp	Wed May 20 15:24:32 2020 +0200
+++ b/src/hotspot/share/jfr/periodic/jfrPeriodic.cpp	Wed May 20 15:56:39 2020 +0200
@@ -468,21 +468,21 @@
 public:
   JfrClassLoaderStatsClosure() : ClassLoaderStatsClosure(NULL) {}
 
-  bool do_entry(oop const& key, ClassLoaderStats* const& cls) {
-    const ClassLoaderData* this_cld = cls->_class_loader != NULL ?
-      java_lang_ClassLoader::loader_data_acquire(cls->_class_loader) : NULL;
-    const ClassLoaderData* parent_cld = cls->_parent != NULL ?
-      java_lang_ClassLoader::loader_data_acquire(cls->_parent) : NULL;
+  bool do_entry(oop const& key, ClassLoaderStats const& cls) {
+    const ClassLoaderData* this_cld = cls._class_loader != NULL ?
+      java_lang_ClassLoader::loader_data_acquire(cls._class_loader) : NULL;
+    const ClassLoaderData* parent_cld = cls._parent != NULL ?
+      java_lang_ClassLoader::loader_data_acquire(cls._parent) : NULL;
     EventClassLoaderStatistics event;
     event.set_classLoader(this_cld);
     event.set_parentClassLoader(parent_cld);
-    event.set_classLoaderData((intptr_t)cls->_cld);
-    event.set_classCount(cls->_classes_count);
-    event.set_chunkSize(cls->_chunk_sz);
-    event.set_blockSize(cls->_block_sz);
-    event.set_hiddenClassCount(cls->_hidden_classes_count);
-    event.set_hiddenChunkSize(cls->_hidden_chunk_sz);
-    event.set_hiddenBlockSize(cls->_hidden_block_sz);
+    event.set_classLoaderData((intptr_t)cls._cld);
+    event.set_classCount(cls._classes_count);
+    event.set_chunkSize(cls._chunk_sz);
+    event.set_blockSize(cls._block_sz);
+    event.set_hiddenClassCount(cls._hidden_classes_count);
+    event.set_hiddenChunkSize(cls._hidden_chunk_sz);
+    event.set_hiddenBlockSize(cls._hidden_block_sz);
     event.commit();
     return true;
   }
--- a/src/hotspot/share/utilities/resourceHash.hpp	Wed May 20 15:24:32 2020 +0200
+++ b/src/hotspot/share/utilities/resourceHash.hpp	Wed May 20 15:56:39 2020 +0200
@@ -51,6 +51,11 @@
 
     Node(unsigned hash, K const& key, V const& value) :
         _hash(hash), _key(key), _value(value), _next(NULL) {}
+
+    // Create a node with a default-constructed value.
+    Node(unsigned hash, K const& key) :
+        _hash(hash), _key(key), _value(), _next(NULL) {}
+
   };
 
   Node* _table[SIZE];
@@ -124,6 +129,41 @@
     }
   }
 
+  // Look up the key.
+  // If an entry for the key exists, leave map unchanged and return a pointer to its value.
+  // If no entry for the key exists, create a new entry from key and a default-created value
+  //  and return a pointer to the value.
+  // *p_created is new if entry was created, false if entry pre-existed.
+  V* put_if_absent(K const& key, bool* p_created) {
+    unsigned hv = HASH(key);
+    Node** ptr = lookup_node(hv, key);
+    if (*ptr == NULL) {
+      *ptr = new (ALLOC_TYPE, MEM_TYPE) Node(hv, key);
+      *p_created = true;
+    } else {
+      *p_created = false;
+    }
+    return &(*ptr)->_value;
+  }
+
+  // Look up the key.
+  // If an entry for the key exists, leave map unchanged and return a pointer to its value.
+  // If no entry for the key exists, create a new entry from key and value and return a
+  //  pointer to the value.
+  // *p_created is new if entry was created, false if entry pre-existed.
+  V* put_if_absent(K const& key, V const& value, bool* p_created) {
+    unsigned hv = HASH(key);
+    Node** ptr = lookup_node(hv, key);
+    if (*ptr == NULL) {
+      *ptr = new (ALLOC_TYPE, MEM_TYPE) Node(hv, key, value);
+      *p_created = true;
+    } else {
+      *p_created = false;
+    }
+    return &(*ptr)->_value;
+  }
+
+
   bool remove(K const& key) {
     unsigned hv = HASH(key);
     Node** ptr = lookup_node(hv, key);
--- a/test/hotspot/gtest/utilities/test_resourceHash.cpp	Wed May 20 15:24:32 2020 +0200
+++ b/test/hotspot/gtest/utilities/test_resourceHash.cpp	Wed May 20 15:56:39 2020 +0200
@@ -26,12 +26,13 @@
 #include "memory/resourceArea.hpp"
 #include "unittest.hpp"
 #include "utilities/debug.hpp"
+#include "utilities/globalDefinitions.hpp"
 #include "utilities/resourceHash.hpp"
 
 class CommonResourceHashtableTest : public ::testing::Test {
  protected:
   typedef void* K;
-  typedef int V;
+  typedef uintx V;
   const static MEMFLAGS MEM_TYPE = mtInternal;
 
   static unsigned identity_hash(const K& k) {
@@ -58,6 +59,7 @@
       }
     }
   };
+
 };
 
 class SmallResourceHashtableTest : public CommonResourceHashtableTest {
@@ -96,7 +98,44 @@
       }
 
       ASSERT_TRUE(rh.remove(as_K(step)));
+      ASSERT_FALSE(rh.contains(as_K(step)));
       rh.iterate(&et);
+
+
+      // Test put_if_absent(key) (creating a default-created value)
+      bool created = false;
+      V* v = rh.put_if_absent(as_K(step), &created);
+      ASSERT_TRUE(rh.contains(as_K(step)));
+      ASSERT_TRUE(created);
+      *v = (V)step;
+
+      // Calling this function a second time should yield the same value pointer
+      V* v2 = rh.put_if_absent(as_K(step), &created);
+      ASSERT_EQ(v, v2);
+      ASSERT_EQ(*v2, *v);
+      ASSERT_FALSE(created);
+
+      ASSERT_TRUE(rh.remove(as_K(step)));
+      ASSERT_FALSE(rh.contains(as_K(step)));
+      rh.iterate(&et);
+
+      // Test put_if_absent(key, value)
+      v = rh.put_if_absent(as_K(step), step, &created);
+      ASSERT_EQ(*v, step);
+      ASSERT_TRUE(rh.contains(as_K(step)));
+      ASSERT_TRUE(created);
+
+      v2 = rh.put_if_absent(as_K(step), step, &created);
+      // Calling this function a second time should yield the same value pointer
+      ASSERT_EQ(v, v2);
+      ASSERT_EQ(*v2, (V)step);
+      ASSERT_FALSE(created);
+
+      ASSERT_TRUE(rh.remove(as_K(step)));
+      ASSERT_FALSE(rh.contains(as_K(step)));
+      rh.iterate(&et);
+
+
     }
   };
 };