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

[Stefan Johansson]

Please see inline.

On 2013-11-13 20:22, Tao Mao wrote:
> Thank you for trying the patch. Please see inline.
>
> Tao
>
> On 11/13/13 5:10 AM, Stefan Johansson wrote:
>> Hi,
>>
>> Thanks for looking at this Tao and Thomas, see comment inline.
>>
>> On 2013-11-13 11:58, 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.
>>>
>>> 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). 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.
>>>
>>> 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.
>>>
>>> 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.
>> I like this proposal, we should try to avoid special cases and as 
>> much as possible. After fixing this please rerun the tests through 
>> JPRT. I tried you current patch and it still fails on sparcv9, since 
>> it is using v.minAlignment which is 512K instead of v.maxAlignment 
>> which is 4M. But I think, as Thomas suggested, that we should rename 
>> these to match the names in hotspot.
> Why, in your case, v.minAlignment = 512K and v.maxAlignment = 4M? 
> What's the large page size? Where does v.minAlignment (i.e. 
> gen_alignment) get that value?
Those alignments are parsed from the output of WhiteBox.printHeapSizes() 
which in turn prints values from the collector policy. minAlingment is 
corresponding to space_alignment and maxAlignment to heap_alignment, and 
I now think I must have run the JPRT run on a repo without my fix to set 
them to the same value.

Looking at the code they should not differ for this collector, and then 
the test shouldn't have failed. Sorry for the noise.

Cheers,
StefanJ
>
>>
>> Cheers,
>> StefanJ
>>>> 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
>>>
>>>
>>>
>>