RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable
Igor Ignatyev
igor.ignatyev at oracle.com
Sun Mar 9 14:00:44 UTC 2014
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