On Thu, 14 Nov 2024 21:34:08 GMT, Larry Cable <d...@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@4a624db0[Running, parallelism = 16, size = > 2, active = 0, running = 0, steals = 2, tasks = 0, submissions = 0] > > Timeout schedulers: > [0] java.util.concurrent.ScheduledThreadPoolExecutor@1f17ae12[Running, pool > size = 0, active threads = 0, queued tasks = 0, completed tasks = 0] > [1] java.util.concurrent.ScheduledThreadPoolExecutor@6193b845[Running, pool > size = 1, active threads = 0, queued tasks = 1, completed tasks = 0] > [2] java.util.concurrent.ScheduledThreadPoolExecutor@c4437c4[Running, pool > size = 0, active threads = 0, queued tasks = 0, completed tasks = 0] > [3] java.util.concurrent.ScheduledThreadPoolExecutor@3f91beef[Running, pool > size = 0, active threads = 0, queued tasks = 0, completed tasks = 0] > > Read I/O pollers: > [0] sun.nio.ch.KQueuePoller@524bf25 [registered = 1] > > Write I/O pollers: > [0] sun.nio.ch.KQueuePoller@25c41da2 [registered = 0] > > Thread groupings: > <root> [platform threads = 11, virtual threads = 0] > java.util.concurrent.ScheduledThreadPoolExecutor@c4437c4 [platform threads = > 0, virtual threads = 0] > java.util.concurrent.ScheduledThreadPoolExecutor@3f91beef [platform threads = > 0, virtual threads = 0] > ForkJoinPool.commonPool/jdk.internal.vm.SharedThreadContainer@4fa374ea > [platform threads = 0, virtual threads = 0] > java.util.concurrent.ThreadPoolExecutor@506e1b77 [platform threads = 1, > virtual threads = 0] > java.util.concurrent.ScheduledThreadPoolExecutor@1f17ae12 [platform threads = > 0, virtual threads = 0] > java.util.concurrent.ThreadPerTaskExecutor@24155ffc [platform threads = 0, > virtual threads = 2] > ForkJoinPool-1/jdk.internal.vm.SharedThreadContainer@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
