RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Fri Mar 7 17:41:04 UTC 2014
The new webrev is uploaded here:
http://cr.openjdk.java.net/~jwilhelm/7607562/webrev.02/
/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