RFR: 8337199: Add jcmd Thread.vthread_summary diagnostic command

Tue Nov 26 01:49:42 UTC 2024

On Thu, 14 Nov 2024 21:34:08 GMT, Larry Cable <duke at openjdk.org> wrote:

> c.f: [https://bugs.openjdk.org/browse/JDK-8339420](https://bugs.openjdk.org/browse/JDK-8339420)
> 
> Summary
> -------
> 
> Add `jcmd <pid> Thread.vthread_summary` to print summary information that is useful when trying to diagnose issues with virtual threads.
> 
> 
> Problem
> -------
> 
> The JDK is lacking tooling to diagnose issues with virtual threads.
> 
> 
> Solution
> --------
> 
> Add a new command that the `jcmd` command line tool can use to print information about virtual threads. The output includes the virtual thread scheduler, the schedulers used to support timeouts, and the I/O pollers used to support virtual threads doing socket I/O, and a summary of the thread groupings.
> 
> Here is sample output. The output is intended for experts and is not intended for automated parsing.
> 
> 
> Virtual thread scheduler:
> java.util.concurrent.ForkJoinPool at 4a624db0[Running, parallelism = 16, size = 2, active = 0, running = 0, steals = 2, tasks = 0, submissions = 0]
> 
> Timeout schedulers:
> [0] java.util.concurrent.ScheduledThreadPoolExecutor at 1f17ae12[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
> [1] java.util.concurrent.ScheduledThreadPoolExecutor at 6193b845[Running, pool size = 1, active threads = 0, queued tasks = 1, completed tasks = 0]
> [2] java.util.concurrent.ScheduledThreadPoolExecutor at c4437c4[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
> [3] java.util.concurrent.ScheduledThreadPoolExecutor at 3f91beef[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
> 
> Read I/O pollers:
> [0] sun.nio.ch.KQueuePoller at 524bf25 [registered = 1]
> 
> Write I/O pollers:
> [0] sun.nio.ch.KQueuePoller at 25c41da2 [registered = 0]
> 
> Thread groupings:
> <root> [platform threads = 11, virtual threads = 0]
> java.util.concurrent.ScheduledThreadPoolExecutor at c4437c4 [platform threads = 0, virtual threads = 0]
> java.util.concurrent.ScheduledThreadPoolExecutor at 3f91beef [platform threads = 0, virtual threads = 0]
> ForkJoinPool.commonPool/jdk.internal.vm.SharedThreadContainer at 4fa374ea [platform threads = 0, virtual threads = 0]
> java.util.concurrent.ThreadPoolExecutor at 506e1b77 [platform threads = 1, virtual threads = 0]
> java.util.concurrent.ScheduledThreadPoolExecutor at 1f17ae12 [platform threads = 0, virtual threads = 0]
> java.util.concurrent.ThreadPerTaskExecutor at 24155ffc [platform threads = 0, virtual threads = 2]
> ForkJoinPool-1/jdk.internal.vm.SharedThreadContainer at 48a03463 [platform threads = 2, virtual threads = 0]
> java.util.concurrent.Scheduled...

Generally looks like a good addition. We need to finalize names and content as per CSR request. A few coding/style issues that I've flagged.

Thanks.

src/hotspot/share/services/diagnosticCommand.cpp line 1124:

> 1122:   Symbol* sym = vmSymbols::jdk_internal_vm_VThreadSummary();
> 1123:   Klass* k = SystemDictionary::resolve_or_fail(sym, true, CHECK);
> 1124:   if (HAS_PENDING_EXCEPTION) {

You can never reach line 1124 if there is a pending exception because the `CHECK` macro will do a return from the method. I see the same problem pre-exists in the other DCmd code. If you want to handle exceptions directly then you want to use `THREAD` not `CHECK`.

src/hotspot/share/services/diagnosticCommand.cpp line 1150:

> 1148:   oop res = cast_to_oop(result.get_jobject());
> 1149:   assert(res->is_typeArray(), "just checking");
> 1150:   assert(TypeArrayKlass::cast(res->klass())->element_type() == T_BYTE, "just checking");

I just called this out in another PR. These assertions are completely unnecessary unless you are trying to check the basic JavaCall operation - which you should not be. You are calling a method that returns a `byte[]` and that is what you will get back.

src/hotspot/share/services/diagnosticCommand.hpp line 434:

> 432:   static const char* name() { return "Thread.print"; }
> 433:   static const char* description() {
> 434:     return "Print all platform threads with stacktraces.";

Probably best not hide this correction in this PR but file a separate issue for it.

src/java.base/share/classes/jdk/internal/vm/VThreadSummary.java line 75:

> 73:     private static void printSchedulers(StringBuilder sb) {
> 74:         sb.append("Virtual thread scheduler:")
> 75:                 .append(System.lineSeparator());

Can't you just do:

sb.append("Virtual thread scheduler:\n")

?

src/java.base/share/classes/jdk/internal/vm/VThreadSummary.java line 77:

> 75:                 .append(System.lineSeparator());
> 76:         sb.append(JLA.virtualThreadDefaultScheduler())
> 77:                 .append(System.lineSeparator());

Stylistically it is common/customary to align the dot operator in chained method calls.

src/java.base/share/classes/jdk/internal/vm/VThreadSummary.java line 106:

> 104:                     .append(masterPoller)
> 105:                     .append(System.lineSeparator());
> 106:             sb.append(System.lineSeparator());

Is this style trying to draw attention to the blank lines between sections? It looks odd to me to not chain the final append as well.

src/java.base/share/classes/sun/nio/ch/Poller.java line 280:

> 278:     public String toString() {
> 279:         return Objects.toIdentityString(this) + " [registered = " + map.size() + "]";
> 280:     }

Why did you move this and "inline" the content of `registered()`?

src/jdk.jcmd/share/man/jcmd.1 line 1:

> 1: .\" Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.

The `.1` nroff files are no longer in the repo, you need to edit the markdown source instead.

test/hotspot/jtreg/serviceability/dcmd/thread/VThreadSummaryTest.java line 74:

> 72:                 .shouldContain(Objects.toIdentityString(defaultScheduler()))
> 73:                 .shouldContain("Timeout schedulers:")
> 74:                 .shouldContain("[0] " + ScheduledThreadPoolExecutor.class.getName());

Again please align dots on chained calls.

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22121#pullrequestreview-2459949901
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857550460
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857551670
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857552627
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857554925
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857554142
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857561627
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857562945
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857563813
PR Review Comment: https://git.openjdk.org/jdk/pull/22121#discussion_r1857564382