RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable

Andrey Zakharov andrey.x.zakharov at oracle.com
Wed Mar 12 09:06:26 UTC 2014 

Hello,
Moving forward.
tests now are in "gc/arguments" folder.
Thanks.

On 10.03.2014 04:56, Jesper Wilhelmsson wrote:
> Strange. It seems the entire 7607562 directory was missing from the 
> new CR server.
>
> It's back in place now.
> /Jesper
>
> Igor Ignatyev skrev 9/3/14 15:00:
>> Jesper,
>> On 03/07/2014 09:41 PM, Jesper Wilhelmsson wrote:
>>> The new webrev is uploaded here:
>>> http://cr.openjdk.java.net/~jwilhelm/7607562/webrev.02/
>>
>> I got 404:
>> $ curl -I http://cr.openjdk.java.net/~jwilhelm/7607562/webrev.02/
>> HTTP/1.1 404 Not Found
>> Content-Type: text/html
>> Content-Length: 345
>> Date: Sun, 09 Mar 2014 13:35:24 GMT
>> Server: lighttpd/1.4.23-devel-Unversioned directory
>>
>> Andrey,
>>> Andrey Zakharov skrev 7/3/14 17:44:
>>>> Updated. But checked code style guides. Nothing special about spaces
>>>> within braces ...
>> Even if we don't have on wiki-pages, we have it in code. the main 
>> rule of
>> code-style -- 'follow the existing style'.
>>
>>>> So, i leave it in "gc" folder for now. Maybe I'm wrong.
>>>> The most closely folder for this tests is "gc/arguments" i think.
>> I wrote 'memory_pressure' just 'cuz you used it before. I dislike 
>> having tests
>> directly in test/{gc,compiler,...}. 'gc/arguments' sounds good to me, 
>> so if you
>> (and other guys on this alias) also think that it's proper, please 
>> use it.
>>
>> PS don't forget to update 'test/TEST.groups' after it.
>>
>> Thanks,
>> Igor
>>
>>> /Jesper
>>>
>>> Andrey Zakharov skrev 7/3/14 17:44:
>>>> Hi, here changed webrev.02
>>>>
>>>> On 07.03.2014 16:56, Igor Ignatyev wrote:
>>>>> Andrey,
>>>>>
>>>>> a few new comments:
>>>>>
>>>>> 1. 'TestDynamicIntVMOption' isn't specific for gc testing, so please
>>>>> move it
>>>>> to testlibrary
>>>> Ok
>>>>>
>>>>> 2. I'd like to have 'check*' methods in a separate auxiliary class in
>>>>> testlibrary. 'VMOptionValueChecker' as you have it before was good
>>>>> place.
>>>> Ok
>>>>>
>>>>> 3. I really really don't like spaces in brackets:
>>>>>>  public TestDynamicIntVMOption( String name ) {
>>>>> ...
>>>>>> checkInvalidValue( "-10" );
>>>>> I don't think that it's fitted w/ our code-style.
>>>> Updated. But checked code style guides. Nothing special about spaces
>>>> within braces
>>>> here
>>>> http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html 
>>>>
>>>>
>>>>
>>>> here https://wiki.openjdk.java.net/display/OpenJFX/Code+Style+Rules
>>>> or here https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>>>> . Last one just notes - "Use more spaces and blank lines between 
>>>> larger
>>>> constructs, such as classes or function definitions."
>>>>
>>>>
>>>>
>>>>>
>>>>> 4. please move test to some subdirectory in test/gc, e.g.
>>>>> test/gc/memory_pressure/ as it was before. or
>>>>> test/gc/memory_pressure/flags/
>>>> I'm doubt about frustrated "memory_pressure" folder. I'd read again
>>>> https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests 
>>>>
>>>> and realize that we have all memory pressure tests divided onto
>>>> common, G1 and
>>>> ParallelGC specific tests.
>>>> So two last ones bunches of tests will lay in gc/g1 and gc/parallelgc
>>>> folders,
>>>> and only question is about common, which will lay in
>>>> "gc/memory_pressure" folder?
>>>> It strange for me that tests about dynamic changing of
>>>> "MaxHeapFreeRatio" flag
>>>> goes to gc/memory_pressure folder.
>>>> So, i leave it in "gc" folder for now. Maybe I'm wrong.
>>>> The most closely folder for this tests is "gc/arguments" i think.
>>>>
>>>>>
>>>>> 5. and the old one:
>>>>>>>> 3. You use management packages, but not all JDK/JRE bundles have
>>>>>>>> it, so
>>>>>>>> you have to add your tests in some group (needs_compact3, I
>>>>>>>> think). LevP
>>>>>>>> should have more information on it.
>>>>> LevP has confirmed that it's 'needs_compact3' group.
>>>>> hint: test/TEST.groups -- the file w/ groups
>>>> Yeah, sure, just didn't do that because of folder :)
>>>> Now in place.
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>> Thanks
>>>>> Igor
>>>>>
>>>>> On 03/07/2014 04:35 PM, Jesper Wilhelmsson wrote:
>>>>>> The webrev is uploaded here:
>>>>>> http://cr.openjdk.java.net/~jwilhelm/7607562/
>>>>>> /Jesper
>>>>>>
>>>>>> Andrey Zakharov skrev 7/3/14 11:23:
>>>>>>> Hi
>>>>>>> Thanks for comments.
>>>>>>> I have completely rework this tests according to comments.
>>>>>>> Also we have found that it will be easier to follow standard
>>>>>>> work-flow of
>>>>>>> submitting patches to branch 9 and then back-porting it to 8 and 7.
>>>>>>> So, here is webrev against
>>>>>>> http://hg.openjdk.java.net/jdk9/hs-gc/hotspot
>>>>>>> I hope, Jesper will help me to upload it right place.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>>
>>>>>>> On 05.03.2014 23:35, Lev wrote:
>>>>>>>> Igor, you are right. You could check it in proposal:
>>>>>>>> http://openjdk.java.net/jeps/161
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Lev
>>>>>>>>
>>>>>>>>
>>>>>>>> -------- Original message --------
>>>>>>>> From: Igor Ignatyev
>>>>>>>> Date:05/03/2014 13:12 (GMT+04:00)
>>>>>>>> To: Andrey Zakharov ,Lev Priima
>>>>>>>> Cc: hotspot-gc-dev at openjdk.java.net
>>>>>>>> Subject: Re: RFR: tests for JDK-8028720 Make the 
>>>>>>>> Min/MaxHeapFreeRatio
>>>>>>>> flags
>>>>>>>> manageable
>>>>>>>>
>>>>>>>> Hi Andrey,
>>>>>>>>
>>>>>>>> 0. In all your tests you have:
>>>>>>>> >   27  * @key gc
>>>>>>>> it's redundant, since the tests are already in gc directory. 
>>>>>>>> please
>>>>>>>> remove it.
>>>>>>>> >   28  * @bug JDK-8028391
>>>>>>>> according to convention, you have to use '8028391' instead of
>>>>>>>> 'JDK-8028391'.
>>>>>>>> $ grep -R "@bug" ./hotspot/test/ | wc -l
>>>>>>>> 434
>>>>>>>> $ grep -R "@bug" ./hotspot/test/ | grep -c JDK
>>>>>>>> 0
>>>>>>>>
>>>>>>>> 1. VMOptionValueChecker has checkInvalidValue and checkValid, 
>>>>>>>> please
>>>>>>>> normalize them:
>>>>>>>>    checkInvalidValue/checkValidValuefO
>>>>>>>>    or
>>>>>>>>    checkInvalid/checkValid
>>>>>>>>
>>>>>>>> 2. I don't like that 'VMOptionValueChecker.getIntWritableValue'
>>>>>>>> method
>>>>>>>> do two things, could you please replace it w/ two methods:
>>>>>>>> VMOptionValueChecker.getIntValue(name)
>>>>>>>> VMOptionValueChecker.checkIsWriteable(name)
>>>>>>>>
>>>>>>>> 3. You use management packages, but not all JDK/JRE bundles have
>>>>>>>> it, so
>>>>>>>> you have to add your tests in some group (needs_compact3, I
>>>>>>>> think). LevP
>>>>>>>> should have more information on it.
>>>>>>>>
>>>>>>>> Lev, could you please help us here? com.sun.management.* and
>>>>>>>> java.lang.management.* are available only in compact3, aren't 
>>>>>>>> they?
>>>>>>>>
>>>>>>>> 4. your tests look similar, can you create a super class for both
>>>>>>>> tests?
>>>>>>>> we will be able to reuse it in other tests for writable flags
>>>>>>>>
>>>>>>>> > class TestDynIntVMFlags  {
>>>>>>>> > String name;
>>>>>>>> > int originalValue;
>>>>>>>> > TestDynIntVMFlags(name) {
>>>>>>>> >   name = name;
>>>>>>>> >   int originalValue = getIntValue(name)
>>>>>>>> >   println(name + " = " + originalValue);
>>>>>>>> > }
>>>>>>>> >
>>>>>>>> > void test() {
>>>>>>>> >       checkIsWriteable(name);
>>>>>>>> >       checkInvalidValue(name, "" + Integer.MIN_VALUE);
>>>>>>>> >       checkInvalidValue(name, "" + Integer.MAX_VALUE);
>>>>>>>> >       checkInvalidValue(name, "-10");
>>>>>>>> >       checkInvalidValue(name, "190");
>>>>>>>> >       checkValid(name, "0");
>>>>>>>> >       extraTest();
>>>>>>>> > }
>>>>>>>> > void extraTest() { }
>>>>>>>> > }
>>>>>>>> > //// TestDynMinHeapFreeRatio.java
>>>>>>>> > TestDynMinHeapFreeRatio extends TestDynIntVMFlags {
>>>>>>>> > private TestDynMinHeapFreeRatio() { super("MinHeapFreeRatio"); }
>>>>>>>> >   String MaxFreeRatioFlagName = "MaxHeapFreeRatio";
>>>>>>>> >   void main() {
>>>>>>>> >     new TestDynMinHeapFreeRatio().test();
>>>>>>>> >   }
>>>>>>>> >   void extraTest() {
>>>>>>>> >     int maxHeapFreeValue = getIntValue(MaxFreeRatioFlagName);
>>>>>>>> >     checkInvalidValue(name, "" + (maxHeapFreeValue + 1));
>>>>>>>> >     checkValid(name, "" + (maxHeapFreeValue));
>>>>>>>> >   }
>>>>>>>> > }//// TestDynMaxHeapFreeRatio.java
>>>>>>>> > TestDynMaxHeapFreeRatio extends TestDynIntVMFlags {
>>>>>>>> > TestDynMaxHeapFreeRatio() { super("MaxHeapFreeRatio"); }
>>>>>>>> >   String MaxFreeRatioFlagName = "MinHeapFreeRatio";
>>>>>>>> >   void main() {
>>>>>>>> >     new TestDynMaxHeapFreeRatio().test();
>>>>>>>> >   }
>>>>>>>> >   void extraTest() {
>>>>>>>> >     int minHeapFreeValue = getIntValue(MinFreeRatioFlagName);
>>>>>>>> >     checkInvalidValue(name, "" + (minHeapFreeValue - 1));
>>>>>>>> >     checkValid(name, "" + (minHeapFreeValue));
>>>>>>>> >   }
>>>>>>>> > }
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Igor
>>>>>>>>
>>>>>>>> On 02/20/2014 10:09 PM, Thomas Schatzl wrote:
>>>>>>>> > Hi,
>>>>>>>> >
>>>>>>>> > On Thu, 2014-02-20 at 15:15 +0400, Andrey Zakharov wrote:
>>>>>>>> >> Any comments are welcome!
>>>>>>>> >> Thanks.
>>>>>>>> >>
>>>>>>>> >> On 14.02.2014 21:13, Jesper Wilhelmsson wrote:
>>>>>>>> >>> Looks good.
>>>>>>>> >>> /Jesper
>>>>>>>> >>>
>>>>>>>> >>> Andrey Zakharov skrev 14/2/14 16:33:
>>>>>>>> >>>> Simple tests to check that MaxHeapFreeRatio and 
>>>>>>>> MinHeapFreeRatio
>>>>>>>> >>>> flags are
>>>>>>>> >>>> writable and managable in proper ways.
>>>>>>>> >>>> webrev:
>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.test.02/
>>>>>>>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8028720
>>>>>>>> >>>>
>>>>>>>> >>>> Thanks.
>>>>>>>> >>
>>>>>>>> >
>>>>>>>> >    some typos: "managable" -> "manageable"
>>>>>>>> >
>>>>>>>> > Otherwise looks good.
>>>>>>>> >
>>>>>>>> > Thomas
>>>>>>>> >
>>>>>>>> >
>>>>>>>
>>>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 7607562.tar.gz
Type: application/gzip
Size: 37566 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140312/4efc0c7a/7607562.tar.gz>

More information about the hotspot-gc-dev mailing list