Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]
On Wed, 31 Jan 2024 04:34:52 GMT, Chris Plummer wrote:
>> 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;
>> }
>> }
>
> I think `key` doesn't really matter when doing the split. `key` is mainly for
> finding the right line. The split is for tokenizing that line into words, and
> the only parts that matters for that are ' ' and the ',' being used as word
> delimiters. I think `key` should just be removed from the regex. I think the
> source of the bug is code copied from ClhsdbInspect.java:
>
> // Escape the token "Method*=" because the split
> method uses
> // a regex, not just a straight String.
> String escapedKey = key.replace("*","\*");
> String[] words = line.split(escapedKey+"|[ ]");
>
> In this case key is `Method*=`. The code works and the escaping and searching
> for `key` is necessary because we are looking for something like
> `Method*=<0xffc2ed70>`, but I think this could have been simplified
> by including `=` in the split pattern rather than all of `escapedKey`.
Well, I see now.
In this case `"[, ]"` separator should work fine.
I still think that use of Matcher is much clear - regexp defines what you are
looking for and extract the value you need. Also there is no need to split
`jstackOutput` into lines - the whole jstack output can be passed to
`Pattern.matcher`.
But I don't force you if you prefer to extract the address manually.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1473643552
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]
On Wed, 31 Jan 2024 03:50:29 GMT, Alex Menkov wrote:
>> 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;
> }
> }
I think `key` doesn't really matter when doing the split. `key` is mainly for
finding the right line. The split is for tokenizing that line into words, and
the only parts that matters for that are ' ' and the ',' being used as word
delimiters. I think `key` should just be removed from the regex. I think the
source of the bug is code copied from ClhsdbInspect.java:
// Escape the token "Method*=" because the split method
uses
// a regex, not just a straight String.
String escapedKey = key.replace("*","\*");
String[] words = line.split(escapedKey+"|[ ]");
In this case key is `Method*=`. The code works and the escaping and searching
for `key` is necessary because we are looking for something like
`Method*=<0xffc2ed70>`, but I think this could have been simplified by
including `=` in the split pattern rather than all of `escapedKey`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17479#discussion_r1472293801
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]
On Sun, 28 Jan 2024 01:20:53 GMT, Chris Plummer 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@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117)
>> at
>> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70)
>> at
>> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36)
>> at
>> sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@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
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]
On Sun, 28 Jan 2024 01:20:53 GMT, Chris Plummer 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@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) >> at >> sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@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 Thanks for the review Kevin. Can I get one more review please? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17479#issuecomment-1917767379
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v3]
> 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@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) > at > sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) > at > sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) > at > sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@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 - Changes: - all: https://git.openjdk.org/jdk/pull/17479/files - new: https://git.openjdk.org/jdk/pull/17479/files/814739bb..6c24a9a2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17479=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17479=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17479.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17479/head:pull/17479 PR: https://git.openjdk.org/jdk/pull/17479
