RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed

[Thomas Schatzl]

Hi,

On Tue, 2013-10-22 at 08:57 +0200, Jesper Wilhelmsson wrote:
> Hi,
> 
> I managed to break out a few more parts from the main bugfix that can be pushed 
> separately. The bugfix patch is still fairly large but I hope that it is clean 
> enough to be reviewable at this point.
> 
> Parts broken out:
> 
> [1] Remove unnecessary code in GenRemSet
>      https://bugs.openjdk.java.net/browse/JDK-8026851
>      webrev: http://cr.openjdk.java.net/~jwilhelm/8026851/
> 
> [2] Use restricted_align_down in collector policy code
>      https://bugs.openjdk.java.net/browse/JDK-8026852
>      webrev: http://cr.openjdk.java.net/~jwilhelm/8026852/
> 
> [3] Prepare GC code for collector policy regression fix
>      https://bugs.openjdk.java.net/browse/JDK-8026853
>      webrev: http://cr.openjdk.java.net/~jwilhelm/8026853/

Look good.

> 
> The remaining bugfix:
> 
> [4] assert(eden_size > 0 && survivor_size > 0) failed: just checking
>      https://bugs.openjdk.java.net/browse/JDK-8016309
>      webrev: http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.2/

GenCollectorPolicy.cpp:

226 // Young space must be aligned and have room for eden + two
survivors
227 size_t GenCollectorPolicy::young_gen_size_lower_bound() {
228   return align_size_up(3 * _space_alignment, _gen_alignment);
229 }

I think the comment should read: "Young *generation* must be aligned and
have remove for eden and two survivor spaces"

The same comment is in the header file. I recommend having it once in
the header file.

In universe.cpp, Universe::initialize_heap(), initialize_all() is called
twice for the G1 collector I think.

In the test I think you need to add -XX:+IgnoreUnrecognizedVMOptions to
the process builders as VM variants that do not have G1 do not recognize
-XX:G1HeapRegionSize.
Or alternatively only add this flag if G1 is available. There is some
code in the g1/TestSummarizeRSetStatsTools.java (isRunningG1GC()) for
that already. Maybe this could be put into the testlibrary.

Also, I think the test calls the VM to test without passing the
collector specified in the @run tag. I.e. regardless of the test the
original VM is run, the MaxNewSize checks are always done on the default
collector.
Again, g1/TestSummarizeRSetStatsTools.java contains some code for that.

The test also contains some commented out code in line 81 without
reason.

Btw, when applying the patches to latest hotspot-gc, beginning with the
third patch there has been some merge error.

Could you have a look at these issues?

Thanks,
Thomas