RFR: 8212879: Make JVMTI TagMap table concurrent [v5]

[Coleen Phillimore]

On Wed, 4 Nov 2020 05:37:00 GMT, Serguei Spitsyn <sspitsyn at 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