8306843: JVMTI tag map extremely slow after JDK-8292741

Reviewed-by: sspitsyn, iklam

src/hotspot/share/classfile/stringTable.cpp

@@ -659,12 +659,11 @@ class VerifyCompStrings : StackObj {
     return java_lang_String::equals(a, b);
   }
 
-  ResizeableResourceHashtable<oop, bool,
-                              AnyObj::C_HEAP, mtInternal,
+  ResizeableResourceHashtable<oop, bool, AnyObj::C_HEAP, mtInternal,
                               string_hash, string_equals> _table;
  public:
   size_t _errors;
-  VerifyCompStrings() : _table(unsigned(_items_count / 8) + 1), _errors(0) {}
+  VerifyCompStrings() : _table(unsigned(_items_count / 8) + 1, 0 /* do not resize */), _errors(0) {}
   bool operator()(WeakHandle* val) {
     oop s = val->resolve();
     if (s == nullptr) {

src/hotspot/share/prims/jvmtiTagMap.cpp

@@ -241,23 +241,14 @@ class CallbackWrapper : public StackObj {
 void inline CallbackWrapper::post_callback_tag_update(oop o,
                                                       JvmtiTagMapTable* hashmap,
                                                       jlong obj_tag) {
-  jlong current_tag = hashmap->find(o);
-  if (current_tag == 0) {
-    if (obj_tag != 0) {
-      // callback has tagged the object
-      assert(Thread::current()->is_VM_thread(), "must be VMThread");
-      hashmap->add(o, obj_tag);
-    }
+  if (obj_tag == 0) {
+    // callback has untagged the object, remove the entry if present
+    hashmap->remove(o);
   } else {
-    // object was previously tagged - the callback may have untagged
-    // the object or changed the tag value
-    if (obj_tag == 0) {
-      hashmap->remove(o);
-    } else {
-      if (obj_tag != current_tag) {
-        hashmap->update(o, obj_tag);
-      }
-    }
+    // object was previously tagged or not present - the callback may have
+    // changed the tag value
+    assert(Thread::current()->is_VM_thread(), "must be VMThread");
+    hashmap->add(o, obj_tag);
   }
 }
 
@@ -347,24 +338,14 @@ void JvmtiTagMap::set_tag(jobject object, jlong tag) {
 
   // see if the object is already tagged
   JvmtiTagMapTable* hashmap = _hashmap;
-  jlong found_tag = hashmap->find(o);
 
-  // if the object is not already tagged then we tag it
-  if (found_tag == 0) {
-    if (tag != 0) {
-      hashmap->add(o, tag);
-    } else {
-      // no-op
-    }
+  if (tag == 0) {
+    // remove the entry if present
+    hashmap->remove(o);
   } else {
-    // if the object is already tagged then we either update
-    // the tag (if a new tag value has been provided)
-    // or remove the object if the new tag value is 0.
-    if (tag == 0) {
-      hashmap->remove(o);
-    } else {
-      hashmap->update(o, tag);
-    }
+    // if the object is already tagged or not present then we add/update
+    // the tag
+    hashmap->add(o, tag);
   }
 }
 

src/hotspot/share/prims/jvmtiTagMapTable.cpp

@@ -35,25 +35,23 @@ JvmtiTagMapKey::JvmtiTagMapKey(oop obj) : _obj(obj) {}
 
 JvmtiTagMapKey::JvmtiTagMapKey(const JvmtiTagMapKey& src) {
   // move object into WeakHandle when copying into the table
-  assert(src._obj != nullptr, "must be set");
+  if (src._obj != nullptr) {
 
-  // obj was read with AS_NO_KEEPALIVE, or equivalent, like during
-  // a heap walk.  The object needs to be kept alive when it is published.
-  Universe::heap()->keep_alive(src._obj);
+    // obj was read with AS_NO_KEEPALIVE, or equivalent, like during
+    // a heap walk.  The object needs to be kept alive when it is published.
+    Universe::heap()->keep_alive(src._obj);
 
-  _wh = WeakHandle(JvmtiExport::weak_tag_storage(), src._obj);
+    _wh = WeakHandle(JvmtiExport::weak_tag_storage(), src._obj);
+  } else {
+    // resizing needs to create a copy.
+    _wh = src._wh;
+  }
+  // obj is always null after a copy.
   _obj = nullptr;
 }
 
-JvmtiTagMapKey::~JvmtiTagMapKey() {
-  // If obj is set null it out, this is called for stack object on lookup,
-  // and it should not have a WeakHandle created for it yet.
-  if (_obj != nullptr) {
-    _obj = nullptr;
-    assert(_wh.is_null(), "WeakHandle should be null");
-  } else {
-    _wh.release(JvmtiExport::weak_tag_storage());
-  }
+void JvmtiTagMapKey::release_weak_handle() const {
+  _wh.release(JvmtiExport::weak_tag_storage());
 }
 
 oop JvmtiTagMapKey::object() const {
@@ -66,11 +64,15 @@ oop JvmtiTagMapKey::object_no_keepalive() const {
   return _wh.peek();
 }
 
-JvmtiTagMapTable::JvmtiTagMapTable() : _table(Constants::_table_size) {}
+static const int INITIAL_TABLE_SIZE = 1007;
+static const int MAX_TABLE_SIZE     = 0x3fffffff;
+
+JvmtiTagMapTable::JvmtiTagMapTable() : _table(INITIAL_TABLE_SIZE, MAX_TABLE_SIZE) {}
 
 void JvmtiTagMapTable::clear() {
   struct RemoveAll {
     bool do_entry(const JvmtiTagMapKey& entry, const jlong& tag) {
+      entry.release_weak_handle();
       return true;
     }
   } remove_all;
@@ -104,31 +106,35 @@ jlong JvmtiTagMapTable::find(oop obj) {
 
 void JvmtiTagMapTable::add(oop obj, jlong tag) {
   JvmtiTagMapKey new_entry(obj);
-  bool is_added = false;
-  _table.put_if_absent(new_entry, tag, &is_added);
-  assert(is_added, "should be added");
-}
-
-void JvmtiTagMapTable::update(oop obj, jlong tag) {
-  JvmtiTagMapKey new_entry(obj);
-  bool is_updated = _table.put(new_entry, tag) == false;
-  assert(is_updated, "should be updated and not added");
+  bool is_added;
+  if (obj->fast_no_hash_check()) {
+    // Can't be in the table so add it fast.
+    is_added = _table.put_when_absent(new_entry, tag);
+  } else {
+    jlong* value = _table.put_if_absent(new_entry, tag, &is_added);
+    *value = tag; // assign the new tag
+  }
+  if (is_added) {
+    if (_table.maybe_grow(5, true /* use_large_table_sizes */)) {
+      int max_bucket_size = DEBUG_ONLY(_table.verify()) NOT_DEBUG(0);
+      log_info(jvmti, table) ("JvmtiTagMap table resized to %d for %d entries max bucket %d",
+                              _table.table_size(), _table.number_of_entries(), max_bucket_size);
+    }
+  }
 }
 
 void JvmtiTagMapTable::remove(oop obj) {
   JvmtiTagMapKey jtme(obj);
-  bool is_removed = _table.remove(jtme);
-  assert(is_removed, "remove not succesfull.");
+  auto clean = [] (const JvmtiTagMapKey& entry, jlong tag) {
+    entry.release_weak_handle();
+  };
+  _table.remove(jtme, clean);
 }
 
 void JvmtiTagMapTable::entry_iterate(JvmtiTagMapKeyClosure* closure) {
   _table.iterate(closure);
 }
 
-void JvmtiTagMapTable::resize_if_needed() {
-  _table.maybe_grow();
-}
-
 void JvmtiTagMapTable::remove_dead_entries(GrowableArray<jlong>* objects) {
   struct IsDead {
     GrowableArray<jlong>* _objects;
@@ -138,6 +144,7 @@ void JvmtiTagMapTable::remove_dead_entries(GrowableArray<jlong>* objects) {
         if (_objects != nullptr) {
           _objects->append(tag);
         }
+        entry.release_weak_handle();
         return true;
       }
       return false;;

src/hotspot/share/prims/jvmtiTagMapTable.hpp

@@ -48,11 +48,9 @@ class JvmtiTagMapKey : public CHeapObj<mtServiceability> {
   JvmtiTagMapKey(const JvmtiTagMapKey& src);
   JvmtiTagMapKey& operator=(const JvmtiTagMapKey&) = delete;
 
-  ~JvmtiTagMapKey();
-
-  void resolve();
   oop object() const;
   oop object_no_keepalive() const;
+  void release_weak_handle() const;
 
   static unsigned get_hash(const JvmtiTagMapKey& entry) {
     assert(entry._obj != nullptr, "must lookup obj to hash");
@@ -73,12 +71,7 @@ ResizeableResourceHashtable <JvmtiTagMapKey, jlong,
                               JvmtiTagMapKey::equals> ResizableResourceHT;
 
 class JvmtiTagMapTable : public CHeapObj<mtServiceability> {
- enum Constants {
-  _table_size  = 1007
- };
-
  private:
-  void resize_if_needed();
   ResizableResourceHT _table;
 
  public:
@@ -87,7 +80,6 @@ class JvmtiTagMapTable : public CHeapObj<mtServiceability> {
 
   jlong find(oop obj);
   void add(oop obj, jlong tag);
-  void update(oop obj, jlong tag);
 
   void remove(oop obj);
 

src/hotspot/share/utilities/resizeableResourceHash.hpp

@@ -85,19 +85,44 @@ class ResizeableResourceHashtable : public ResourceHashtableBase<
                                      K, V, ALLOC_TYPE, MEM_TYPE, HASH, EQUALS>;
   using Node = ResourceHashtableNode<K, V>;
   NONCOPYABLE(ResizeableResourceHashtable);
+
+  // Calculate next "good" hashtable size based on requested count
+  int calculate_resize(bool use_large_table_sizes) const {
+    const int resize_factor = 2;     // by how much we will resize using current number of entries
+
+    // possible hashmap sizes - odd primes that roughly double in size.
+    // To avoid excessive resizing the odd primes from 4801-76831 and
+    // 76831-307261 have been removed.
+    const int large_table_sizes[] =  { 107, 1009, 2017, 4049, 5051, 10103, 20201,
+                                       40423, 76831, 307261, 614563, 1228891, 2457733,
+                                       4915219, 9830479, 19660831, 39321619, 78643219 };
+    const int large_array_size = sizeof(large_table_sizes)/sizeof(int);
+
+    int requested = resize_factor * BASE::number_of_entries();
+    int start_at = use_large_table_sizes ? 8 : 0;
+    int newsize;
+    for (int i = start_at; i < large_array_size; i++) {
+      newsize = large_table_sizes[i];
+      if (newsize >= requested) {
+        return newsize;
+      }
+    }
+    return requested; // greater than a size in the table
+  }
+
 public:
-  ResizeableResourceHashtable(unsigned size, unsigned max_size = 0)
+  ResizeableResourceHashtable(unsigned size, unsigned max_size)
   : BASE(size), _max_size(max_size) {
     assert(size <= 0x3fffffff && max_size <= 0x3fffffff, "avoid overflow in resize");
   }
 
-  bool maybe_grow(int load_factor = 8) {
+  bool maybe_grow(int load_factor = 8, bool use_large_table_sizes = false) {
     unsigned old_size = BASE::_table_size;
     if (old_size >= _max_size) {
       return false;
     }
     if (BASE::number_of_entries() / int(old_size) > load_factor) {
-      unsigned new_size = MIN2<unsigned>(old_size * 2, _max_size);
+      unsigned new_size = MIN2<unsigned>(calculate_resize(use_large_table_sizes), _max_size);
       resize(new_size);
       return true;
     } else {
@@ -114,7 +139,7 @@ public:
       Node* node = *bucket;
       while (node != nullptr) {
         Node* next = node->_next;
-        unsigned hash = HASH(node->_key);
+        unsigned hash = node->_hash;
         unsigned index = hash % new_size;
 
         node->_next = new_table[index];
@@ -131,6 +156,28 @@ public:
     BASE::_table = new_table;
     BASE::_table_size = new_size;
   }
+
+#ifdef ASSERT
+  int verify() {
+    Node** table = BASE::_table;
+    // Return max bucket size.  If hashcode is broken, this will be
+    // too high.
+    int max_bucket_size = 0;
+    int index = 0;
+    Node* const* bucket = table;
+    while (bucket < &table[BASE::_table_size]) {
+      int count = 0;
+      Node* node = *bucket;
+      while (node != nullptr) {
+        count++;
+        node = node->_next;
+      }
+      max_bucket_size = MAX2(count, max_bucket_size);
+      ++bucket;
+    }
+    return max_bucket_size;
+  }
+#endif // ASSERT
 };
 
 #endif // SHARE_UTILITIES_RESIZEABLERESOURCEHASH_HPP

src/hotspot/share/utilities/resourceHash.hpp

@@ -40,12 +40,14 @@ public:
   V _value;
   ResourceHashtableNode* _next;
 
-  ResourceHashtableNode(unsigned hash, K const& key, V const& value) :
-    _hash(hash), _key(key), _value(value), _next(nullptr) {}
+  ResourceHashtableNode(unsigned hash, K const& key, V const& value,
+                        ResourceHashtableNode* next = nullptr) :
+    _hash(hash), _key(key), _value(value), _next(next) {}
 
   // Create a node with a default-constructed value.
-  ResourceHashtableNode(unsigned hash, K const& key) :
-    _hash(hash), _key(key), _value(), _next(nullptr) {}
+  ResourceHashtableNode(unsigned hash, K const& key,
+                        ResourceHashtableNode* next = nullptr) :
+    _hash(hash), _key(key), _value(), _next(next) {}
 };
 
 template<
@@ -136,6 +138,29 @@ class ResourceHashtableBase : public STORAGE {
     }
   }
 
+ /**
+  * Inserts a value in the front of the table, assuming that
+  * the entry is absent.
+  * The table must be locked for the get or test that the entry
+  * is absent, and for this operation.
+  * This is a faster variant of put_if_absent because it adds to the
+  * head of the bucket, and doesn't search the bucket.
+  * @return: true: a new item is always added
+  */
+  bool put_when_absent(K const& key, V const& value) {
+    unsigned hv = HASH(key);
+    unsigned index = hv % table_size();
+    assert(*lookup_node(hv, key) == nullptr, "use put_if_absent");
+    Node** ptr = bucket_at(index);
+    if (ALLOC_TYPE == AnyObj::C_HEAP) {
+      *ptr = new (MEM_TYPE) Node(hv, key, value, *ptr);
+    } else {
+      *ptr = new Node(hv, key, value, *ptr);
+    }
+    _number_of_entries ++;
+    return true;
+  }
+
  /**
   * Inserts or replaces a value in the table.
   * @return: true:  if a new item is added
@@ -202,14 +227,15 @@ class ResourceHashtableBase : public STORAGE {
     return &(*ptr)->_value;
   }
 
-
-  bool remove(K const& key) {
+  template<typename Function>
+  bool remove(K const& key, Function function) {
     unsigned hv = HASH(key);
     Node** ptr = lookup_node(hv, key);
 
     Node* node = *ptr;
     if (node != nullptr) {
       *ptr = node->_next;
+      function(node->_key, node->_value);
       if (ALLOC_TYPE == AnyObj::C_HEAP) {
         delete node;
       }
@@ -219,6 +245,11 @@ class ResourceHashtableBase : public STORAGE {
     return false;
   }
 
+  bool remove(K const& key) {
+    auto dummy = [&] (K& k, V& v) { };
+    return remove(key, dummy);
+  }
+
   // ITER contains bool do_entry(K const&, V const&), which will be
   // called for each entry in the table.  If do_entry() returns false,
   // the iteration is cancelled.

test/hotspot/jtreg/serviceability/jvmti/SetTag/TagMapTest.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8306843
+ * @summary Test that 10M tags doesn't time out.
+ * @requires vm.jvmti
+ * @run main/othervm/native -agentlib:TagMapTest
+ *                          -Xlog:jvmti+table
+ *                          TagMapTest
+ */
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+public class TagMapTest {
+    private static final List<TagMapTest> items = new ArrayList<>();
+
+    private static native void setTag(Object object);
+    private static native long getTag(Object object);
+    private static native void iterate(boolean tagged);
+
+    public static void main(String[] args) {
+        System.loadLibrary("TagMapTest");
+        for (int i = 0; i < 10_000_000; i++) {
+            items.add(new TagMapTest());
+        }
+
+        long startTime = System.nanoTime();
+        for (TagMapTest item : items) {
+            setTag(item);
+        }
+        System.out.println("setTag: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) + " ms");
+
+        startTime = System.nanoTime();
+        for (TagMapTest item : items) {
+            getTag(item);
+        }
+        System.out.println("getTag: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) + " ms");
+
+        startTime = System.nanoTime();
+        iterate(true);
+        System.out.println("iterate tagged: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) + " ms");
+
+        startTime = System.nanoTime();
+        iterate(false);
+        System.out.println("iterate all: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) + " ms");
+    }
+}

test/hotspot/jtreg/serviceability/jvmti/SetTag/libTagMapTest.cpp

@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <jvmti.h>
+#include <cstdlib>
+#include <cstring>
+
+namespace {
+    jlong nextTag = 1;
+    jvmtiEnv *jvmti = NULL;
+
+    void checkJvmti(int code, const char* message) {
+        if (code != JVMTI_ERROR_NONE) {
+            printf("Error %s: %d\n", message, code);
+            abort();
+        }
+    }
+
+    jvmtiIterationControl JNICALL heapObjectCallback(jlong class_tag, jlong size, jlong* tag_ptr, void* user_data) {
+        if (*tag_ptr == 0) {
+            *tag_ptr = nextTag++;
+        }
+        return JVMTI_ITERATION_CONTINUE;
+    }
+}
+
+extern "C" JNIEXPORT void JNICALL Java_TagMapTest_setTag(JNIEnv* jni_env, jclass clazz, jobject object) {
+    checkJvmti(jvmti->SetTag(object, nextTag++), "could not set tag");
+}
+
+extern "C" JNIEXPORT jlong JNICALL Java_TagMapTest_getTag(JNIEnv* jni_env, jclass clazz, jobject object) {
+    jlong tag;
+    checkJvmti(jvmti->GetTag(object, &tag), "could not get tag");
+    return tag;
+}
+
+extern "C" JNIEXPORT void JNICALL Java_TagMapTest_iterate(JNIEnv* jni_env, jclass clazz, jboolean tagged) {
+    checkJvmti(jvmti->IterateOverHeap(tagged ? JVMTI_HEAP_OBJECT_TAGGED : JVMTI_HEAP_OBJECT_EITHER, &heapObjectCallback, NULL), "could not iterate");
+}
+
+extern "C" JNIEXPORT jint JNICALL Agent_OnLoad(JavaVM *vm, char *options, void *reserved) {
+    if (vm->GetEnv(reinterpret_cast<void **>(&jvmti), JVMTI_VERSION) != JNI_OK || !jvmti) {
+        printf("Could not initialize JVMTI\n");
+        abort();
+    }
+    jvmtiCapabilities capabilities;
+    memset(&capabilities, 0, sizeof(capabilities));
+    capabilities.can_tag_objects = 1;
+    checkJvmti(jvmti->AddCapabilities(&capabilities), "adding capabilities");
+    printf("Loaded agent\n");
+    fflush(stdout);
+    return JVMTI_ERROR_NONE;
+}
+