RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Tue Apr 1 12:25:36 UTC 2014
Uploaded:
http://cr.openjdk.java.net/~jwilhelm/8037924/webrev.10/
In general would agree regarding avoiding duplicated code, but when it comes to
the control flow I agree with Erik. More hard to read code leads to harder
maintenance than duplicating a few lines of code.
/Jesper
Andrey Zakharov skrev 1/4/14 11:05:
> Hi, Erik. Thanks for comments.
>
> On 01.04.2014 10:49, Erik Helin wrote:
>> Andrey,
>>
>> a couple of comments:
>> - We do not use the @author tag in the tests
>> (if you've seen other tests have the @author tag, then that is a bug)
> removed
>> - I'm not a big fan of using inheritance for sharing code between
>> tests because it makes it very hard to open a test, e.g.
>> TestHumongousShrinkHeap.java, and see what it does.
> Agreed, I'm too. But doubled code leads to harder maintenance.
>>
>> The code you share in TestShrinkHeap is basically the check and the
>> control flow of the tests. I would prefer if you could move the
>> control flow (System.gc(), sample, eat, sample, free, check samples)
>> into the tests and then write helper functions for retrieving the
>> flag values.
>>
>> As for the check, please use the assertions in
>> testlibrary/com/oracle/java/testlibrary/Asserts.java instead of
>> throwing a RuntimeException.
> Good point, changed.
> Jesper, can we upload webrev.10 from attachment? Thanks.
> I'm still need GC reviewers to approve push.
>
> Thanks.
>>
>> Thanks,
>> Erik
>>
>> On 2014-03-31 13:46, Jesper Wilhelmsson wrote:
>>> 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