8287008: Improve tests for thread dumps in JSON format #8784

AlanBateman · 2022-05-19T09:38:06Z

This change is mostly test infrastructure and improvements to the testing of thread dumps in JSON format. It also tweaks the thread dump format so that the process identifier and thread identifiers are a type string rather than number.

The tests for thread dumps added by JEP 425 are changed to use the test infrastructure library. The tests for JEP 428 will also make use of this test infrastructure.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

JDK-8287008: Improve tests for thread dumps in JSON format

Reviewers

Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8784/head:pull/8784
$ git checkout pull/8784

Update a local copy of the PR:
$ git checkout pull/8784
$ git pull https://git.openjdk.java.net/jdk pull/8784/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8784

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8784.diff

bridgekeeper · 2022-05-19T09:39:25Z

👋 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 · 2022-05-19T09:41:17Z

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

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 · 2022-05-19T09:48:50Z

/label remove imx

openjdk · 2022-05-19T09:49:48Z

@AlanBateman
The label imx is not a valid label.
These labels are valid:

serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

AlanBateman · 2022-05-19T09:50:26Z

/label remove jmx

openjdk · 2022-05-19T09:51:19Z

@AlanBateman
The jmx label was successfully removed.

mlbridge · 2022-05-19T11:21:07Z

Webrevs

plummercj · 2022-05-19T15:28:01Z

It would be useful if ThreadDump.java contained a comment with a description of the JSON file syntax. It could either be in the form of a simple dump or as a simple syntactic description.

AlanBateman · 2022-05-19T15:33:46Z

It would be useful if ThreadDump.java contained a comment with a description of the JSON file syntax. It could either be in the form of a simple dump or as a simple syntactic description.

Still TBD on where the JSON "schema" will be specified, it might be HotSpotDiagnosMXBean.dumpThreads as it will needed by tools that parse the JSON text. So when we do that then I think we can reference it from the test class.

plummercj · 2022-05-19T16:25:05Z

Ok, but I was mostly hoping to have it to make reviewing this PR easier.

AlanBateman · 2022-05-19T16:49:52Z

If we use JSON schema then it will something like this, does that help?

{
  "type": "object",
  "properties": {
    "threadDump": {
      "type": "object",
      "properties": {
        "processId": {
          "type": "integer",
          "description": "The native process id of the Java virtual machine."
        },
        "time": {
          "type": "string",
          "description": "The time in ISO 8601 format when the thread dump was generated."
        },
        "runtimeVersion": {
          "type": "string",
          "description": "The runtime version, see java.lang.Runtime.version()."
        },
        "threadContainers": {
          "type": "array",
          "description": "The array of thread containers.",
          "items": [
            {
              "type": "object",
              "properties": {
                "container": {
                  "type": "string",
                  "description": "The container name. The container name is unique."
                },
                "parent": {
                  "type": ["string", "null"],
                  "description": "The parent container name or null for the root container."
                },
                "owner": {
                  "type": ["integer", "null"],
                  "description": "The thread identifier of the owner thread or null if not owned."
                },
                "threads": {
                  "type": "array",
                  "description": "The array of threads in the thread container.",
                  "items": [
                    {
                      "type": "object",
                      "properties": {
                        "tid": {
                          "type": "integer",
                          "description": "The thread identifier, see java.lang.Thread.threadId()."
                        },
                        "name": {
                          "type": "string",
                          "description": "The thread name."
                        },
                        "stack": {
                          "type": "array",
                          "description": "The thread stack. The first element is the top of the stack.",
                          "items": [
                            {
                              "type": "string",
                              "description": "A stack trace element, see java.lang.StackTraceElement."
                            }
                          ]
                        }
                      },
                      "required": [
                        "tid",
                        "name",
                        "stack"
                      ]
                    }
                  ]
                },
                "threadCount": {
                  "type": "integer",
                  "description": "The number of threads in the thread container."
                }
              },
              "required": [
                "container",
                "parent",
                "owner",
                "threads",
                "threadCount"
              ]
            }
          ]
        }
      },
      "required": [
        "processId",
        "time",
        "runtimeVersion",
        "threadContainers"
      ]
    }
  },
  "required": [
    "threadDump"
  ]
}

plummercj · 2022-05-20T04:22:21Z

I think a short example dump would actually be easier to read alongside the code.

AlanBateman · 2022-05-20T11:31:30Z

I think a short example dump would actually be easier to read alongside the code.

That's a good idea. I've updated the class description to include an example and also a code example of how it can be used in tests. The thread dump example collapses several objects to avoid it getting too unwieldily.

I've started PR8807 to document the format.

test/lib/jdk/test/lib/threaddump/ThreadDump.java

test/jdk/com/sun/management/HotSpotDiagnosticMXBean/DumpThreads.java

AlanBateman · 2022-05-23T08:32:58Z

I've updated the patch to generate the process and thread identifiers as strings rather than numbers (as JSON numbers that are integers don't have the required range). The test infra and DumpThreads.java test are expanded a bit and now test the processId, time and runtimeVersion fields.

test/lib/jdk/test/lib/threaddump/ThreadDump.java

openjdk · 2022-05-23T22:20:04Z

@AlanBateman 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:

