RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Mon Mar 31 11:46:56 UTC 2014
Hi,
New webrev uploaded.
http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.09/
I'm not a Reviewer either so if you got Igor's blessing already, my review won't
be enough to push unfortunately.
/Jesper
Andrey Zakharov skrev 31/3/14 12:15:
> Hi,
> Jepser, here is updated webrev.09
> Thomas, Jesper can you review it as well?
> Thanks.
>
> On 31.03.2014 13:16, Igor Ignatyev wrote:
>> 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