RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Thu Mar 13 11:15:58 UTC 2014
The new webrev is uploaded here:
http://cr.openjdk.java.net/~jwilhelm/7607562/webrev.03/
/Jesper
Andrey Zakharov skrev 12/3/14 10:06:
> 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
>>>>>>>>> >
>>>>>>>>> >
>>>>>>>>
>>>>>
>
More information about the hotspot-gc-dev
mailing list