8256072: Eliminate JVMTI tagmap rehashing

coleenp · coleenp · commit 94eb25a4f1ff · 2022-11-03T17:27:00.000Z
Reviewed-by: kbarrett, eosterlund
diff --git a/src/hotspot/share/gc/shared/collectedHeap.cpp b/src/hotspot/share/gc/shared/collectedHeap.cpp
@@ -652,11 +652,6 @@ bool CollectedHeap::is_archived_object(oop object) const {
   return false;
 }
 
-uint32_t CollectedHeap::hash_oop(oop obj) const {
-  const uintptr_t addr = cast_from_oop<uintptr_t>(obj);
-  return static_cast<uint32_t>(addr >> LogMinObjAlignment);
-}
-
 // It's the caller's responsibility to ensure glitch-freedom
 // (if required).
 void CollectedHeap::update_capacity_and_used_at_gc() {
diff --git a/src/hotspot/share/gc/shared/collectedHeap.hpp b/src/hotspot/share/gc/shared/collectedHeap.hpp
@@ -280,8 +280,6 @@ class CollectedHeap : public CHeapObj<mtGC> {
 
   DEBUG_ONLY(bool is_in_or_null(const void* p) const { return p == NULL || is_in(p); })
 
-  virtual uint32_t hash_oop(oop obj) const;
-
   void set_gc_cause(GCCause::Cause v);
   GCCause::Cause gc_cause() { return _gc_cause; }
 
diff --git a/src/hotspot/share/gc/shared/weakProcessor.cpp b/src/hotspot/share/gc/shared/weakProcessor.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2022, 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
@@ -48,13 +48,6 @@ void notify_jvmti_tagmaps() {
   // the tagmap's oopstorage notification handler to not care whether it's
   // invoked by STW or concurrent reference processing.
   JvmtiTagMap::set_needs_cleaning();
-
-  // Notify JVMTI tagmaps that a STW collection may have moved objects, so
-  // the tagmaps need rehashing.  This isn't the right place for this, but
-  // is convenient because all the STW collectors use WeakProcessor.  One
-  // problem is that the end of a G1 concurrent collection also comes here,
-  // possibly triggering unnecessary rehashes.
-  JvmtiTagMap::set_needs_rehashing();
 #endif // INCLUDE_JVMTI
 }
 
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp
@@ -606,9 +606,6 @@ void ShenandoahConcurrentGC::op_final_mark() {
       ShenandoahCodeRoots::arm_nmethods();
       ShenandoahStackWatermark::change_epoch_id();
 
-      // Notify JVMTI that oops are changed.
-      JvmtiTagMap::set_needs_rehashing();
-
       if (ShenandoahPacing) {
         heap->pacer()->setup_for_evac();
       }
diff --git a/src/hotspot/share/gc/z/zCollectedHeap.cpp b/src/hotspot/share/gc/z/zCollectedHeap.cpp
@@ -145,10 +145,6 @@ bool ZCollectedHeap::requires_barriers(stackChunkOop obj) const {
   return false;
 }
 
-uint32_t ZCollectedHeap::hash_oop(oop obj) const {
-  return _heap.hash_oop(ZOop::to_address(obj));
-}
-
 HeapWord* ZCollectedHeap::allocate_new_tlab(size_t min_size, size_t requested_size, size_t* actual_size) {
   const size_t size_in_bytes = ZUtils::words_to_bytes(align_object_size(requested_size));
   const uintptr_t addr = _heap.alloc_tlab(size_in_bytes);
diff --git a/src/hotspot/share/gc/z/zCollectedHeap.hpp b/src/hotspot/share/gc/z/zCollectedHeap.hpp
@@ -74,8 +74,6 @@ class ZCollectedHeap : public CollectedHeap {
   virtual bool is_in(const void* p) const;
   virtual bool requires_barriers(stackChunkOop obj) const;
 
-  virtual uint32_t hash_oop(oop obj) const;
-
   virtual oop array_allocate(Klass* klass, size_t size, int length, bool do_zero, TRAPS);
   virtual HeapWord* mem_allocate(size_t size, bool* gc_overhead_limit_was_exceeded);
   virtual MetaWord* satisfy_failed_metadata_allocation(ClassLoaderData* loader_data,
diff --git a/src/hotspot/share/gc/z/zHeap.cpp b/src/hotspot/share/gc/z/zHeap.cpp
@@ -436,9 +436,6 @@ void ZHeap::relocate_start() {
 
   // Update statistics
   ZStatHeap::set_at_relocate_start(_page_allocator.stats());
-
-  // Notify JVMTI
-  JvmtiTagMap::set_needs_rehashing();
 }
 
 void ZHeap::relocate() {
diff --git a/src/hotspot/share/gc/z/zHeap.hpp b/src/hotspot/share/gc/z/zHeap.hpp
@@ -90,7 +90,6 @@ class ZHeap {
   size_t unsafe_max_tlab_alloc() const;
 
   bool is_in(uintptr_t addr) const;
-  uint32_t hash_oop(uintptr_t addr) const;
 
   // Threads
   uint active_workers() const;
diff --git a/src/hotspot/share/gc/z/zHeap.inline.hpp b/src/hotspot/share/gc/z/zHeap.inline.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2022, 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
@@ -28,7 +28,6 @@
 
 #include "gc/z/zAddress.inline.hpp"
 #include "gc/z/zForwardingTable.inline.hpp"
-#include "gc/z/zHash.inline.hpp"
 #include "gc/z/zMark.inline.hpp"
 #include "gc/z/zPage.inline.hpp"
 #include "gc/z/zPageTable.inline.hpp"
@@ -43,11 +42,6 @@ inline ReferenceDiscoverer* ZHeap::reference_discoverer() {
   return &_reference_processor;
 }
 
-inline uint32_t ZHeap::hash_oop(uintptr_t addr) const {
-  const uintptr_t offset = ZAddress::offset(addr);
-  return ZHash::address_to_uint32(offset);
-}
-
 inline bool ZHeap::is_object_live(uintptr_t addr) const {
   ZPage* page = _page_table.get(addr);
   return page->is_object_live(addr);
diff --git a/src/hotspot/share/oops/oop.cpp b/src/hotspot/share/oops/oop.cpp
@@ -99,10 +99,7 @@ void oopDesc::verify(oopDesc* oop_desc) {
 intptr_t oopDesc::slow_identity_hash() {
   // slow case; we have to acquire the micro lock in order to locate the header
   Thread* current = Thread::current();
-  ResetNoHandleMark rnm; // Might be called from LEAF/QUICK ENTRY
-  HandleMark hm(current);
-  Handle object(current, this);
-  return ObjectSynchronizer::identity_hash_value_for(object);
+  return ObjectSynchronizer::FastHashCode(current, this);
 }
 
 // used only for asserts and guarantees
diff --git a/src/hotspot/share/oops/oop.hpp b/src/hotspot/share/oops/oop.hpp
@@ -293,6 +293,7 @@ class oopDesc {
   // identity hash; returns the identity hash key (computes it if necessary)
   inline intptr_t identity_hash();
   intptr_t slow_identity_hash();
+  inline bool fast_no_hash_check();
 
   // marks are forwarded to stack when object is locked
   inline bool     has_displaced_mark() const;
diff --git a/src/hotspot/share/oops/oop.inline.hpp b/src/hotspot/share/oops/oop.inline.hpp
@@ -356,6 +356,14 @@ intptr_t oopDesc::identity_hash() {
   }
 }
 
+// This checks fast simple case of whether the oop has_no_hash,
+// to optimize JVMTI table lookup.
+bool oopDesc::fast_no_hash_check() {
+  markWord mrk = mark_acquire();
+  assert(!mrk.is_marked(), "should never be marked");
+  return mrk.is_unlocked() && mrk.has_no_hash();
+}
+
 bool oopDesc::has_displaced_mark() const {
   return mark().has_displaced_mark_helper();
 }
diff --git a/src/hotspot/share/prims/jvmtiTagMap.cpp b/src/hotspot/share/prims/jvmtiTagMap.cpp
@@ -76,7 +76,6 @@ bool JvmtiTagMap::_has_object_free_events = false;
 JvmtiTagMap::JvmtiTagMap(JvmtiEnv* env) :
   _env(env),
   _lock(Mutex::nosafepoint, "JvmtiTagMap_lock"),
-  _needs_rehashing(false),
   _needs_cleaning(false),
   _posting_events(false) {
 
@@ -136,7 +135,7 @@ bool JvmtiTagMap::is_empty() {
   return hashmap()->is_empty();
 }
 
-// This checks for posting and rehashing before operations that
+// This checks for posting before operations that use
 // this tagmap table.
 void JvmtiTagMap::check_hashmap(GrowableArray<jlong>* objects) {
   assert(is_locked(), "checking");
@@ -148,14 +147,9 @@ void JvmtiTagMap::check_hashmap(GrowableArray<jlong>* objects) {
       env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
     remove_dead_entries_locked(objects);
   }
-  if (_needs_rehashing) {
-    log_info(jvmti, table)("TagMap table needs rehashing");
-    hashmap()->rehash();
-    _needs_rehashing = false;
-  }
 }
 
-// This checks for posting and rehashing and is called from the heap walks.
+// This checks for posting and is called from the heap walks.
 void JvmtiTagMap::check_hashmaps_for_heapwalk(GrowableArray<jlong>* objects) {
   assert(SafepointSynchronize::is_at_safepoint(), "called from safepoints");
 
@@ -2932,21 +2926,6 @@ void JvmtiTagMap::follow_references(jint heap_filter,
   post_dead_objects(&dead_objects);
 }
 
-// Concurrent GC needs to call this in relocation pause, so after the objects are moved
-// and have their new addresses, the table can be rehashed.
-void JvmtiTagMap::set_needs_rehashing() {
-  assert(SafepointSynchronize::is_at_safepoint(), "called in gc pause");
-  assert(Thread::current()->is_VM_thread(), "should be the VM thread");
-
-  JvmtiEnvIterator it;
-  for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
-    JvmtiTagMap* tag_map = env->tag_map_acquire();
-    if (tag_map != NULL) {
-      tag_map->_needs_rehashing = true;
-    }
-  }
-}
-
 // Verify gc_notification follows set_needs_cleaning.
 DEBUG_ONLY(static bool notified_needs_cleaning = false;)
 
diff --git a/src/hotspot/share/prims/jvmtiTagMap.hpp b/src/hotspot/share/prims/jvmtiTagMap.hpp
@@ -40,7 +40,6 @@ class JvmtiTagMap :  public CHeapObj<mtInternal> {
   JvmtiEnv*             _env;                       // the jvmti environment
   Monitor               _lock;                      // lock for this tag map
   JvmtiTagMapTable*     _hashmap;                   // the hashmap for tags
-  bool                  _needs_rehashing;
   bool                  _needs_cleaning;
   bool                  _posting_events;
 
@@ -115,7 +114,6 @@ class JvmtiTagMap :  public CHeapObj<mtInternal> {
   void post_dead_objects(GrowableArray<jlong>* const objects);
 
   static void check_hashmaps_for_heapwalk(GrowableArray<jlong>* objects);
-  static void set_needs_rehashing() NOT_JVMTI_RETURN;
   static void set_needs_cleaning() NOT_JVMTI_RETURN;
   static void gc_notification(size_t num_dead_entries) NOT_JVMTI_RETURN;
 
diff --git a/src/hotspot/share/prims/jvmtiTagMapTable.cpp b/src/hotspot/share/prims/jvmtiTagMapTable.cpp
@@ -85,7 +85,7 @@ void JvmtiTagMapTable::free_entry(JvmtiTagMapEntry* entry) {
 
 unsigned int JvmtiTagMapTable::compute_hash(oop obj) {
   assert(obj != NULL, "obj is null");
-  return Universe::heap()->hash_oop(obj);
+  return obj->identity_hash();
 }
 
 JvmtiTagMapEntry* JvmtiTagMapTable::find(int index, unsigned int hash, oop obj) {
@@ -113,6 +113,10 @@ JvmtiTagMapEntry* JvmtiTagMapTable::find(int index, unsigned int hash, oop obj)
 }
 
 JvmtiTagMapEntry* JvmtiTagMapTable::find(oop obj) {
+  if (obj->fast_no_hash_check()) {
+    // Objects in the table all have a hashcode.
+    return NULL;
+  }
   unsigned int hash = compute_hash(obj);
   int index = hash_to_index(hash);
   return find(index, hash, obj);
@@ -220,48 +224,3 @@ void JvmtiTagMapTable::remove_dead_entries(GrowableArray<jlong>* objects) {
   log_info(jvmti, table) ("JvmtiTagMap entries counted %d removed %d",
                           oops_counted, oops_removed);
 }
-
-// Rehash oops in the table
-void JvmtiTagMapTable::rehash() {
-  ResourceMark rm;
-  GrowableArray<JvmtiTagMapEntry*> moved_entries;
-
-  int oops_counted = 0;
-  for (int i = 0; i < table_size(); ++i) {
-    JvmtiTagMapEntry** p = bucket_addr(i);
-    JvmtiTagMapEntry* entry = bucket(i);
-    while (entry != NULL) {
-      oops_counted++;
-      oop l = entry->object_no_keepalive();
-      if (l != NULL) {
-        // Check if oop has moved, ie its hashcode is different
-        // than the one entered in the table.
-        unsigned int new_hash = compute_hash(l);
-        if (entry->hash() != new_hash) {
-          *p = entry->next();
-          entry->set_hash(new_hash);
-          unlink_entry(entry);
-          moved_entries.push(entry);
-        } else {
-          p = entry->next_addr();
-        }
-      } else {
-        // Skip removed oops. They may still have to be posted.
-        p = entry->next_addr();
-      }
-      // get next entry
-      entry = *p;
-    }
-  }
-
-  int rehash_len = moved_entries.length();
-  // Now add back in the entries that were removed.
-  for (int i = 0; i < rehash_len; i++) {
-    JvmtiTagMapEntry* moved_entry = moved_entries.at(i);
-    int index = hash_to_index(moved_entry->hash());
-    Hashtable<WeakHandle, mtServiceability>::add_entry(index, moved_entry);
-  }
-
-  log_info(jvmti, table) ("JvmtiTagMap entries counted %d rehashed %d",
-                          oops_counted, rehash_len);
-}
diff --git a/src/hotspot/share/prims/jvmtiTagMapTable.hpp b/src/hotspot/share/prims/jvmtiTagMapTable.hpp
@@ -89,7 +89,6 @@ class JvmtiTagMapTable : public Hashtable<WeakHandle, mtServiceability> {
 
   // Cleanup cleared entries and store dead object tags in objects array
   void remove_dead_entries(GrowableArray<jlong>* objects);
-  void rehash();
   void clear();
 };
 
diff --git a/src/hotspot/share/runtime/synchronizer.cpp b/src/hotspot/share/runtime/synchronizer.cpp
@@ -978,13 +978,6 @@ intptr_t ObjectSynchronizer::FastHashCode(Thread* current, oop obj) {
   }
 }
 
-// Deprecated -- use FastHashCode() instead.
-
-intptr_t ObjectSynchronizer::identity_hash_value_for(Handle obj) {
-  return FastHashCode(Thread::current(), obj());
-}
-
-
 bool ObjectSynchronizer::current_thread_holds_lock(JavaThread* current,
                                                    Handle h_obj) {
   assert(current == JavaThread::current(), "Can only be called on current thread");
diff --git a/src/hotspot/share/runtime/synchronizer.hpp b/src/hotspot/share/runtime/synchronizer.hpp
@@ -178,7 +178,6 @@ class ObjectSynchronizer : AllStatic {
 
   // Returns the identity hash value for an oop
   // NOTE: It may cause monitor inflation
-  static intptr_t identity_hash_value_for(Handle obj);
   static intptr_t FastHashCode(Thread* current, oop obj);
 
   // java.lang.Thread support

Original file line number	Diff line number	Diff line change
`@@ -606,9 +606,6 @@ void ShenandoahConcurrentGC::op_final_mark() {`
`606`	`606`	`ShenandoahCodeRoots::arm_nmethods();`
`607`	`607`	`ShenandoahStackWatermark::change_epoch_id();`
`608`	`608`
`609`		`- // Notify JVMTI that oops are changed.`
`610`		`- JvmtiTagMap::set_needs_rehashing();`
`611`		`-`
`612`	`609`	`if (ShenandoahPacing) {`
`613`	`610`	`heap->pacer()->setup_for_evac();`
`614`	`611`	`}`
Original file line number	Diff line number	Diff line change
`@@ -436,9 +436,6 @@ void ZHeap::relocate_start() {`
`436`	`436`
`437`	`437`	`// Update statistics`
`438`	`438`	`ZStatHeap::set_at_relocate_start(_page_allocator.stats());`
`439`		`-`
`440`		`- // Notify JVMTI`
`441`		`- JvmtiTagMap::set_needs_rehashing();`
`442`	`439`	`}`
`443`	`440`
`444`	`441`	`void ZHeap::relocate() {`