RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Mon Mar 10 00:56:49 UTC 2014
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