RFR 8037924: CMM Testing: Check Min/MaxHeapFreeRatio flags allows to shrink the heap when using ParallelGC
Igor Ignatyev
igor.ignatyev at oracle.com
Sat Mar 29 16:08:20 UTC 2014
Andrey,
1. could you please move MemoryUsagePrinter and TestShrinkHeap to
'c.o.j.testlibrary.gc' package?
2. TestShrinkHeap
2.1.
> 32 * Fails if heap doesn't shrinks
type, it should be 'if heap doesn't shrink'
2.2.
> 39
redundant empty line
2.3.
> 41 public TestShrinkHeap() {
it's better to have constructors of abstract classes protected.
2.4.
> 44 public void test() {
I'd prefer to have it final
2.5.
> 69 public abstract void eat();
> 70 public abstract void free();
it's better to have these method protected, since we don't have in mind
to execute them by other classes.
3. parallelScavenge/TestDynShrinkHeap
3.1.
> 44 protected final TestDynamicVMOption minRatioOption;
> 45 protected final TestDynamicVMOption maxRatioOption;
private
3.2.
> 48 private int maxHeapFreeValue;
you don't need it, it can be a local variable in <init>
3.3.
> 62
> 74
redundant empty lines
3.4.
> 88 public static void gc() {
why isn't 'TestDynShrinkHeap.gc()' 'TestShrinkHeap.gc()'?
4. g1/TestHumongousShrinkHeap.java
4.1.
> 40 private static final ArrayList< ArrayList< byte[]>> garbage = new ArrayList<>();
redundant spaces in '< >'
4.2.
> 41 private static final int PageSize = 1024 * 1024; // 1M
> 42 private static final int PagesNum = 5;
please align these lines w/ code conventions:
>> According to section 9 in java code conventions[1]:
>>> The names of variables declared class constants and of ANSI constants
>>> should be all uppercase with words separated by underscores ("_").
>>> (ANSI constants should be avoided, for ease of debugging.)
4.3.
> 74 public static void eatList(List garbage, int count, int size) {
private
5. if you need assured fullGC, we have WhiteBox API for it --
s.h.WhiteBox.fullGC()
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