8272854: split runtime/CommandLine/PrintTouchedMethods.java test #5231

iignatev · 2021-08-23T21:30:07Z

Hi all,

could you please review this patch that extracts part of runtime/CommandLine/PrintTouchedMethods.java test to runtime/CommandLine/PrintTouchedMethodsJcmd.java?

from JBS:

part of runtime/CommandLine/PrintTouchedMethods.java test can be done in a driver mode and ignore external flags, while another part should be executed in an othervm mode. to make execution mode efficient, the test can be split into two.

testing: runtime/CommandLine/ on {linux,windows,macos}-x64

-- Igor

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8272854: split runtime/CommandLine/PrintTouchedMethods.java test

Reviewers

Ioi Lam (@iklam - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5231

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

Using diff file

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

bridgekeeper · 2021-08-23T21:31:12Z

👋 Welcome back iignatyev! 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 · 2021-08-23T21:34:29Z

@iignatev The following label will be automatically applied to this pull request:

hotspot-runtime

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.

mlbridge · 2021-08-23T21:39:41Z

Webrevs

00: Full (747c0f3f)

dholmes-ora · 2021-08-24T07:18:04Z

test/hotspot/jtreg/runtime/CommandLine/PrintTouchedMethodsJcmd.java

+    public static void main(String args[]) throws Exception {
+      var pid = Long.toString(ProcessHandle.current().pid());
+      var pb = new ProcessBuilder();
+      pb.command(new String[] {JDKToolFinder.getJDKTool("jcmd"), pid, "VM.print_touched_methods"});


Can't you just include the desired VM flags as part of the String[] here and keep everything in one file and using driver mode?

VM flags which we pass here are for jcmd (that connects to the main process), but it's the main process that needs to be run w/ -XX:+UnlockDiagnosticVMOptions -XX:+LogTouchedMethods.

we can keep all testing in one test file by introducing yet another process (which does nothing and just waits for the signal from the main process to exit) and pass its pid to jcmd. IMHO, this will just add unneeded complexity to the test.

bridgekeeper · 2021-09-21T09:14:26Z

@iignatev This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2021-10-19T10:59:13Z

@iignatev This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

iignatev · 2021-10-20T19:43:55Z

/open

openjdk · 2021-10-20T19:44:50Z

@iignatev This pull request is now open

iklam

LGTM. A nit about an existing problem in the test case.

iklam · 2021-10-20T21:09:00Z

test/hotspot/jtreg/runtime/CommandLine/PrintTouchedMethodsJcmd.java

+      try {
+        output.shouldContain("PrintTouchedMethodsJcmd.main:([Ljava/lang/String;)V");
+      } catch (RuntimeException e) {
+        output.shouldContain("Unknown diagnostic command");


I think this is an existing bug. Do we need this catch block? TouchedMethodsDCmd is always enabled as long as the current JVM has INCLUDE_SERVICES is true. If INCLUDE_SERVICES is false, the jcmd connection will fail, and you will get a different error, like

com.sun.tools.attach.AttachNotSupportedException: Unable to open socket file /proc/18810/root/tmp/.java_pid18810: target process 18810 doesn't respond within 10500ms or HotSpot VM not loaded

The catch block will incorrectly ignore the problem if the VM is incorrectly modified and the TouchedMethodsDCmd is inadvently excluded.

I agree that it doesn't seem we need the catch block here. yet I'd prefer to remove it by a separate RFE.

openjdk · 2021-10-20T21:10:29Z

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

8272854: split runtime/CommandLine/PrintTouchedMethods.java test

Reviewed-by: iklam

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

a120937: 8274988: G1: refine G1SegmentedArrayAllocOptions and G1CardSetAllocOptions
c7a80e6: 8275607: G1: G1CardSetAllocator::drop_all needs to call G1SegmentedArray::drop_all
af7c56b: 8275167: x86 intrinsic for unsignedMultiplyHigh
cea3f01: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
d1e3ca4: 8233558: [TESTBUG] WindowOwnedByEmbeddedFrameTest.java fails on macos
913f928: 8273507: Convert test/jdk/java/nio/channels/Channels/TransferTo.java to TestNG test
0021a2f: 8275449: Add linux-aarch64-zero build profile
46b5bfb: 8233648: [TESTBUG] DefaultMenuBarTest.java failing on macos
bbc6061: 8272614: Unused parameters in MethodHandleNatives linking methods
7e28bdd: 8275055: Improve HeapRegionRemSet::split_card()
... and 730 more: https://git.openjdk.java.net/jdk/compare/03b5e99d998e037f84e9e2395b49321979c0acd8...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.

iignatev · 2021-10-23T03:58:39Z

/integrate

openjdk · 2021-10-23T04:00:10Z

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

fec470f: 8272163: Add -version option to keytool and jarsigner
6523c55: 8198336: java/awt/FontMetrics/FontCrash.java fails in headless mode
88bbf3c: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
4dec8fc: 8275645: [JVMCI] avoid unaligned volatile reads on AArch64
4e647aa: 8275416: G1: remove unnecessary make_referent_alive in precleaning phase
dd622e5: 8275783: G1: fix incorrect region type documentation in HeapRegionType
b2128a9: 8263155: Allow additional contents for DMG
1efe946: 8275712: Hashtable literal_size functions are broken
fab3d6c: 8275761: Backout: JDK-8274794 Print all owned locks in hs_err file
c978ca8: 8275344: -Xcheck:jni produces some warnings in the LCMS.c
... and 759 more: https://git.openjdk.java.net/jdk/compare/03b5e99d998e037f84e9e2395b49321979c0acd8...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-23T04:00:25Z

@iignatev Pushed as commit c94dc2a.

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

8272854

747c0f3

openjdk bot changed the title ~~8272854~~ 8272854: split runtime/CommandLine/PrintTouchedMethods.java test Aug 23, 2021

openjdk bot added the rfr label Aug 23, 2021

openjdk bot added the hotspot-runtime label Aug 23, 2021

dholmes-ora reviewed Aug 24, 2021

View changes

bridgekeeper bot closed this Oct 19, 2021

openjdk bot reopened this Oct 20, 2021

iklam approved these changes Oct 20, 2021

View changes

openjdk bot added the ready label Oct 20, 2021

openjdk bot closed this Oct 23, 2021

openjdk bot added integrated and removed ready rfr labels Oct 23, 2021

fmatte1 mentioned this pull request Nov 24, 2021

8275687: runtime/CommandLine/PrintTouchedMethods test shouldn't catch RuntimeException #6536

Closed

3 tasks

8272854: split runtime/CommandLine/PrintTouchedMethods.java test #5231

8272854: split runtime/CommandLine/PrintTouchedMethods.java test #5231

iignatev commented Aug 23, 2021 •

edited by openjdk[bot] bot

bridgekeeper bot commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

mlbridge bot commented Aug 23, 2021

dholmes-ora Aug 24, 2021

iignatev Oct 20, 2021

bridgekeeper bot commented Sep 21, 2021

bridgekeeper bot commented Oct 19, 2021

iignatev commented Oct 20, 2021

openjdk bot commented Oct 20, 2021

iklam left a comment

iklam Oct 20, 2021

iignatev Oct 21, 2021

openjdk bot commented Oct 20, 2021 •

edited

iignatev commented Oct 23, 2021

openjdk bot commented Oct 23, 2021

openjdk bot commented Oct 23, 2021

8272854: split runtime/CommandLine/PrintTouchedMethods.java test #5231

8272854: split runtime/CommandLine/PrintTouchedMethods.java test #5231

Conversation

iignatev commented Aug 23, 2021 • edited by openjdk[bot] bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

mlbridge bot commented Aug 23, 2021

Webrevs

dholmes-ora Aug 24, 2021

Choose a reason for hiding this comment

iignatev Oct 20, 2021

Choose a reason for hiding this comment

bridgekeeper bot commented Sep 21, 2021

bridgekeeper bot commented Oct 19, 2021

iignatev commented Oct 20, 2021

openjdk bot commented Oct 20, 2021

iklam left a comment

iklam Oct 20, 2021

Choose a reason for hiding this comment

iignatev Oct 21, 2021

Choose a reason for hiding this comment

openjdk bot commented Oct 20, 2021 • edited

iignatev commented Oct 23, 2021

openjdk bot commented Oct 23, 2021

openjdk bot commented Oct 23, 2021

iignatev commented Aug 23, 2021 •

edited by openjdk[bot] bot

openjdk bot commented Oct 20, 2021 •

edited