RFR: 8212879: Make JVMTI TagMap table not hash on oop address

[Stefan Karlsson]

On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change turns the HashTable that JVMTI uses for object tagging into a regular Hotspot hashtable - the one in hashtable.hpp with resizing and rehashing.   Instead of pointing directly to oops so that GC has to walk the table to follow oops and then to rehash the table, this table points to WeakHandle.  GC walks the backing OopStorages concurrently.
> 
> The hash function for the table is a hash of the lower 32 bits of the address.  A flag is set during GC (gc_notification if in a safepoint, and through a call to JvmtiTagMap::needs_processing()) so that the table is rehashed at the next use.
> 
> The gc_notification mechanism of weak oop processing is used to notify Jvmti to post ObjectFree events.  In concurrent GCs there can be a window of time between weak oop marking where the oop is unmarked, so dead (the phantom load in peek returns NULL) but the gc_notification hasn't been done yet.  In this window, a heap walk or GetObjectsWithTags call would not find an object before the ObjectFree event is posted.  This is dealt with in two ways:
> 
> 1. In the Heap walk, there's an unconditional table walk to post events if events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is required, we use the VM thread to post the event.
> 
> Event posting cannot be done in a JavaThread because the posting needs to be done while holding the table lock, so that the JvmtiEnv state doesn't change before posting is done.  ObjectFree callbacks are limited in what they can do as per the JVMTI Specification.  The allowed callbacks to the VM already have code to allow NonJava threads.
> 
> To avoid rehashing, I also tried to use object->identity_hash() but this breaks because entries can be added to the table during heapwalk, where the objects use marking.  The starting markWord is saved and restored.  Adding a hashcode during this operation makes restoring the former markWord (locked, inflated, etc) too complicated.  Plus we don't want all these objects to have hashcodes because locking operations after tagging would have to always use inflated locks.
> 
> Much of this change is to remove serial weak oop processing for the weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with jvmti code.
> 
> It has also been tested with tier1-6.
> 
> Thank you to Stefan, Erik and Kim for their help with this change.

Commented on nits, and reviewed GC code and tag map code. Didn't look closely on the hashmap changes.

src/hotspot/share/gc/shared/oopStorageSet.hpp line 41:

> 39:   // Must be updated when new OopStorages are introduced
> 40:   static const uint strong_count = 4 JVMTI_ONLY(+ 1);
> 41:   static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1);

All other uses `+ 1` instead of `+1`.

src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp line 49:

> 47:   double _phase_times_sec[1];
> 48:   size_t _phase_dead_items[1];
> 49:   size_t _phase_total_items[1];

This should be removed and the associated reset_items

src/hotspot/share/gc/z/zOopClosures.hpp line 64:

> 62: };
> 63: 
> 64: class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure {

Seems like you flipped the location of these two. Maybe revert?

src/hotspot/share/prims/jvmtiExport.hpp line 405:

> 403: 
> 404:   // Delete me and all my callers!
> 405:   static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {}

Maybe delete?

src/hotspot/share/prims/jvmtiTagMap.cpp line 126:

> 124:   // concurrent GCs. So fix it here once we have a lock or are
> 125:   // at a safepoint.
> 126:   // SetTag and GetTag should not post events!

I think it would be good to explain why. Otherwise, this just leaves the readers wondering why this is the case.

src/hotspot/share/prims/jvmtiTagMap.cpp line 131:

> 129:   // Operating on the hashmap must always be locked, since concurrent GC threads may
> 130:   // notify while running through a safepoint.
> 131:   assert(is_locked(), "checking");

Maybe move this to the top of the function to make it very clear.

src/hotspot/share/prims/jvmtiTagMap.cpp line 133:

> 131:   assert(is_locked(), "checking");
> 132:   if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
> 133:     log_info(jvmti, table)("TagMap table needs posting before heap walk");

Not sure about the "before heap walk" since this is also done from GetObjectsWithTags, which does *not* do a heap walk but still requires posting.

src/hotspot/share/prims/jvmtiTagMap.cpp line 140:

> 138:     hashmap()->rehash();
> 139:     _needs_rehashing = false;
> 140:   }

It's not clear to me that it's correct to rehash *after* posting. I think it is, because unlink_and_post will use load barriers to fixup old pointers.

src/hotspot/share/prims/jvmtiTagMap.cpp line 146:

> 144: // The ZDriver may be walking the hashmaps concurrently so all these locks are needed.
> 145: void JvmtiTagMap::check_hashmaps_for_heapwalk() {
> 146: 

Extra white space. (Also double whitespace after this function)

src/hotspot/share/prims/jvmtiTagMap.cpp line 144:

> 142: 
> 143: // This checks for posting and rehashing and is called from the heap walks.
> 144: // The ZDriver may be walking the hashmaps concurrently so all these locks are needed.

Should this comment be moved down to the lock taking?

src/hotspot/share/prims/jvmtiTagMap.cpp line 377:

> 375:   MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag);
> 376: 
> 377:   // Check if we have to processing for concurrent GCs.

Sentence seems to be missing a few words.

src/hotspot/share/prims/jvmtiTagMap.cpp line 954:

> 952:                          o->klass()->external_name());
> 953:     return;
> 954:   }

