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

[Stefan Johansson]

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.

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