Request for review: 8027915: TestParallelHeapSizeFlags fails with unexpected heap size on sparcv9

[Tao Mao]

Hi,

New webrev uploaded http://cr.openjdk.java.net/~tamao/8027915/webrev.02/

Note that FlagsValue.java file was first introduced in JDK-6521376 
(http://cr.openjdk.java.net/~tamao/6521376/webrev.05/). It was reviewed 
but could not be pushed due to the current development phase. and, 
getFlagLongValue() is not used here.

Thanks.
Tao

On 11/15/13 6:47 PM, Tao Mao wrote:
> Hi all,
>
> JDK8 cr. Need quick reviews to check it in. Thank you.
>
> new webrev:
> http://cr.openjdk.java.net/~tamao/8027915/webrev.01/
>
> Please see comments inline.
>
> Thanks.
> Tao
>
> On 11/13/13 2:58 AM, Thomas Schatzl wrote:
>> Hi Tao,
>>
>>    thanks for picking this up.
>>
>> On Tue, 2013-11-12 at 17:01 -0800, Tao Mao wrote:
>>> Hi all,
>>>
>>> 8027915: TestParallelHeapSizeFlags fails with unexpected heap size on
>>> sparcv9
>>> https://bugs.openjdk.java.net/browse/JDK-8027915
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~tamao/8027915/webrev.00/
>> Some notes:
>>
>> +    // bug 8027915&  8027960: compensate for some cases of large pages
>>
>> I do not think it is a good idea to mention bug numbers in the tests:
>> we can always get this information from the repository.
> Removed.
>>
>> Also I think the problem is not large pages, but how the collectors
>> align the spaces and generations.
>>
>> +    if (gcflag.equals("-XX:+UseParallelOldGC")) {
>>
>> I think it would be better to check for general use of parallel GC
>> instead of only the parallel old implementation - I think both do the
>> same heap sizing. I.e. whether the flag -XX:+UseParallelGC is set when
>> invoking the VM. I.e. let the getMinInitialMaxHeap() method return the
>> collector type used by parsing the output of -XX:+PrintFlagsFinal (which
>> needs to be added).
> Done. Look for "parallelGC".
>
>> I am not sure if that is needed though, see below.
>>
>> +        int numberOfSpaces = 4;
>> +        expectedHeapSize = Math.max(expectedHeapSize, numberOfSpaces *
>> v.minAlignment);
>>
>> I believe this is not correct in the general case. The formula used in
>> the VM (for generational collectors) is:
>>
>> expectedHeapSize = Math.max(expectedHeapSize, smallestPossibleHeapSize)
>>
>> where
>>
>> smallestNewSize = align_size_up(3 * _space_alignment, _gen_alignment);
>> smallestPossibleHeapSize = align_size_up(smallest_new_size +   // 
>> probably better named smallest_young_size
>>                                           
>> align_size_up(_space_alignment, _gen_alignment), // old gen
>>                                             _heap_alignment); // 
>> align for heap size
>>
>> The change needs to get the space alignment for that from the VM for
>> this.
> Quoted from my last response:
>
> "
> I know the internal alignment mechanism in VM code here. But, as 
> philosophy of testing, I don't like the idea to match up exactly every 
> line of code "in the box", because the code may be wrong in one way or 
> another especially when changes around it are introduced later.
>
> Rather, a test should test what it believes VM needs to do. With that, 
> it may catch other potential bugs in future. So, I'd rather keep the 
> test simple and non-intrusive (into the code).
> "
>>
>> Recently the "minAlignment" and "maxAlignment" members of
>> CollectorPolicy changed to spaceAlignment and heapAlignment. Is it
>> possible to adapt the naming in the test? This seems to be an
>> oversight in reviews for the earlier patch that changed that in the
>> Hotspot code.
> Changed in whitebox.cpp.
>>
>> After introducing all this to the test, there is no need for special
>> casing parallel gc too. In case of G1 it should be sufficient to set
>> space alignment and gen alignment to the same value.
>>
>>> changeset:
>>> Modified the test code so that it can deal with the case of using
>>> large pages.
>>>
>>> *Note another failure of gc/arguments/TestMinInitialErgonomics.java
>>> mentioned in the CR is dealt with in a new CR JDK-8028254.
>> You are correct, it is a slightly different but similar issue: I think
>> min and initial heap size are now (or always have been, did not look)
>> aligned to the heap alignment (formerly maxAlignment).
>>
>> The test now aligns the expected min/initial heap sizes to minAlignment
>> only, which is wrong.
>>
>> Thanks,
>>   Thomas
>>
>>
>>