Why is this done as a part of this RFE? Is this a bug fix that should be done as a separate patch?

src/hotspot/share/prims/jvmtiTagMap.cpp line 1152:

> 1150: void JvmtiTagMap::unlink_and_post_locked() {
> 1151:   MutexLocker ml(lock(), Mutex::_no_safepoint_check_flag);
> 1152:   log_info(jvmti, table)("TagMap table needs posting before GetObjectTags");

There's no function called GetObjectTags. This log line needs to be adjusted.

src/hotspot/share/prims/jvmtiTagMap.cpp line 1162:

> 1160:   VMOp_Type type() const { return VMOp_Cleanup; }
> 1161:   void doit() {
> 1162:     _tag_map->unlink_and_post_locked();

Either inline unlink_and_post_locked() or updated gc_notification to use it?

src/hotspot/share/prims/jvmtiTagMap.cpp line 1279:

> 1277:     // Can't post ObjectFree events here from a JavaThread, so this
> 1278:     // will race with the gc_notification thread in the tiny
> 1279:     // window where the oop is not marked but hasn't been notified that

Please don't use "oop" when referring to "objects".

src/hotspot/share/prims/jvmtiTagMap.cpp line 2975:

> 2973: }
> 2974: 
> 2975: // Concurrent GC needs to call this in relocation pause, so after the oops are moved

oops => objects

src/hotspot/share/prims/jvmtiTagMap.cpp line 2977:

> 2975: // Concurrent GC needs to call this in relocation pause, so after the oops are moved
> 2976: // and have their new addresses, the table can be rehashed.
> 2977: void JvmtiTagMap::set_needs_processing() {

Maybe rename to set_needs_rehashing?

src/hotspot/share/prims/jvmtiTagMap.cpp line 2985:

> 2983: 
> 2984:   JvmtiEnvIterator it;
> 2985:   for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {

The iterator seems fast enough, so it seems unnecessary to have the environments_might_exist check.

src/hotspot/share/prims/jvmtiTagMap.cpp line 2998:

> 2996:   // thread creation and before VMThread creation (1 thread); initial GC
> 2997:   // verification can happen in that window which gets to here.
> 2998:   if (!JvmtiEnv::environments_might_exist()) { return; }

I don't know what this comment is saying, and why the code is needed.

src/hotspot/share/prims/jvmtiTagMap.cpp line 3020:

> 3018:       JvmtiTagMap* tag_map = env->tag_map_acquire();
> 3019:       if (tag_map != NULL && !tag_map->is_empty()) {
> 3020:         if (num_dead_entries > 0) {

The other num_dead_entries check for != 0. Maybe use the same in the two branches?

src/hotspot/share/prims/jvmtiTagMap.cpp line 3023:

> 3021:           tag_map->hashmap()->unlink_and_post(tag_map->env());
> 3022:         }
> 3023:         tag_map->_needs_rehashing = true;

Maybe add a small comment why this is deferred.

src/hotspot/share/prims/jvmtiTagMap.hpp line 56:

> 54:   void entry_iterate(JvmtiTagMapEntryClosure* closure);
> 55:   void post_dead_object_on_vm_thread();
> 56:  public:

Looked nicer when there was a blank line before public. Now it looks like public "relates" more to the code before than after.

src/hotspot/share/prims/jvmtiTagMap.hpp line 114:

> 112:   static void check_hashmaps_for_heapwalk();
> 113:   static void set_needs_processing() NOT_JVMTI_RETURN;
> 114:   static void gc_notification(size_t num_dead_entries) NOT_JVMTI_RETURN;

Have you verified that this builds without JVMTI?

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 50:

> 48:   // A subsequent oop_load without AS_NO_KEEPALIVE (the object() accessor)
> 49:   // keeps the oop alive before doing so.
> 50:   return literal().peek();

I'm not sure we should be talking about the low-level Access names. Maybe reword in terms of WeakHandle operations?

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 81:

> 79: void JvmtiTagMapTable::free_entry(JvmtiTagMapEntry* entry) {
> 80:   unlink_entry(entry);
> 81:   entry->literal().release(JvmtiExport::weak_tag_storage()); // release OopStorage

release *to* OopStorage?

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 82:

> 80:   unlink_entry(entry);
> 81:   entry->literal().release(JvmtiExport::weak_tag_storage()); // release OopStorage
> 82:   FREE_C_HEAP_ARRAY(char, entry); // C_Heap free.

Seems excessively redundant:
// C_Heap free.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 98:

> 96: 
> 97:       // The obj is in the table as a target already
> 98:       if (target != NULL && target == obj) {

Wonder if we could assert that obj is not NULL at the entry of this function, and then change this to simply target == obj?

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 122:

> 120:   int index = hash_to_index(hash);
> 121:   // One was added while acquiring the lock
> 122:   JvmtiTagMapEntry* entry = find(index, hash, obj);

Should this be done inside ASSERT?

test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp line 64:

> 62: static jclass klass = NULL;
> 63: static jobject testedObject = NULL;
> 64: const jlong TESTED_TAG_VALUE = (5555555L);

Remove parenthesis?

-------------

PR: https://git.openjdk.java.net/jdk/pull/967