Hi David,
Currently sun.tools.ProcessHelper is implemented for Linux platform only since
it uses the proc filesystem that is limited to Linux platform. As a result
the test is limited to Linux platform as well. All recorded timeouts that this
change is fixing were reported for Linux platform only. In future we could
consider providing the similar functionality for other platforms but it will
need to use some different mechanisms.
Please find below the statistics for running this newly added test
test/jdk/sun/tools/jcmd/TestProcessHelper.java with linux-x64-debug build in
Mach5:
1. With Graal ("-XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI
-XX:+TieredCompilation -XX:+UseJVMCICompiler -Djvmci.Compiler=graal") - 6s
2. With Xcomp ("-Xcomp") - 20 s
3. With Graal and Xcomp ("-Xcomp -XX:+UnlockExperimentalVMOptions
-XX:+EnableJVMCI -XX:+TieredCompilation -XX:+UseJVMCICompiler
-Djvmci.Compiler=graal") - 1m 21s
Best regards,
Daniil
On 2/7/19, 3:08 PM, "David Holmes" <[email protected]> wrote:
Hi Daniil,
Thanks for the additional testing.
On 8/02/2019 3:52 am, Daniil Titov wrote:
> Hi Serguei and David,
>
> Please review a new version of the fix that adds a new test
test/jdk/sun/tools/jcmd/TestProcessHelper.java that starts Java processes using
different command line options and verifies that
sun.tools.ProcessHelper.getMainClass(pid) method returns a correct main class.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.05/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
Why linux only?
Also wondering how long this will take to run? We need to be mindful of
impact on tier testing. AFAICS this will run in tier3, tier4-graal and
tier8-Xcomp - and it's the last one we may need to watch for time.
Thanks,
David
> Thanks!
> -Daniil
>
>
>
> On 2/4/19, 2:26 PM, "[email protected]"
<[email protected]> wrote:
>
> Hi Daniil,
>
> Thank you for the update!
>
> It looks good in general.
>
> I think, it can be a good idea to add a simple tests for this
> command-line processing.
> It should save us from any surprises.
>
> Thanks,
> Serguei
>
>
> On 2/4/19 13:24, Daniil Titov wrote:
> > Hi Serguei,
> >
> > Thank you for reviewing this fix.
> >
> > Please review a new version of the fix that includes all changes
you suggested but one about lines 88-91 and 97-101.
> >
> > 88 if (parts[i].equals("-p") ||
parts[i].equals("--module-path")) {
> > 89 i++;
> > 90 continue;
> > 91 }
> > ...
> > 97 // If this is a classpath option then skip the
next part as well ( the classpath itself)
> > 98 if (parts[i].equals("-cp") ||
parts[i].equals("-classpath")) {
> > 99 i++;
> > 100 continue;
> > 101 }
> > 102 // Skip all other Java options
> > 103 if (parts[i].startsWith("-")) {
> > 104 continue;
> > 105 }
> >
> > You are right, these statements are needed to filter out the parts
which have nothing to do with the mainClass. But we cannot remove these lines
and just return the latest part[i] that was not filtered out as the mainClass
since it will not work in the case when the command line includes arguments
specified after the main class. In the approach we use the main class is the
*first* part[i] (with i > 0) that is not a Java option ( part[i] that doesn't
start with '-' and is not classpath, module path, jar file path or module
name). This condition that the mainClass is not null is checked on line 89
inside for loop.
> >
> > 89 for (int i = 1; i < parts.length && mainClass == null;
i++) {
> >
> > To simplify the code in the new version of the patch the lines
88-91 and 97-101 are combined in the one "if" block.
> >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.04
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
> >
> > Thanks.
> > --Daniil
> >
> > From: "[email protected]" <[email protected]>
> > Date: Thursday, January 31, 2019 at 1:23 PM
> > To: David Holmes <[email protected]>, Daniil Titov
<[email protected]>, serviceability-dev
<[email protected]>
> > Subject: Re: RFR 8205654:
serviceability/dcmd/framework/HelpTest.java timed out
> >
> > Hi Daniil,
> >
> >
> > I have some secondary comment on new file:
> >
> >
http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java.html
> >
> > 70 for (int i = 0; i < parts.length && mainClass ==
null; i++) {
> > 71 // Check the executable
> > 72 if (i == 0) {
> > 73 String[] executablePath =
parts[i].split("/");
> > 74 if (executablePath.length > 0) {
> > 75 String binaryName =
executablePath[executablePath.length - 1];
> > 76 if (!"java".equals(binaryName)) {
> > 77 // Skip the process if it is not
started with java launcher
> > 78 return null;
> > 79 }
> > 80 }
> > 81 continue;
> > 82 }
> >
> > It is better execute the logic in lines 73-80 before the loop.
> > It will simplify the code a little bit.
> > String[] executablePath = parts[i].split("/");
> > if (executablePath.length > 0) {
> > String binaryName = executablePath[executablePath.length -
1];
> > if (!binaryName.equals("java") {
> > return null; // Skip the process if it is not started
with java launcher
> >
> > }
> > }
> > for (int i = 1; i < parts.length && mainClass == null; i++) {
> >
> > In the fragment below:
> >
> > 83 // Check if the module is executed with
explicitly specified main class
> > 84 if ((parts[i].equals("-m") ||
parts[i].equals("--module")) && i < parts.length - 1) {
> > 85 mainClass =
getMainClassFromModuleArg(parts[i + 1]);
> > 86 break;
> > 87 }
> >
> > would it better to just return the main class instead of having
a break statement? :
> > 85 return getMainClassFromModuleArg(parts[i +
1]);
> >
> >
> > The lines:
> > 108 if (jarFile != null) {
> > 109 return getMainClassFromJar(jarFile, pid);
> > 110 }
> >
> > is better to execute inside the loop the same as it is done for
getMainClassFromModuleArg().
> >
> > // Check if the main class needs to be read from the
manifest.mf in a JAR file
> > if (parts[i].equals("-jar") && i < parts.length - 1) {
> > return getMainClassFromJar(jarFile, pid);
> > }
> >
> > In the if statements:
> > 84 if ((parts[i].equals("-m") ||
parts[i].equals("--module")) && i < parts.length - 1) {
> > ...
> > 93 if (parts[i].equals("-jar") && i < parts.length
- 1) {
> >
> > the last condition (i < parts.length - 1) is better to make the
first (pre-condition).
> > They even can be combined together like below:
> > if (i < parts.length - 1) {
> > if ((parts[i].equals("-m") || parts[i].equals("--module")))
{
> > return getMainClassFromModuleArg(parts[i + 1]);
> > }
> > // Check if the main class needs to be read from the
manifest.mf in a JAR file
> > if (parts[i].equals("-jar") ) {
> > return getMainClassFromJar(jarFile, pid);
> > }
> > }
> >
> > The biggest concern are the fragments:
> > 88 if (parts[i].equals("-p") ||
parts[i].equals("--module-path")) {
> > 89 i++;
> > 90 continue;
> > 91 }
> > ...
> > 97 // If this is a classpath option then skip the
next part as well ( the classpath itself)
> > 98 if (parts[i].equals("-cp") ||
parts[i].equals("-classpath")) {
> > 99 i++;
> > 100 continue;
> > 101 }
> > 102 // Skip all other Java options
> > 103 if (parts[i].startsWith("-")) {
> > 104 continue;
> > 105 }
> > If I understand it correctly, these statements are needed to
filter out
> > the parts which have nothing to do with the mainClass.
> > The latest part[i] that was not filtered out is returned as the
mainClass.
> >
> > I'm thinking about more general approach here. Probably, we just
need to remove
> > the fragments 88-91 and 97-101 as they are covered by the
fragment 102-105.
> > It will also simplify the code.
> >
> > With all the suggestion above it should converge to something
like this:
> > String[] parts = cmdLine.split(" ");
> > String mainClass = null;
> >
> > String[] executablePath = parts[i].split("/");
> > if (executablePath.length > 0) {
> > String binaryName = executablePath[executablePath.length -
1];
> > if (!binaryName.equals("java") {
> > return null; // Skip the process if it is not started
with java launcher
> >
> > }
> > }
> > for (int 1 = 0; i < parts.length && mainClass == null; i++) {
> > if (i < parts.length - 1) {
> > if ((parts[i].equals("-m") ||
parts[i].equals("--module"))) {
> > return getMainClassFromModuleArg(parts[i + 1]);
> > }
> > // Check if the main class needs to be read from the
manifest.mf in a JAR file
> > if (parts[i].equals("-jar") ) {
> > return getMainClassFromJar(parts[i + 1], pid);
> > }
> > }
> > // Skip all other Java options
> > if (parts[i].startsWith("-")) {
> > continue;
> > }
> > mainClass = parts[i];
> > }
> > return mainClass;
> >
> >
> >
http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/test/hotspot/jtreg/serviceability/dcmd/framework/TestJavaProcess.java.html
> >
> > 49 if (cmd.equals("quit")) {
> > 50 log("'quit' received");
> > 51
> > 52 } else {
> >
> > The empty line 51 can be removed.
> >
> > Looking at this command-line processing I kind of understand the
David's concern.
> >
> > Thanks,
> > Serguei
> >
> >
> > On 1/20/19 21:18, David Holmes wrote:
> > Thanks for the update Daniil. I still remain concerned about the
robustness of the command-line parsing - this seems like a feature that needs
its own set of tests.
> >
> > I'll leave it up to Serguei and others as to how to proceed.
> >
> > David
> > -----
> >
> > On 19/01/2019 9:08 am, Daniil Titov wrote:
> >
> > Hi David and Serguei,
> >
> > Please review a new version of the fix that now covers the case
when Java executes a module with the main class name explicitly specified in
the command line.
> >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.03
> > Bug: : https://bugs.openjdk.java.net/browse/JDK-8205654
> >
> > Thanks!
> > --Daniil
> >
> > On 1/8/19, 6:05 PM, "David Holmes" mailto:[email protected]
wrote:
> >
> > Hi Daniil,
> > Sorry this slipped through the Xmas break cracks :)
> > On 22/12/2018 12:04 pm, Daniil Titov wrote:
> > > Hi David and Serguei,
> > >
> > > Please review a new version of the fix that for Linux
platform uses the proc filesystem to retrieve the main class name for the
running Java process.
> > >
> > > Webrev:
http://cr.openjdk.java.net/~dtitov/8205654/webrev.02/
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
> > It's more complex than I had envisaged but seems to be
doing the job.
> > I'm not sure how robust the command-line parsing is, in
particular it
> > doesn't handle these forms:
> > or java [options] -m <module>[/<mainclass>]
[args...]
> > java [options] --module <module>[/<mainclass>]
[args...]
> > (to execute the main class in a module)
> > I can't really comment on all the details.
> > Thanks,
> > David
> > -----
> > > Thanks,
> > > Daniil
> > >
> > > On 11/29/18, 4:52 PM, "David Holmes"
mailto:[email protected] wrote:
> > >
> > > Hi Daniil,
> > >
> > > On 30/11/2018 7:30 am, Daniil Titov wrote:
> > > > Thank you, David!
> > > >
> > > > The proposed fix didn't help. It still hangs at
some occasions. Additional tracing showed that when jcmd is invoked with the
main class name it iterates over all running Java processes and temporary
attaches to them to retrieve the main class name. It hangs while trying to
attach to one of the running Java processes. There are numerous Java processes
running at the host machine some associated with the test framework itself and
another with the tests running in parallel. It is not clear what exact is this
particular process since the jcmd hangs before retrieving the process' main
class name, but after all tests terminated the process with this id is no
longer running. I have to revoke this review since more investigation is
required.
> > >
> > > That sounds like an unsolvable problem for the test.
You can't control
> > > other Java processes on the machine, and searching by
name requires
> > > asking each of them in turn.
> > >
> > > How do we get the list of Java processes in the first
place? Perhaps we
> > > need to do some /proc/<pid>/cmdline peeking?
> > >
> > > Cheers,
> > > David
> > >
> > > >
> > > > Best regards,
> > > > Daniil
> > > >
> > > >
> > > >
> > > > On 11/11/18, 1:35 PM, "David Holmes"
mailto:[email protected] wrote:
> > > >
> > > > Hi Daniil,
> > > >
> > > > I took a quick look at this one ... two minor
comments
> > > >
> > > > The static class names could just be "Process"
as they will acquire the
> > > > enclosing class name as part of their own name
anyway. As it is this
> > > > gets repeated eg:
> > > >
> > > > HelpTest$HelpTestProcess
> > > > InvalidCommandTest$InvalidCommandTestProcess
> > > >
> > > > TestJavaProcess.java:
> > > >
> > > > 39 public static void main(String argv[]) {
> > > >
> > > > Nit: Should be "String[] argv" in Java style
> > > >
> > > > Thanks,
> > > > David
> > > >
> > > > On 10/11/2018 3:18 PM, Daniil Titov wrote:
> > > > > Please review the change that fixes
serviceability/dcmd/framework/* tests from a time out. The fix for JDK-8166642
made serviceability/dcmd/framework/* tests non-concurrent to ensure that they
don't interact with each other and there are no multiple tests running
simultaneously since all they do share the common main class name
com.sun.javatest.regtest.agent.MainWrapper. However, it looks like the tests
from other directories still might run in parallel with these tests and they
also have com.sun.javatest.regtest.agent.MainWrapper as a main class.
> > > > >
> > > > > The fix ensures that each
serviceability/dcmd/framework/* test uses a Java process with a unique main
class name when connecting to this process with jcmd and the main class name.
> > > > >
> > > > > Bug:
https://bugs.openjdk.java.net/browse/JDK-8205654
> > > > > Webrev:
http://cr.openjdk.java.net/~dtitov/8205654/webrev.001/
> > > > >
> > > > > Best regards,
> > > > > Daniil
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
>
>
>
>
