On Fri, 18 Jun 2021 06:08:48 GMT, Denghui Dong <dd...@openjdk.org> wrote:
>> Hi,
>>
>> Could I have a review of this change that merges three vm
>> operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump
>> and signal_thread_entry.
>>
>> `jstack` is a very common command, even in the production environment.
>>
>> In addition to reduce the cost of entering safepoint, I think this patch
>> also could ensure the consistency of the results of VM_PrintThreads and
>> VM_FindDeadlocks.
>
> Denghui Dong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> polish
This looks much better. A few minor code changes requested - and possibly a big
simplification.
Thanks,
David
src/hotspot/share/runtime/os.cpp line 402:
> 400: // the output stream (e.g. tty->flush()) after output. See
> 4803766.
> 401: // Each module also prints an extra carriage return after its
> output.
> 402: VM_PrintThreads op(tty, PrintConcurrentLocks, false, true);
Please comment what the false/true arguments mean e.g.
... false /* no extended info */, true /* print JNI handle info */);
src/hotspot/share/runtime/vmOperation.hpp line 112:
> 110: template(JFROldObject) \
> 111: template(JvmtiPostObjectFree) \
> 112: template(ExtendedPrintThreads)
This change is not needed now.
src/hotspot/share/runtime/vmOperations.hpp line 146:
> 144: bool _print_concurrent_locks;
> 145: bool _print_extended_info;
> 146: bool _print_jni;
Please call this _print_jni_handle_info and update parameter names accordingly.
Though it seems we never pass false here and so always print the JNI handle
info, in which case we can get rid of the field and the parameter.
src/hotspot/share/runtime/vmOperations.hpp line 150:
> 148: VM_PrintThreads()
> 149: : _out(tty), _print_concurrent_locks(PrintConcurrentLocks),
> _print_extended_info(false), _print_jni(false)
> 150: {}
This no-arg constructor is unused now.
src/hotspot/share/runtime/vmOperations.hpp line 151:
> 149: : _out(tty), _print_concurrent_locks(PrintConcurrentLocks),
> _print_extended_info(false), _print_jni(false)
> 150: {}
> 151: VM_PrintThreads(outputStream* out, bool print_concurrent_locks, bool
> print_extended_info, bool print_jni = false)
As far as I can see you don't rely on the default parameter value so it is not
needed.
src/hotspot/share/services/attachListener.cpp line 187:
> 185:
> 186: // thread stacks and JNI global handles
> 187: VM_PrintThreads op1(out, print_concurrent_locks, print_extended_info,
> true);
Please comment what the true argument means.
src/hotspot/share/services/diagnosticCommand.cpp line 538:
> 536: void ThreadDumpDCmd::execute(DCmdSource source, TRAPS) {
> 537: // thread stacks and JNI global handles
> 538: VM_PrintThreads op1(output(), _locks.value(), _extended.value(), true);
Please comment what the true argument means.
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4504
