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

[Jon Masamitsu]

Tao,

Looks good.

Jon

On 11/21/2013 2:30 PM, Tao Mao wrote:
> 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
>>>
>>>
>>>