Skip to content

8356870: HotSpotDiagnosticMXBean.dumpThreads and jcmd Thread.dump_to_file updates #25429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: pr/25425
Choose a base branch
from

Conversation

AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented May 24, 2025

Updates the thread dump generated by HotSpotDiagnosticMXBean.dumpThreads and jcmd Thread.dump_to_file to include thread state and lock information. Also update the HotSpotDiagnosticMXBean.dumpThreads API description to link to a description of the JSON format dump as that format is intended to be parseable/read by tools.

This PR is dependent on pull/25425. As noted in that PR, the changes accumulated in the loom repo, and have been split up to make it easier to review.

The changes include some re-implementation of ThreadDumper. This is because it used PrintStream and didn't fail if there was an I/O error, e.g. file system full. Furthermore, the indentation to pretty print the json was fragile and hard to maintain so this is changed to use a supporting writer class to do this.

Test coverage is significantly expanded, including updating the test library that is used by several tests to parse the thread dump.

Testing: tier1-6


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8356872 to be approved

Integration blocker

 ⚠️ Dependency #25425 must be integrated first

Issues

  • JDK-8356870: HotSpotDiagnosticMXBean.dumpThreads and jcmd Thread.dump_to_file updates (Enhancement - P3)
  • JDK-8356872: HotSpotDiagnosticMXBean.dumpThreads and jcmd Thread.dump_to_file updates (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25429/head:pull/25429
$ git checkout pull/25429

Update a local copy of the PR:
$ git checkout pull/25429
$ git pull https://git.openjdk.org/jdk.git pull/25429/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25429

View PR using the GUI difftool:
$ git pr show -t 25429

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25429.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 24, 2025

👋 Welcome back alanb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented May 24, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label May 24, 2025
@openjdk
Copy link

openjdk bot commented May 24, 2025

@AlanBateman The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot
  • jmx
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@AlanBateman
Copy link
Contributor Author

/label remove hotspot
/label remove core-libs
/label remove jmx

@openjdk
Copy link

openjdk bot commented May 24, 2025

@AlanBateman
The hotspot label was successfully removed.

@openjdk
Copy link

openjdk bot commented May 24, 2025

@AlanBateman
The core-libs label was successfully removed.

@openjdk
Copy link

openjdk bot commented May 24, 2025

@AlanBateman
The jmx label was successfully removed.

@AlanBateman AlanBateman marked this pull request as ready for review May 24, 2025 07:36
@openjdk openjdk bot added the rfr Pull request is ready for review label May 24, 2025
@mlbridge
Copy link

mlbridge bot commented May 24, 2025

Webrevs

* text or JSON format.
* This class defines static methods to support the Thread.dump_to_file diagnostic command
* and the HotSpotDiagnosticMXBean.dumpThreads API. It defines methods to generate a
* thread dump to a file or byte array in plain text or JSON format.
*/
public class ThreadDumper {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class ThreadDumper {
public final class ThreadDumper {

sb.append(String.format("\\u%04x", c));
} else {
sb.append(c);
private static class JsonWriter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static class JsonWriter {
private static final class JsonWriter {
private static final class Node {

Comment on lines +80 to +83
StringBuffer sb = new StringBuffer();
sb.append(System.currentTimeMillis());
String s = sb.toString();
ref.set(s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
StringBuffer sb = new StringBuffer();
sb.append(System.currentTimeMillis());
String s = sb.toString();
ref.set(s);
ref.set(
new StringBuffer()
.append(System.currentTimeMillis())
.toString());

Comment on lines 384 to 385
sb.append(",");
sb.append(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sb.append(",");
sb.append(name);
sb.append(",")
.append(name);

@AlanBateman AlanBateman changed the base branch from master to pr/25425 May 25, 2025 06:16
Instant now = Instant.now();
Thread.State state = snapshot.threadState();
writer.println("#" + thread.threadId() + " \"" + snapshot.threadName()
+ "\" " + (thread.isVirtual() ? "virtual " : "") + state + " " + now);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ "\" " + (thread.isVirtual() ? "virtual " : "") + state + " " + now);
+ "\" " + (thread.isVirtual() ? "virtual " : "") + state + " " + now);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csr Pull request needs approved CSR before integration rfr Pull request is ready for review serviceability [email protected]
Development

Successfully merging this pull request may close these issues.

4 participants