RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Andrey Zakharov
andrey.x.zakharov at oracle.com
Mon Mar 31 10:15:47 UTC 2014
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.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8037924.tar.gz
Type: application/gzip
Size: 135800 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140331/d12bc64b/8037924.tar.gz>
More information about the hotspot-gc-dev
mailing list