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

[Alex Menkov]

On Wed, 31 Jan 2024 04:34:52 GMT, Chris Plummer <cjplummer at openjdk.org> 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*=<0x00000000ffc2ed70>`, 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