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

Reply via email to