RFR(XS): 8193509: Test dynamic path to retrieve active processor count.

[Doerr, Martin]

Hi Götz,

thanks for providing this test which runs through the CPU_ALLOC code path. Looks good.

Best regards,
Martin


-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of David Holmes
Sent: Donnerstag, 14. Dezember 2017 23:55
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(XS): 8193509: Test dynamic path to retrieve active processor count.

My testing passed. So just needing second review.

Also for now I suggest ensuring the "FixVersion" is not set when doing 
the push, as the correct version for JDK 11 has not yet been set in the 
configuration info used by hgupdater. Otherwise if you correctly set 
FixVersion to 11, hgupdater will create a 10 backport; if you set it to 
10 and hgupdater gets fixed before you push, then you will get a 
backport for 11. So leave it blank or just wait until we know hgupdater 
is doing the right thing.

Thanks,
David

On 15/12/2017 7:12 AM, David Holmes wrote:
> Hi Goetz,
> 
> On 14/12/2017 11:01 PM, Lindenmaier, Goetz wrote:
>> Hi David,
>>
>> I implemented the test to detect iff the VM is run on an older OS
>> than it was compiled on which lacks the CPU_ALLOC support.
>> Basically that's what I wrote into the @summary :)
> 
> So do you plan on running the test on such older OS? And on such an OS 
> will you ever find >1024 processors and so try to really take the path 
> that test shows is not usable ?
> 
>> I ran the test with jdk 9 on ppc64, ppc64le, s390, x86_64,
>> SLES 11, 12; RHEL 6, 7 (not all combinations, though).
>> Tonight it will run with jdk/hs and a similar coverage.  (Actually, the
>> test has been in the test suite for the other platforms, too, but
>> is skipped there, so skipping is tested, too.)
>>
>> New webrev with renamed test:
>> http://cr.openjdk.java.net/~goetz/wr17/8193509-activeProcTest/webrev.02/
> 
> Okay. I'll apply it locally and run on our systems just for a sanity 
> check. Meanwhile a second reviewer is needed.
> 
> Thanks,
> David
> 
>>
>> Best regards,
>>   Goetz.
>>
>>
>>
>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Donnerstag, 14. Dezember 2017 13:40
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>>> dev at openjdk.java.net
>>> Subject: Re: RFR(XS): 8193509: Test dynamic path to retrieve active 
>>> processor
>>> count.
>>>
>>> Hi Goetz,
>>>
>>> On 14/12/2017 10:06 PM, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> active_processor_count() in os_linux.cpp has two paths, one is only
>>>> excercised if the VM runs on a machine with more than 1024 processors.
>>>> Flag UseCpuAllocPath allows to force this path. Add a test with
>>>> this flag to excercise this code.
>>>
>>> Not sure I see the value in having a test for this. Where would this
>>> test run such that it would show us there is some problem that needs
>>> fixing? The real test of that code path is on a machine with > 1024
>>> processors.
>>>
>>> That aside please reformat the
>>>
>>>     29  * @summary If #processors > 1024 os_linux.cpp uses special
>>> coding. Excercise this by forcing usage of this coding. If this fails,
>>> this VM was either compiled on a platform which does not define
>>> CPU_ALLOC, or it is executed on a platform that does not support it.
>>>
>>> to use multiple lines. Though a simple:
>>>
>>> @summary  Test the UseCpuAllocPath code path
>>>
>>> would suffice IMHO.
>>>
>>> And the name of the test should more accurately be TestUseCpuAllocPath.
>>>
>>>> Please review this change. I please need a sponsor. (Do I need one with
>>> the new repo setup?)
>>>> http://cr.openjdk.java.net/~goetz/wr17/8193509-
>>> activeProcTest/webrev.01/
>>>
>>> The need for a sponsor is not related to the repo setup, but the need
>>> for adequate cross-platform testing. If the set of platforms you've
>>> tested on includes all the platforms Oracle would test on, then a
>>> sponsor would not add anything to the equation. But your RFR needs to
>>> include what testing you have done.
>>>
>>> Thanks,
>>> David
>>>
>>>> Best regards,
>>>>     Goetz.
>>>>