8287008: Improve tests for thread dumps in JSON format

Reviewed-by: cjplummer

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 53 new commits pushed to the master branch:

02fec1e: 8287155: Additional make typos
5b7d066: 8287109: Distrust.java failed with CertificateExpiredException
6a19220: 8286832: JavaDoc pages call browser history API too often
9df93a1: 8286887: Remove logging from search.js
646c8aa: 8286277: CDS VerifyError when calling clone() on object array
ef7a9f8: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate
ac274c4: 8286956: Loom: Define test groups for development/porting use
5d8d6da: 8286972: Support the new loop induction variable related PopulateIndex IR node on x86
8122466: 8287113: JFR: Periodic task thread uses period for method sampling events
940e94f: 8285739: disable EscapeBarrier deopt for virtual threads
... and 43 more: https://git.openjdk.java.net/jdk/compare/47500b2429c8e09f322ec3ff95ff09293db26a9a...master

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 master branch, type /integrate in a new comment.

AlanBateman · 2022-05-24T06:19:39Z

/integrate

openjdk · 2022-05-24T06:21:07Z

Going to push as commit 15f1583.
Since your change was applied there have been 57 commits pushed to the master branch:

a5caffd: 8286786: [macos] javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java still fails
c1db70d: 8285973: x86_64: Improve fp comparison and cmove for eq/ne
782ae38: 8286262: Windows: Cleanup deprecation warning suppression
9f73fb5: 8225013: sanity/client/SwingSet/src/ScrollPaneDemoTest.java fails on Linux
02fec1e: 8287155: Additional make typos
5b7d066: 8287109: Distrust.java failed with CertificateExpiredException
6a19220: 8286832: JavaDoc pages call browser history API too often
9df93a1: 8286887: Remove logging from search.js
646c8aa: 8286277: CDS VerifyError when calling clone() on object array
ef7a9f8: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate
... and 47 more: https://git.openjdk.java.net/jdk/compare/47500b2429c8e09f322ec3ff95ff09293db26a9a...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-24T06:21:21Z

@AlanBateman Pushed as commit 15f1583.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Initial commit

c0a1274

openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org labels May 19, 2022

openjdk bot removed the jmx jmx-dev@openjdk.org label May 19, 2022

AlanBateman marked this pull request as ready for review May 19, 2022 11:15

openjdk bot added the rfr Pull request is ready for review label May 19, 2022

Add example thread dump and usage to class description

15c2fd7

dfuch reviewed May 20, 2022

View reviewed changes

test/lib/jdk/test/lib/threaddump/ThreadDump.java Outdated Show resolved Hide resolved

Remove spurious colon

4f9590f

plummercj reviewed May 23, 2022

View reviewed changes

Use string rater than number for process/thread identifiers

353fc53

plummercj reviewed May 23, 2022

View reviewed changes

test/lib/jdk/test/lib/threaddump/ThreadDump.java Outdated Show resolved Hide resolved

Review comments

7e0c151

plummercj approved these changes May 23, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 23, 2022

openjdk bot added the integrated Pull request has been integrated label May 24, 2022

openjdk bot closed this May 24, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8287008: Improve tests for thread dumps in JSON format #8784

8287008: Improve tests for thread dumps in JSON format #8784

AlanBateman commented May 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 19, 2022

openjdk bot commented May 19, 2022

AlanBateman commented May 19, 2022

openjdk bot commented May 19, 2022

AlanBateman commented May 19, 2022

openjdk bot commented May 19, 2022

mlbridge bot commented May 19, 2022 •

edited

Loading

plummercj commented May 19, 2022

AlanBateman commented May 19, 2022 •

edited

Loading

plummercj commented May 19, 2022

AlanBateman commented May 19, 2022 •

edited

Loading

plummercj commented May 20, 2022

AlanBateman commented May 20, 2022

AlanBateman commented May 23, 2022 •

edited

Loading

openjdk bot commented May 23, 2022

AlanBateman commented May 24, 2022

openjdk bot commented May 24, 2022

openjdk bot commented May 24, 2022

8287008: Improve tests for thread dumps in JSON format #8784

8287008: Improve tests for thread dumps in JSON format #8784

Conversation

AlanBateman commented May 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 19, 2022

openjdk bot commented May 19, 2022

AlanBateman commented May 19, 2022

openjdk bot commented May 19, 2022

AlanBateman commented May 19, 2022

openjdk bot commented May 19, 2022

mlbridge bot commented May 19, 2022 • edited Loading

Webrevs

plummercj commented May 19, 2022

AlanBateman commented May 19, 2022 • edited Loading

plummercj commented May 19, 2022

AlanBateman commented May 19, 2022 • edited Loading

plummercj commented May 20, 2022

AlanBateman commented May 20, 2022

AlanBateman commented May 23, 2022 • edited Loading

openjdk bot commented May 23, 2022

AlanBateman commented May 24, 2022

openjdk bot commented May 24, 2022

openjdk bot commented May 24, 2022

AlanBateman commented May 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 19, 2022 •

edited

Loading

AlanBateman commented May 19, 2022 •

edited

Loading

AlanBateman commented May 19, 2022 •

edited

Loading

AlanBateman commented May 23, 2022 •

edited

Loading