8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow #17479
Conversation
|
👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into |
openjdk
bot
changed the title
|
@plummercj The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
This seems to be a change that will impact the end user of these tools. Instead of disabling j.u.c.locks by default and proving an option to turn them on, you should provide an option to turn them off, and our tests can use that. What you proposed makes sense for an initial design of the tool, but not for a change after the fact.
|
@dholmes-ora I understand your point. There are pros and cons to both approaches, and I think that needs to be considered. One advantage of my approach is preventing users from accidentally stumbling into this is issue, and wondering why jstack won't complete. I think this is more likely than being confused by the change (expecting locks and not seeing them). Another advantage is consistency with bin/jstack and "jhsdb jstack" in that the locks are not included by default. |
|
You will need a Release Note to document the change. I also wonder if we should be looking at a different way to track j.u.c lock usage, as searching the full heap does not seem at all reasonable. |
Good question. It looks like bin/jstack does the same, but there is much less cost to doing this within hotspot than with using SA: |
|
Ping! I still need 2 reviews. Thanks! |
| break; | ||
| } | ||
| } | ||
| if (addressString != null) |
There was a problem hiding this comment.
Did we break already, is this if not needed?
There was a problem hiding this comment.
There are two nested loops. If we did not break out of the inner loop (which is scanning words in the given line), then addressString might still be null and we need to search the next line. If we don't do this check and break here, then we will always search the next line even if addressString was found.
| if (line.contains(key)) { | ||
| String[] words = line.split(key + "|[, ]"); | ||
| for (String word : words) { | ||
| word = word.replace("<","").replace(">",""); |
There was a problem hiding this comment.
| word = word.replace("<","").replace(">",""); | |
| word = word.replace("<", "").replace(">", ""); |
|
Thanks for the review Kevin. Can I get one more review please? Thanks! |
| String addressString = null; | ||
| for (String line : lines) { | ||
| if (line.contains(key)) { | ||
| String[] words = line.split(key + "|[, ]"); |
There was a problem hiding this comment.
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;
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@plummercj This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 149 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks for the reviews Kevin and Alex! /integrate |
|
Going to push as commit 192349e.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@plummercj Pushed as commit 192349e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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:
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. 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.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17479/head:pull/17479$ git checkout pull/17479Update a local copy of the PR:
$ git checkout pull/17479$ git pull https://git.openjdk.org/jdk.git pull/17479/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17479View PR using the GUI difftool:
$ git pr show -t 17479Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17479.diff
Webrev
Link to Webrev Comment