On Wed, 4 Nov 2020 05:37:00 GMT, Serguei Spitsyn <[email protected]> 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
