On Mon, 2 Nov 2020 08:08:53 GMT, Stefan Karlsson <stef...@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.
>
> 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`.
Fixed, although I think the space looks strange there but I'll go along.
> 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
Removed.
> 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?
Reverted. There was a rebasing conflict here so this was unintentional.
> 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?
Yes, meant to do that.
> 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.
ok.
> 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.
I don't call check_hashmap for GetObjectsWithTags.
> 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.
I think it's better that the rehashing doesn't encounter null entries and the
WeakHandle.peek() operation is used for both so I hope it would get the same
answer. If not, which seems bad, the last answer should be what we hash on.
> 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?
ok, I also made it singular since Erik pointed out that we don't need the other
lock.
> 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)
? I removed the double whitespace after the function and put the whitespace
here. It needs some whitespace.
// This checks for posting and rehashing and is called from the heap walks.
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;
> 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.
removed the sentence, because non concurrent GCs also defer rehashing to next
use.
> 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?
Because it crashed with my changes and didn't without. I cannot recollect why.
> 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.
GetObjectsWithTags fixed.
> 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?
I thought of trying to share it but one logs and the other doesn't and it only
saves 1 lines of code.
> 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".
fixed.
> 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
fixed.
> 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?
Since I went back and forth about what this function did (it posted events at
one time), I thought the generic _processing name would be better. GC callers
shouldn't really have to know what processing we're doing here. Hopefully it
won't change from rehashing. That's why I like processing.
> 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.
yes, it looks like it does the same thing. I'll remove it.
> 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.
I've spent tons of time trying to understand this comment too. I think gc
verification used to call oops do on the tagmap table. This comments obsolete
now, and I'll remove it.
> 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?
ok.
> 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.
// Later GC code will relocate the oops, so defer rehashing until then.
?
> 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.
ok
> 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?
I will do (might have already done) that. Building non-oracle platforms builds
minimal vm.
> 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?
I'm going to say:
// Just peek at the object without keeping it alive.
> 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?
fixed
> 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?
makes sense.
> 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?
yes.
> 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?
I copied this from some other place that had parenthesis.
-------------
PR: https://git.openjdk.java.net/jdk/pull/967
