Skip to content

Commit

Permalink
Code review comments from StefanK.
Browse files Browse the repository at this point in the history
  • Loading branch information
coleenp committed Nov 2, 2020
1 parent 534326d commit cb4c83e
Show file tree
Hide file tree
Showing 9 changed files with 27 additions and 63 deletions.
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/shared/oopStorageSet.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
20 changes: 0 additions & 20 deletions src/hotspot/share/gc/shared/weakProcessorPhaseTimes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -65,8 +48,6 @@ WeakProcessorPhaseTimes::WeakProcessorPhaseTimes(uint max_threads) :
{
assert(_max_threads > 0, "max_threads must not be zero");

reset_phase_data();

WorkerDataArray<double>** wpt = _worker_data;
OopStorageSet::Iterator it = OopStorageSet::weak_iterator();
for ( ; !it.is_end(); ++it) {
Expand Down Expand Up @@ -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();
}
Expand Down
6 changes: 0 additions & 6 deletions src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,6 @@ class WeakProcessorPhaseTimes : public CHeapObj<mtGC> {
// 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<double>* _worker_data[worker_data_count];
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/gc/z/zOopClosures.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ 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);

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);
Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/prims/jvmtiExport.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 15 additions & 19 deletions src/hotspot/share/prims/jvmtiTagMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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
//
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand All @@ -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) {
Expand All @@ -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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/prims/jvmtiTagMap.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ class JvmtiTagMap : public CHeapObj<mtInternal> {

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(); }

Expand Down
17 changes: 7 additions & 10 deletions src/hotspot/share/prims/jvmtiTagMapTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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) {
Expand All @@ -88,14 +84,16 @@ 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) {

// Peek the object to check if it is the right target.
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);
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;


Expand Down

0 comments on commit cb4c83e

Please sign in to comment.