On Wed, 4 Nov 2020 05:37:00 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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(); > } @sspitsyn Thank you for reviewing the code. The gc_notification refactoring is awkward because each table needs a lock and not a global lock. If we find another place in the GCs to call set_needs_rehashing() it might be possible to make gc_notification call two functions with a boolean to decide whether to take the lock. We're still working on @kimbarrett comments so maybe the notification will change to some new thread and be refactored that way if necessary. I fixed the code for your other comments. ------------- PR: https://git.openjdk.java.net/jdk/pull/967