RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Erik Helin
erik.helin at oracle.com
Tue Apr 1 06:49:09 UTC 2014
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)
- 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.
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.
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