RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Andrey Zakharov
andrey.x.zakharov at oracle.com
Tue Apr 1 09:05:05 UTC 2014
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.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>
>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8037924.tar.gz
Type: application/gzip
Size: 155404 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140401/8b25c4a0/8037924.tar.gz>
More information about the hotspot-gc-dev
mailing list