RFR: 8268857: Merge VM_PrintJNI and VM_PrintThreads and remove the unused field 'is_deadlock' of DeadlockCycle [v5]

[David Holmes]

On Fri, 18 Jun 2021 06:08:48 GMT, Denghui Dong <ddong at 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