RFR:8153992 (re-review, missed one file) : Some hotspot tests fail on compact2 due to an unnecessary test library dependency

Alexander Kulyakhtin alexander.kulyakhtin at oracle.com
Tue Apr 26 10:17:56 UTC 2016


Dmitry,

Thank you very much for the review.

Best regards,
Alexander

----- Original Message -----
From: dmitry.samersoff at oracle.com
To: alexander.kulyakhtin at oracle.com, serviceability-dev at 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 at oracle.com
> To: alexander.kulyakhtin at oracle.com, alexandre.iline at oracle.com
> Cc: hotspot-dev at 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 at 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 at oracle.com
>> To: hotspot-dev at openjdk.java.net
>> Cc: mandy.chung at 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 at oracle.com
>> To: alexander.kulyakhtin at oracle.com
>> Cc: serviceability-dev at 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 at 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.


More information about the serviceability-dev mailing list