RFR: 8212879: Make JVMTI TagMap table concurrent [v4]
Kim Barrett
kbarrett at openjdk.java.net
Tue Nov 3 18:12:10 UTC 2020
On Tue, 3 Nov 2020 14:53:12 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.
>
> Coleen Phillimore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into jvmti-table
> - Merge branch 'master' into jvmti-table
> - More review comments from Stefan and ErikO
> - Code review comments from StefanK.
> - 8212879: Make JVMTI TagMap table not hash on oop address
src/hotspot/share/prims/jvmtiTagMap.cpp line 3018:
> 3016: }
> 3017: // Later GC code will relocate the oops, so defer rehashing until then.
> 3018: tag_map->_needs_rehashing = true;
This is wrong for some collectors. I think all collectors ought to be calling set_needs_rehashing in appropriate places, and it can't be be correctly piggybacked on the num-dead callback. (See discussion above for that function.)
For example, G1 remark pause does weak processing (including weak oopstorage) and will call the num-dead callback, but does not move objects, so does not require tagmap rehashing.
(I think CMS oldgen remark may have been similar, for what that's worth.)
src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:
> 3013: if (tag_map != NULL && !tag_map->is_empty()) {
> 3014: if (num_dead_entries != 0) {
> 3015: tag_map->hashmap()->unlink_and_post(tag_map->env());
Why are we doing this in the callback, rather than just setting a flag? I thought part of the point of this change was to get tagmap processing out of GC pauses. The same question applies to the non-safepoint side. The idea was to be lazy about updating the tagmap, waiting until someone actually needed to use it. Or if more prompt ObjectFree notifications are needed then signal some thread (maybe the service thread?) for followup.
src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:
> 2977:
> 2978: // Concurrent GC needs to call this in relocation pause, so after the objects are moved
> 2979: // and have their new addresses, the table can be rehashed.
I think the comment is confusing and wrong. The requirement is that the collector must call this before exposing moved objects to the mutator, and must provide the to-space invariant. (This whole design would not work with the old Shenandoah barriers without additional work. I don't know if tagmaps ever worked at all for them? Maybe they added calls to Access<>::resolve (since happily deceased) to deal with that?) I also think there are a bunch of missing calls; piggybacking on the num-dead callback isn't correct (see later comment about that).
src/hotspot/share/prims/jvmtiTagMap.cpp line 127:
> 125: // The table cleaning, posting and rehashing can race for
> 126: // concurrent GCs. So fix it here once we have a lock or are
> 127: // at a safepoint.
I think this comment and the one below about locking are confused, at least about rehashing. I _think_ this is referring to concurrent num-dead notification? I've already commented there about it being a problem to do the unlink &etc in the GC pause (see later comment). It also seems like a bad idea to be doing this here and block progress by a concurrent GC because we're holding the tagmap lock for a long time, which is another reason to not have the num-dead notification do very much (and not require a lock that might be held here for a long time).
-------------
PR: https://git.openjdk.java.net/jdk/pull/967
More information about the hotspot-dev
mailing list