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

[Jesper Wilhelmsson]

Hi Thomas,

Thanks for looking at this!

Comments inline.

Thomas Schatzl skrev 24/10/13 1:48 PM:
> 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.

Thanks!

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

Yes. Fixed.

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

Since the comment describes implementation details rather than being a higher 
level description of the method, I removed the comment from the header file and 
moved it into the method definition.

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

Nope.

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

I changed the test to only add -XX:G1HeapRegionSize if running with G1.

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

As far as I can see the test runs with all GCs. The gcName is passed to the VM 
and looking in the logfiles when running jtreg I can see command lines with all 
different GCs.

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

Good catch! Removed.

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

A new webrev is available now with changes based on your comments and comments 
from John Coomes. Right now it applies cleanly to hotspot-gc. The bugfix builds 
on top of the cleanup, so apply the cleanup first. The cleanup is unchanged and 
has gotten sufficient reviews by now.

Cleanup: http://cr.openjdk.java.net/~jwilhelm/8016309/Cleanup/webrev.3/

Bugfix:  http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.3/


Thanks!
/Jesper

>
> Thanks,
> Thomas
>
>
>
>
>