RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]

[Alex Menkov]

On Sun, 28 Jan 2024 01:20:53 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> I noticed that "clhsdb jstack" seemed to hang when I attached to process with a somewhat large heap. It had taken over 10 minutes when I finally decided to have a look at the SA process (using bin/jstack), which came up with the following in the stack:
>> 
>> 
>> at sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent at 23-internal](jdk.hotspot.agent at 23-internal)/ObjectHeap.java:117)
>> at sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent at 23-internal](jdk.hotspot.agent at 23-internal)/ConcurrentLocksPrinter.java:70)
>> at sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.<init>([jdk.hotspot.agent at 23-internal](jdk.hotspot.agent at 23-internal)/ConcurrentLocksPrinter.java:36)
>> at sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent at 23-internal](jdk.hotspot.agent at 23-internal)/StackTrace.java:71)
>> 
>> 
>> This code is meant to print information about j.u.c. locks. It it works by searching the entire java heap for instances of AbstractOwnableSynchronizer. This is a very expensive operation because it means not only does SA need to read in the entire java heap, but it needs to create a Klass mirror for every heap object so it can determine its type.
>> 
>> Our testing doesn't seem to run into this slowness problem because "clhsdb jstack" only iterates over the heap if AbstractOwnableSynchronizer has been loaded, which it won't be if no j.u.c. locks have been created. This seems to be the case wherever we use "clhsdb jstack" in testing. We do have some tests that likely result in j.u.c locks, but they all use "jhsdb jstack", which doesn't have this issue (it requires use of the --locks argument to enable printing of j.u.c locks).
>> 
>> This issue was already called out in [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am proposing that "clhsdb jstack" not include j.u.c lock scanning by default, and add the -l argument to enable it. This will make it similar to bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has a --locks argument (which maps internally to the Jstack.java -l argument).
>> 
>> The same changes are also being made to "clhsdb pstack".
>> 
>> Tested with all tier1, tier2, and tier5 svc tests.
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix spacing

test/hotspot/jtreg/serviceability/sa/ClhsdbJstackWithConcurrentLock.java line 65:

> 63:             for (String line : lines) {
> 64:                 if (line.contains(key)) {
> 65:                     String[] words = line.split(key + "|[, ]");

String.split() expect regexp as an argument
Not sure how this code works (without escaping '$' and parentheses).
I think it would be clearer to use standard regexp pattern/matcher, something like


Pattern pattern = Pattern.compile("\\s+- <0x([0-9a-fA-F]+)>, \(a java/util/concurrent/locks/ReentrantLock\$NonfairSync\)");
for (String line: lines) {
    Matcher matcher = pattern.matcher(line);
    if (matcher.find()) {
        addressString = matcher.group(1);
        break;
    }
}

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1472272773