From cb4c83e0cd89f737be647e3bc8f336f471e2eebc Mon Sep 17 00:00:00 2001 From: Coleen Phillimore Date: Mon, 2 Nov 2020 10:52:18 -0500 Subject: [PATCH] Code review comments from StefanK. --- src/hotspot/share/gc/shared/oopStorageSet.hpp | 2 +- .../gc/shared/weakProcessorPhaseTimes.cpp | 20 ----------- .../gc/shared/weakProcessorPhaseTimes.hpp | 6 ---- src/hotspot/share/gc/z/zOopClosures.hpp | 4 +-- src/hotspot/share/prims/jvmtiExport.hpp | 3 -- src/hotspot/share/prims/jvmtiTagMap.cpp | 34 ++++++++----------- src/hotspot/share/prims/jvmtiTagMap.hpp | 2 +- src/hotspot/share/prims/jvmtiTagMapTable.cpp | 17 ++++------ .../capability/CM02/cm02t001/cm02t001.cpp | 2 +- 9 files changed, 27 insertions(+), 63 deletions(-) diff --git a/src/hotspot/share/gc/shared/oopStorageSet.hpp b/src/hotspot/share/gc/shared/oopStorageSet.hpp index 1a709ce8d92d8..3c336eae3741f 100644 --- a/src/hotspot/share/gc/shared/oopStorageSet.hpp +++ b/src/hotspot/share/gc/shared/oopStorageSet.hpp @@ -38,7 +38,7 @@ class OopStorageSet : public AllStatic { public: // Must be updated when new OopStorages are introduced static const uint strong_count = 4 JVMTI_ONLY(+ 1); - static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1); + static const uint weak_count = 5 JVMTI_ONLY(+ 1) JFR_ONLY(+ 1); static const uint all_count = strong_count + weak_count; private: diff --git a/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.cpp b/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.cpp index 9ee95e29cb7be..e365f0a6902fb 100644 --- a/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.cpp +++ b/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.cpp @@ -39,23 +39,6 @@ const double uninitialized_time = -1.0; static bool is_initialized_time(double t) { return t >= 0.0; } #endif // ASSERT -static void reset_times(double* times, size_t ntimes) { - for (size_t i = 0; i < ntimes; ++i) { - times[i] = uninitialized_time; - } -} - -static void reset_items(size_t* items, size_t nitems) { - for (size_t i = 0; i < nitems; ++i) { - items[i] = 0; - } -} - -void WeakProcessorPhaseTimes::reset_phase_data() { - reset_times(_phase_times_sec, ARRAY_SIZE(_phase_times_sec)); - reset_items(_phase_dead_items, ARRAY_SIZE(_phase_dead_items)); - reset_items(_phase_total_items, ARRAY_SIZE(_phase_total_items)); -} WeakProcessorPhaseTimes::WeakProcessorPhaseTimes(uint max_threads) : _max_threads(max_threads), @@ -65,8 +48,6 @@ WeakProcessorPhaseTimes::WeakProcessorPhaseTimes(uint max_threads) : { assert(_max_threads > 0, "max_threads must not be zero"); - reset_phase_data(); - WorkerDataArray** wpt = _worker_data; OopStorageSet::Iterator it = OopStorageSet::weak_iterator(); for ( ; !it.is_end(); ++it) { @@ -103,7 +84,6 @@ void WeakProcessorPhaseTimes::set_active_workers(uint n) { void WeakProcessorPhaseTimes::reset() { _active_workers = 0; _total_time_sec = uninitialized_time; - reset_phase_data(); for (size_t i = 0; i < ARRAY_SIZE(_worker_data); ++i) { _worker_data[i]->reset(); } diff --git a/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp b/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp index 2d69805203ec4..a65996f72ae41 100644 --- a/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp +++ b/src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp @@ -43,12 +43,6 @@ class WeakProcessorPhaseTimes : public CHeapObj { // Total time for weak processor. double _total_time_sec; - // why is this an array? - double _phase_times_sec[1]; - size_t _phase_dead_items[1]; - size_t _phase_total_items[1]; - void reset_phase_data(); - // Per-worker times and linked items. static const uint worker_data_count = WeakProcessorPhases::oopstorage_phase_count; WorkerDataArray* _worker_data[worker_data_count]; diff --git a/src/hotspot/share/gc/z/zOopClosures.hpp b/src/hotspot/share/gc/z/zOopClosures.hpp index 2e02678b67681..ce48ea950c153 100644 --- a/src/hotspot/share/gc/z/zOopClosures.hpp +++ b/src/hotspot/share/gc/z/zOopClosures.hpp @@ -53,7 +53,7 @@ class ZPhantomIsAliveObjectClosure : public BoolObjectClosure { virtual bool do_object_b(oop o); }; -class ZPhantomCleanOopClosure : public ZRootsIteratorClosure { +class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure { public: virtual void do_oop(oop* p); virtual void do_oop(narrowOop* p); @@ -61,7 +61,7 @@ class ZPhantomCleanOopClosure : public ZRootsIteratorClosure { virtual ZNMethodEntry nmethod_entry() const; }; -class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure { +class ZPhantomCleanOopClosure : public ZRootsIteratorClosure { public: virtual void do_oop(oop* p); virtual void do_oop(narrowOop* p); diff --git a/src/hotspot/share/prims/jvmtiExport.hpp b/src/hotspot/share/prims/jvmtiExport.hpp index 9a55a00230433..c3e606d90409b 100644 --- a/src/hotspot/share/prims/jvmtiExport.hpp +++ b/src/hotspot/share/prims/jvmtiExport.hpp @@ -401,9 +401,6 @@ class JvmtiExport : public AllStatic { static void cleanup_thread (JavaThread* thread) NOT_JVMTI_RETURN; static void clear_detected_exception (JavaThread* thread) NOT_JVMTI_RETURN; - // Delete me and all my callers! - static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {} - static void transition_pending_onload_raw_monitors() NOT_JVMTI_RETURN; #if INCLUDE_SERVICES diff --git a/src/hotspot/share/prims/jvmtiTagMap.cpp b/src/hotspot/share/prims/jvmtiTagMap.cpp index 0a2ab8457ed9e..db79edd153ac8 100644 --- a/src/hotspot/share/prims/jvmtiTagMap.cpp +++ b/src/hotspot/share/prims/jvmtiTagMap.cpp @@ -120,15 +120,15 @@ bool JvmtiTagMap::is_empty() { } void JvmtiTagMap::check_hashmap(bool post_events) { + assert(is_locked(), "checking"); + // The table cleaning, posting and rehashing can race for // concurrent GCs. So fix it here once we have a lock or are // at a safepoint. - // SetTag and GetTag should not post events! if (is_empty()) { return; } // Operating on the hashmap must always be locked, since concurrent GC threads may // notify while running through a safepoint. - assert(is_locked(), "checking"); if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) { log_info(jvmti, table)("TagMap table needs posting before heap walk"); hashmap()->unlink_and_post(env()); @@ -141,23 +141,22 @@ void JvmtiTagMap::check_hashmap(bool post_events) { } // This checks for posting and rehashing and is called from the heap walks. -// The ZDriver may be walking the hashmaps concurrently so all these locks are needed. void JvmtiTagMap::check_hashmaps_for_heapwalk() { - assert(SafepointSynchronize::is_at_safepoint(), "called from safepoints"); + // Verify that the tag map tables are valid and unconditionally post events // that are expected to be posted before gc_notification. JvmtiEnvIterator it; for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { JvmtiTagMap* tag_map = env->tag_map_acquire(); if (tag_map != NULL) { + // The ZDriver may be walking the hashmaps concurrently so this lock is needed. MutexLocker ml(tag_map->lock(), Mutex::_no_safepoint_check_flag); tag_map->check_hashmap(/*post_events*/ true); } } } - // Return the tag value for an object, or 0 if the object is // not tagged // @@ -341,7 +340,9 @@ class TwoOopCallbackWrapper : public CallbackWrapper { void JvmtiTagMap::set_tag(jobject object, jlong tag) { MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag); - // Check if we have to process for concurrent GCs. + // SetTag should not post events because the JavaThread has to + // transition to native for the callback and this cannot stop for + // safepoints with the hashmap lock held. check_hashmap(false); // resolve the object @@ -374,7 +375,9 @@ void JvmtiTagMap::set_tag(jobject object, jlong tag) { jlong JvmtiTagMap::get_tag(jobject object) { MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag); - // Check if we have to processing for concurrent GCs. + // GetTag should not post events because the JavaThread has to + // transition to native for the callback and this cannot stop for + // safepoints with the hashmap lock held. check_hashmap(false); // resolve the object @@ -1149,7 +1152,7 @@ void JvmtiTagMap::iterate_through_heap(jint heap_filter, void JvmtiTagMap::unlink_and_post_locked() { MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag); - log_info(jvmti, table)("TagMap table needs posting before GetObjectTags"); + log_info(jvmti, table)("TagMap table needs posting before GetObjectsWithTags"); hashmap()->unlink_and_post(env()); } @@ -1276,7 +1279,7 @@ jvmtiError JvmtiTagMap::get_objects_with_tags(const jlong* tags, MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag); // Can't post ObjectFree events here from a JavaThread, so this // will race with the gc_notification thread in the tiny - // window where the oop is not marked but hasn't been notified that + // window where the object is not marked but hasn't been notified that // it is collected yet. entry_iterate(&collector); } @@ -2972,15 +2975,12 @@ void JvmtiTagMap::follow_references(jint heap_filter, VMThread::execute(&op); } -// Concurrent GC needs to call this in relocation pause, so after the oops are moved +// 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_processing() { assert(SafepointSynchronize::is_at_safepoint(), "called in gc pause"); assert(Thread::current()->is_VM_thread(), "should be the VM thread"); - // Hopefully quick exit. - if (!JvmtiEnv::environments_might_exist()) { return; } - JvmtiEnvIterator it; for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { JvmtiTagMap* tag_map = env->tag_map_acquire(); @@ -2992,11 +2992,6 @@ void JvmtiTagMap::set_needs_processing() { void JvmtiTagMap::gc_notification(size_t num_dead_entries) { - // No locks during VM bring-up (0 threads) and no safepoints after main - // thread creation and before VMThread creation (1 thread); initial GC - // verification can happen in that window which gets to here. - if (!JvmtiEnv::environments_might_exist()) { return; } - bool is_vm_thread = Thread::current()->is_VM_thread(); if (!is_vm_thread) { if (num_dead_entries != 0) { @@ -3017,9 +3012,10 @@ void JvmtiTagMap::gc_notification(size_t num_dead_entries) { for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { JvmtiTagMap* tag_map = env->tag_map_acquire(); if (tag_map != NULL && !tag_map->is_empty()) { - if (num_dead_entries > 0) { + if (num_dead_entries != 0) { tag_map->hashmap()->unlink_and_post(tag_map->env()); } + // Later GC code will relocate the oops, so defer rehashing until then. tag_map->_needs_rehashing = true; } } diff --git a/src/hotspot/share/prims/jvmtiTagMap.hpp b/src/hotspot/share/prims/jvmtiTagMap.hpp index 67a4f80649b3d..72791fd8c3d46 100644 --- a/src/hotspot/share/prims/jvmtiTagMap.hpp +++ b/src/hotspot/share/prims/jvmtiTagMap.hpp @@ -53,8 +53,8 @@ class JvmtiTagMap : public CHeapObj { void entry_iterate(JvmtiTagMapEntryClosure* closure); void post_dead_object_on_vm_thread(); - public: + public: // indicates if this tag map is locked bool is_locked() { return lock()->is_locked(); } diff --git a/src/hotspot/share/prims/jvmtiTagMapTable.cpp b/src/hotspot/share/prims/jvmtiTagMapTable.cpp index 769fecb9facdf..e42a5a97c003f 100644 --- a/src/hotspot/share/prims/jvmtiTagMapTable.cpp +++ b/src/hotspot/share/prims/jvmtiTagMapTable.cpp @@ -42,11 +42,7 @@ oop JvmtiTagMapEntry::object() { } oop JvmtiTagMapEntry::object_no_keepalive() { - // The AS_NO_KEEPALIVE peeks at the oop without keeping it alive. - // This is dangerous in general but is okay if the loaded oop does - // not leak out past a thread transition where a safepoint can happen. - // A subsequent oop_load without AS_NO_KEEPALIVE (the object() accessor) - // keeps the oop alive before doing so. + // Just peek at the object without keeping it alive. return literal().peek(); } @@ -78,8 +74,8 @@ JvmtiTagMapEntry* JvmtiTagMapTable::new_entry(unsigned int hash, WeakHandle w, j void JvmtiTagMapTable::free_entry(JvmtiTagMapEntry* entry) { unlink_entry(entry); - entry->literal().release(JvmtiExport::weak_tag_storage()); // release OopStorage - FREE_C_HEAP_ARRAY(char, entry); // C_Heap free. + entry->literal().release(JvmtiExport::weak_tag_storage()); // release to OopStorage + FREE_C_HEAP_ARRAY(char, entry); } unsigned int JvmtiTagMapTable::compute_hash(oop obj) { @@ -88,6 +84,8 @@ unsigned int JvmtiTagMapTable::compute_hash(oop obj) { } JvmtiTagMapEntry* JvmtiTagMapTable::find(int index, unsigned int hash, oop obj) { + assert(obj != NULL, "Cannot search for a NULL object"); + for (JvmtiTagMapEntry* p = bucket(index); p != NULL; p = p->next()) { if (p->hash() == hash) { @@ -95,7 +93,7 @@ JvmtiTagMapEntry* JvmtiTagMapTable::find(int index, unsigned int hash, oop obj) oop target = p->object_no_keepalive(); // The obj is in the table as a target already - if (target != NULL && target == obj) { + if (target == obj) { ResourceMark rm; log_trace(jvmti, table)("JvmtiTagMap entry found for %s index %d", obj->print_value_string(), index); @@ -119,8 +117,7 @@ JvmtiTagMapEntry* JvmtiTagMapTable::add(oop obj, jlong tag) { unsigned int hash = compute_hash(obj); int index = hash_to_index(hash); // One was added while acquiring the lock - JvmtiTagMapEntry* entry = find(index, hash, obj); - assert (entry == NULL, "shouldn't already be present"); + assert (find(index, hash, obj) == NULL, "shouldn't already be present"); // obj was read with AS_NO_KEEPALIVE, or equivalent. // The object needs to be kept alive when it is published. diff --git a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp index 1964cdba98e93..496358bef1f8a 100644 --- a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp +++ b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp @@ -61,7 +61,7 @@ static jlong timeout = 0; static jthread thread = NULL; static jclass klass = NULL; static jobject testedObject = NULL; -const jlong TESTED_TAG_VALUE = (5555555L); +const jlong TESTED_TAG_VALUE = 5555555L; static bool testedObjectNotified = false;