[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-dev
Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an unnecessary test library dependency
From: Mandy Chung <mandy.chung () oracle ! com>
Date: 2016-04-21 17:48:24
Message-ID: BDDE39F2-BA2A-46AE-ABF4-97D001ABAD65 () oracle ! com
[Download RAW message or body]
The patch looks fine to me.
Due to ProcessTools.getVmInputArguments() dependency, any test using ProcessTools has \
@modules java.management even it does not use this method.
It'd be good to refactor ProcessTools.getVmInputArguments() and maybe other test \
library to eliminate unnecessary dependency. Shura has cleaned up \
jdk/test/lib/testlibrary in the jdk side: \
https://bugs.openjdk.java.net/browse/JDK-8139430
Can you file a separate issue to refactor the hotspot test library and similar fix to \
JDK-8139430 can be applied in the future?
thanks
Mandy
> On Apr 21, 2016, at 5:23 AM, Alexander Kulyakhtin <alexander.kulyakhtin@oracle.com> \
> wrote:
> Mandy,
>
> Thank you very much for your review.
>
> I have updated the fix per your comments, making ProcessTools.getProcessId() return \
> long.
> The function ProcessTools.getVmInputArguments(), although does depend on the API \
> from the java.management, is not used by any of the tests addressed by the CR.
> Best regards,
> Alexander
>
> ----- Original Message -----
> From: alexander.kulyakhtin@oracle.com
> To: hotspot-dev@openjdk.java.net
> Cc: mandy.chung@oracle.com
> Sent: Thursday, April 21, 2016 3:11:46 PM GMT +03:00 Iraq
> Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an unnecessary \
> test library dependency
> Hi,
>
> Could you, please, review this fix (updated to address the findings of the previous \
> review)
> CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on \
> compact2 due to an unnecessary test library dependency"
> Webrev: http://cr.openjdk.java.net/~akulyakh/8153992_02/index.html
>
> Before the fix the ProcessTools.getProcessId() used the \
> ManagementFactory.getRuntimeMXBean() API. The API is not available on compact2 and \
> below. Therefore the tests failed. We are changing the ProcessTools.getProcessId() \
> method to use the JDK 9 Process.getPid(). This eliminates the unnecessary \
> dependency making the tests pass on compact2.
> Since, with this change ProcessTools.getProcessId() now returns long we are also \
> trivially modifying all the affected tests.
> Best regards,
> Alexander
>
>
>
> ----- Original Message -----
> From: mandy.chung@oracle.com
> To: alexander.kulyakhtin@oracle.com
> Cc: serviceability-dev@openjdk.java.net
> Sent: Thursday, April 21, 2016 12:03:14 AM GMT +03:00 Iraq
> Subject: Re: RFR:8153989:Some SVC tests fail on compact2 due to an unnecessary test \
> library dependency
>
> > On Apr 20, 2016, at 9:06 AM, Alexander Kulyakhtin \
> > <alexander.kulyakhtin@oracle.com> wrote:
> > Hi,
> >
> > Could you, please, review this small tests-only fix:
> >
> > CR: https://bugs.openjdk.java.net/browse/JDK-8153992 "Some SVC tests fail on \
> > compact2 due to an unnecessary test library dependency"
> > Webrev: http://cr.openjdk.java.net/~akulyakh/8153992/test/testlibrary/jdk/test/lib/ProcessTools.java.udiff.html
> >
> > Before the fix the ProcessTools.getProcessId() used the \
> > ManagementFactory.getRuntimeMXBean() API. The API is not available on compact2 \
> > and below. Therefore the tests failed.
> > We are changing the ProcessTools.getProcessId() method to use the JDK 9 \
> > Process.getPid(). This eliminates the unnecessary dependency making the tests \
> > pass on compact2.
>
> This looks okay. But I see that getVmInputArguments calls \
> ManagementFactory.getRuntimeMXBean. So ProcessTools still has a dependency on \
> java.management.
> The jdk test library ProcessTools::getProcessId has been long ago to call \
> Process::getPid and the method is changed to return long. I thought that similar \
> change would have been made in the hotspot test library at that time.
> > I am not sure how acceptable it is to cast from long to int this change. If it is \
> > not acceptable we can change the return type to long. This however, will cause \
> > massive changes throughout the hotspot tests which presently expect \
> > getProcessId() to return int.
>
> IMO it would be good to return long or have the callsite to call \
> ProcessHandle.current().getPid().
> Mandy
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic