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

[Tao Mao]

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?

>
> 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
>>
>>
>>
>