RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Igor Ignatyev
igor.ignatyev at oracle.com
Mon Mar 31 09:16:06 UTC 2014
Andrey,
1. TEST.groups:
please update copyright year:
> 2 # Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
should be
> 2 # Copyright (c) 2013, 2014, Oracle and/or its affiliates. All rights reserved.
2. TestShrinkHeap.java
> 22 */
> 23 package com.oracle.java.testlibrary.gc;
add empty line between 22 and 23 as you do in all other files,
otherwise it looks good to me.
Keep in mind I'm not a Reviewer, but I can be mentioned in 'Reviewed-by'
section )
Thanks,
Igor
On 03/31/2014 10:33 AM, Andrey Zakharov wrote:
> Hi, team
> webrev: http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.08/
> bug: https://bugs.openjdk.java.net/browse/JDK-8037924
> bug: https://bugs.openjdk.java.net/browse/JDK-8037925
>
> Thanks
>
> On 30.03.2014 22:06, Jesper Wilhelmsson wrote:
>> Webrev uploaded:
>>
>> http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.08/
>>
>> /Jesper
> Thank you, Jesper.
>
>>
>> Andrey Zakharov skrev 30/3/14 10:03:
>>> Hi! Here is webrev.08,
>>> Jesper, can you upload it? Thanks!
>>>
>>> Igor, thanks for well detailed review!
>>>
>>> On 30.03.2014 02:33, Igor Ignatyev wrote:
>>>> Andrey,
>>>>
>>>> 1. both tests:
>>>>> 48 public void eat() {
>>>>> 61 public void free() {
>>>> change visibility of concrete methods to protected
>>>> 2. TestHumongousShrinkHeap
>>>>> 32 */
>>>>> 33 import com.oracle.java.testlibrary.gc.MemoryUsagePrinter;
>>>> add empty line between these lines or remove empty line #32 in
>>>> TestDynShrinkHeap
>>>>> 62 //do not free last one list
>>>> space after '//'
>>>> 3. both tests:
>>>>> 26 * @bug 8037924
>>>>> 26 * @bug 8037925
>>>> after @bug you should specify product bug ids which can be tested by
>>>> this
>>>> test. I guess 8016479 instead of 8037924, and 8036025/8037925.
>>>>
>>>> otherwise it looks good, thank you for your work and patience )
>>>>
>>>> Igor
>>>>
>>>> On 03/30/2014 02:21 AM, Jesper Wilhelmsson wrote:
>>>>> Hi, New webrev in place:
>>>>>
>>>>> http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.07/
>>>>>
>>>>> /Jesper
>>>>>
>>>>> Andrey Zakharov skrev 29/3/14 20:57:
>>>>>> Hi, here updated webrev.
>>>>>> Jesper, can you, please, upload it to your webspace.
>>>>>> Igor, please, see comment below, everything uncommented is done.
>>>>>> Thanks.
>>>>>>
>>>>>> On 29.03.2014 20:08, Igor Ignatyev wrote:
>>>>>>> why isn't 'TestDynShrinkHeap.gc()' 'TestShrinkHeap.gc()'?
>>>>>> I cut this out.
>>>>>>> 5. if you need assured fullGC, we have WhiteBox API for it --
>>>>>>> s.h.WhiteBox.fullGC()
>>>>>>>
>>>>>> Not sure is it applicable for me, because every time in feature
>>>>>> ticket
>>>>>> descriptions I see "System.gc()", so... let it be.
>>>>>>> Thanks
>>>>>>> Igor
>>>>>>>
>>>>>>> On 03/29/2014 07:37 PM, Andrey Zakharov wrote:
>>>>>>>> Hi, team!
>>>>>>>> webrev: http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.06/
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8037924
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8037925
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>> On 29.03.2014 16:19, Andrey Zakharov wrote:
>>>>>>>>> Hi, Igor, here is just non-uploaded attachment to review. It
>>>>>>>>> will be
>>>>>>>>> upload later.
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>> On 29.03.2014 12:10, Igor Ignatyev wrote:
>>>>>>>>>> TestDynShrinkHeap:
>>>>>>>>>>> 45 public static byte[] temp;
>>>>>>>>>> unused field
>>>>>>>>>>
>>>>>>>>>> it looks like 'g1/TestHumongousShrinkHeap.java' and
>>>>>>>>>> 'parallelScavenge/TestDynShrinkHeap.java' should have one super
>>>>>>>>>> class,
>>>>>>>>>>
>>>>>>>>>> abstract class TestDyn {
>>>>>>>>>>
>>>>>>>>>> protected static final String MIN_FREE_RATIO_FLAG_NAME =
>>>>>>>>>> "MinHeapFreeRatio";
>>>>>>>>>> protected static final String MAX_FREE_RATIO_FLAG_NAME =
>>>>>>>>>> "MaxHeapFreeRatio";
>>>>>>>>>>
>>>>>>>>>> protected abstract void eat();
>>>>>>>>>>
>>>>>>>>>> protected abstract void free();
>>>>>>>>>>
>>>>>>>>>> public final void test() {
>>>>>>>>>> gc();
>>>>>>>>>> MemoryUsagePrinter.printMemoryUsage("init");
>>>>>>>>>> eat();
>>>>>>>>>> MemoryUsagePrinter.printMemoryUsage("eaten");
>>>>>>>>>> MemoryUsage muFull =
>>>>>>>>>> ManagementFactory.getMemoryMXBean().getHeapMemoryUsage();
>>>>>>>>>> free();
>>>>>>>>>> MemoryUsage muFree =
>>>>>>>>>> ManagementFactory.getMemoryMXBean().getHeapMemoryUsage();
>>>>>>>>>> if (!(muFree.getCommitted() < muFull.getCommitted())) {
>>>>>>>>>> throw new RuntimeException(
>>>>>>>>>> String.format("committed free heap size is not less than
>>>>>>>>>> committed full heap size, heap hasn't been shrunk?%n%s = %s%n%s =
>>>>>>>>>> %s",
>>>>>>>>>> MIN_FREE_RATIO_FLAG_NAME,
>>>>>>>>>>
>>>>>>>>>> ManagementFactoryHelper.getDiagnosticMXBean().getVMOption(MIN_FREE_RATIO_FLAG_NAME).getValue(),
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> MAX_FREE_RATIO_FLAG_NAME,
>>>>>>>>>>
>>>>>>>>>> ManagementFactoryHelper.getDiagnosticMXBean().getVMOption(MAX_FREE_RATIO_FLAG_NAME).getValue()
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> )
>>>>>>>>>> );
>>>>>>>>>> }
>>>>>>>>>> gc();
>>>>>>>>>> MemoryUsagePrinter.printMemoryUsage("done");
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> public static void gc() {
>>>>>>>>>> MemoryUsagePrinter.printMemoryUsage("before full GC");
>>>>>>>>>> System.gc();
>>>>>>>>>> MemoryUsagePrinter.printMemoryUsage("after full GC");
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Igor
>>>>>>>>>>
>>>>>>>>>> On 03/29/2014 10:59 AM, Andrey Zakharov wrote:
>>>>>>>>>>> Hi, here is updated webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.05/
>>>>>>>>>>> Any comments are welcome.
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> On 28.03.2014 19:28, Igor Ignatyev wrote:
>>>>>>>>>>>> On 03/28/2014 06:06 PM, Andrey Zakharov wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 28.03.2014 17:48, Igor Ignatyev wrote:
>>>>>>>>>>>>>> Andrey,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> And as I said in 8037925:
>>>>>>>>>>>>>>>>> You don't need to use enum here and string constant
>>>>>>>>>>>>>>>>> (Labels.*) in
>>>>>>>>>>>>>>>>> TestDynShrinkHeap. I've mistakenly think that
>>>>>>>>>>>>>>>>> MemoryUsagePrinter
>>>>>>>>>>>>>>>>> uses
>>>>>>>>>>>>>>>>> String argument to change its own behavior. Sorry for
>>>>>>>>>>>>>>>>> useless
>>>>>>>>>>>>>>>>> extra
>>>>>>>>>>>>>>>>> work.
>>>>>>>>>>>>>> Please remove class 'Labels', it's unnecessary and just
>>>>>>>>>>>>>> decrease
>>>>>>>>>>>>>> readability.
>>>>>>>>>>>>> Ok
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 26 * @key gc
>>>>>>>>>>>>>>>>> why do you need this?
>>>>>>>>>>>>>>>> Hm, used as common practice.
>>>>>>>>>>>>>> if you don't understand meaning of '@key gc', please don't
>>>>>>>>>>>>>> use
>>>>>>>>>>>>>> it.
>>>>>>>>>>>>> I think that this is used for test execution - to execute
>>>>>>>>>>>>> all test
>>>>>>>>>>>>> that
>>>>>>>>>>>>> relates to "gc" for example.
>>>>>>>>>>>> All test related to "gc" are located in test/gc, so we don't
>>>>>>>>>>>> need it
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> otherwise it looks good.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Igor
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 03/28/2014 05:37 PM, Andrey Zakharov wrote:
>>>>>>>>>>>>>>> Hi, here is updated webrev:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.04/
>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 28.03.2014 02:29, Igor Ignatyev wrote:
>>>>>>>>>>>>>>>> Andrey,
>>>>>>>>>>>>>>>> you've placed the wrong link, the correct one is
>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.02/.
>>>>>>>>>>>>>>>> And as I said in 8037925:
>>>>>>>>>>>>>>>>> You don't need to use enum here and string constant
>>>>>>>>>>>>>>>>> (Labels.*) in
>>>>>>>>>>>>>>>>> TestDynShrinkHeap. I've mistakenly think that
>>>>>>>>>>>>>>>>> MemoryUsagePrinter
>>>>>>>>>>>>>>>>> uses
>>>>>>>>>>>>>>>>> String argument to change its own behavior. Sorry for
>>>>>>>>>>>>>>>>> useless
>>>>>>>>>>>>>>>>> extra
>>>>>>>>>>>>>>>>> work.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> MemoryUsagePrinter:
>>>>>>>>>>>>>>>>> 44 float freeratio = 1f - (float)
>>>>>>>>>>>>>>>>> memusage.getUsed() /
>>>>>>>>>>>>>>>>> memusage.getCommitted();
>>>>>>>>>>>>>>>> I'd prefer '1.0f', but it's only my fad, it's not
>>>>>>>>>>>>>>>> necessary to
>>>>>>>>>>>>>>>> change.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> TestDynShrinkHeap:
>>>>>>>>>>>>>>>>> 96 StringBuilder strb = new StringBuilder("committed
>>>>>>>>>>>>>>>>> heap size under pressure is not less than committed
>>>>>>>>>>>>>>>>> full heap
>>>>>>>>>>>>>>>>> size,
>>>>>>>>>>>>>>>>> heap hasn't been shrunk?");
>>>>>>>>>>>>>>>>> 97 strb.append(System.getProperty("line.separator"));
>>>>>>>>>>>>>>>>> 98 strb.append(MinFreeRatioFlagName + " = " +
>>>>>>>>>>>>>>>>> minHeapFreeValue);
>>>>>>>>>>>>>>>>> 99 strb.append(System.getProperty("line.separator"));
>>>>>>>>>>>>>>>>> 100 strb.append(MaxFreeRatioFlagName + " = " +
>>>>>>>>>>>>>>>>> maxHeapFreeValue);
>>>>>>>>>>>>>>>>> 101 throw new
>>>>>>>>>>>>>>>>> RuntimeException(strb.toString());
>>>>>>>>>>>>>>>> it can be replaced by:
>>>>>>>>>>>>>>>> throw new RuntimeException(String.format("committed
>>>>>>>>>>>>>>>> heap size
>>>>>>>>>>>>>>>> under
>>>>>>>>>>>>>>>> pressure is not less than committed full heap size, heap
>>>>>>>>>>>>>>>> hasn't
>>>>>>>>>>>>>>>> been
>>>>>>>>>>>>>>>> shrunk?%n%s=%d%n%s=%n",MinFreeRatioFlagName,minHeapFreeValue,MaxFreeRatioFlagName,maxHeapFreeValue));
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> but it's also not necessary.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>> Igor
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 03/27/2014 04:42 PM, Andrey Zakharov wrote:
>>>>>>>>>>>>>>>>> Here is updated webrev with string constants:
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8037925/webrev.02/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 26.03.2014 20:02, Andrey Zakharov wrote:
>>>>>>>>>>>>>>>>>> Test to check that ParallelGC respect dynamic change of
>>>>>>>>>>>>>>>>>> MaxFreeRatio
>>>>>>>>>>>>>>>>>> and shrinks heap.
>>>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8037924/webrev/
>>>>>>>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8037924
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>
More information about the hotspot-gc-dev
mailing list