RFR: tests for JDK-8028720 Make the Min/MaxHeapFreeRatio flags manageable
Andrey Zakharov
andrey.x.zakharov at oracle.com
Fri Mar 7 16:44:58 UTC 2014
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: 19627 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140307/1ddd85af/7607562.tar.gz>
More information about the hotspot-gc-dev
mailing list