'Re: RFR:8153992 (re-review, missed one file) : Some hotspot tests fail on compact2 due to an unneces'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR:8153992 (re-review, missed one file) : Some hotspot tests fail on compact2 due to an unneces
From:       Alexander Kulyakhtin <alexander.kulyakhtin () oracle ! com>
Date:       2016-04-26 10:17:56
Message-ID: 319dc8fc-58a6-440a-b3e5-caed9b4681df () default
[Download RAW message or body]

Dmitry,

Thank you very much for the review.

Best regards,
Alexander

----- Original Message -----
From: dmitry.samersoff@oracle.com
To: alexander.kulyakhtin@oracle.com, serviceability-dev@openjdk.java.net
Sent: Tuesday, April 26, 2016 1:12:56 PM GMT +03:00 Iraq
Subject: Re: RFR:8153992 (re-review, missed one file) : Some hotspot tests fail on \
compact2 due to an unnecessary test library dependency

Alexander,

Looks good for me!

-Dmitry


On 2016-04-26 12:36, Alexander Kulyakhtin wrote:
> Hi,
> 
> I've missed one file in the changeset reviewed before, therefore the push failed \
> and I have to ask for a quick re-review. 
> The missing change is here: \
> http://cr.openjdk.java.net/~akulyakh/8153992_02/test/testlibrary/jdk/test/lib/dcmd/FileJcmdExecutor.java.udiff.html
>  
> I've verified that all the hotspot tests have passed with that change.
> 
> The full changeset (reviewed before):
> 
> 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
> 
> Best regards,
> Alexander
> 
> ----- Forwarded Message -----
> From: mandy.chung@oracle.com
> To: alexander.kulyakhtin@oracle.com, alexandre.iline@oracle.com
> Cc: hotspot-dev@openjdk.java.net
> Sent: Thursday, April 21, 2016 8:48:23 PM GMT +03:00 Iraq
> Subject: Re: RFR:8153992:Some hotspot tests fail on compact2 due to an unnecessary \
> test library dependency 
> 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
> > 
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic