On Wed, 4 Nov 2020 02:15:52 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Code review comments from Kim and Albert. > > Hi Coleen, > > Wow, there are a lot of simplifications and code removal with this fix! > It looks great in general, just some nits below. > I also wanted to suggest renaming the 'set_needs_processing' to > 'set_needs_rehashing'. :) > > src/hotspot/share/prims/jvmtiTagMap.hpp: > > Nit: Would it better to use a plural form 'post_dead_objects_on_vm_thread'? : > `+ void post_dead_object_on_vm_thread();` > > src/hotspot/share/prims/jvmtiTagMap.cpp: > > Nit: It'd be nice to add a short comment before the check_hashmap similar to > L143 also explaining a difference (does check and post just for one env) with > the check_hashmaps_for_heapwalk: > 122 void JvmtiTagMap::check_hashmap(bool post_events) { > . . . > 143 // This checks for posting and rehashing and is called from the heap > walks. > 144 void JvmtiTagMap::check_hashmaps_for_heapwalk() { > > I'm just curious how this fragment was added. Did you get any failures in > testing? : > 1038 // skip if object is a dormant shared object whose mirror hasn't been > loaded > 1039 if (obj != NULL && obj->klass()->java_mirror() == NULL) { > 1040 log_debug(cds, heap)("skipped dormant archived object " > INTPTR_FORMAT " (%s)", p2i(obj), > 1041 obj->klass()->external_name()); > 1042 return; > 1043 } > > Nit: Can we rename this field to something like '_some_dead_found' or > '_dead_found'? : > `1186 bool _some_dead;` > > Nit: The lines 2997-3007 and 3009-3019 do the same but in different contexts. > 2996 if (!is_vm_thread) { > 2997 if (num_dead_entries != 0) { > 2998 JvmtiEnvIterator it; > 2999 for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { > 3000 JvmtiTagMap* tag_map = env->tag_map_acquire(); > 3001 if (tag_map != NULL) { > 3002 // Lock each hashmap from concurrent posting and cleaning > 3003 tag_map->unlink_and_post_locked(); > 3004 } > 3005 } > 3006 // there's another callback for needs_rehashing > 3007 } > 3008 } else { > 3009 assert(SafepointSynchronize::is_at_safepoint(), "must be"); > 3010 JvmtiEnvIterator it; > 3011 for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { > 3012 JvmtiTagMap* tag_map = env->tag_map_acquire(); > 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()); > 3016 } > 3017 // Later GC code will relocate the oops, so defer rehashing > until then. > 3018 tag_map->_needs_rehashing = true; > 3019 } > It feels like it can be refactored/simplified, at least, a little bit. > Is it possible to check and just return if (num_dead_entries == 0)? > If not, then, at least, it can be done the same way (except of locking). > Q: Should the _needs_rehashing be set in both contexts? > > Also, can we have just one (static?) lock for the whole gc_notification (not > per JVMTI env/JvmtiTagMap)? How much do we win by locking per each > env/JvmtiTagMap? Note, that in normal case there is just one agent. It is > very rare to have multiple agents requesting object tagging and ObjectFree > events. It seems, this can be refactored to more simple code with one > function doing work in both contexts. > > src/hotspot/share/utilities/hashtable.cpp: > > Nit: Need space after the '{' : > `+const int _small_table_sizes[] = {107, 1009, 2017, 4049, 5051, 10103, > 20201, 40423 } ;` > > src/hotspot/share/prims/jvmtiTagMapTable.cpp: > > Nit: Extra space after assert: > `119 assert (find(index, hash, obj) == NULL, "shouldn't already be > present");` > > Thanks, > Serguei More about possible refactoring of the JvmtiTagMap::gc_notification(). I'm thinking about something like below: void JvmtiTagMap::unlink_and_post_for_all_envs() { if (num_dead_entries == 0) { return; // nothing to unlink and post } JvmtiEnvIterator it; 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()) { tag_map->unlink_and_post(); } } } void JvmtiTagMap::gc_notification(size_t num_dead_entries) { if (Thread::current()->is_VM_thread()) { assert(SafepointSynchronize::is_at_safepoint(), "must be"); unlink_and_post_for_all_envs(); set_needs_rehashing(); } else { MutexLocker ml(JvmtiTagMap_lock(), Mutex::_no_safepoint_check_flag); unlink_and_post_for_all_envs(); // there's another callback for needs_rehashing } } If we still need a lock per each JvmtiTagMap then it is possible to add this fragment to the unlink_and_post_for_all_envs: bool is_vm_thread = Thread::current()->is_VM_thread() MutexLocker ml(is_vm_thread ? NULL : lock(), Mutex::_no_safepoint_check_flag); Then the code above could look like below: void JvmtiTagMap::unlink_and_post_for_all_envs() { if (num_dead_entries == 0) { return; // nothing to unlink and post } bool is_vm_thread = Thread::current()->is_VM_thread() JvmtiEnvIterator it; 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()) { MutexLocker ml(is_vm_thread ? NULL : lock(), Mutex::_no_safepoint_check_flag); tag_map->unlink_and_post(); } } } void JvmtiTagMap::gc_notification(size_t num_dead_entries) { if (Thread::current()->is_VM_thread()) { assert(SafepointSynchronize::is_at_safepoint(), "must be"); set_needs_rehashing(); } unlink_and_post_for_all_envs(); } ------------- PR: https://git.openjdk.java.net/jdk/pull/967