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

[Thomas Schatzl]

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.

> 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