On Wed, 4 Nov 2020 02:15:52 GMT, Serguei Spitsyn <[email protected]> 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
