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

Reply via email to