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

[Tao Mao]

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