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

[Jesper Wilhelmsson]

Hi John,

Thanks for the review!

I have fixed the things you commented on. Rather than making a new webrev for 
the entire change I made one to put on top of the previous change. This way you 
can see exactly what I have changed this time without having to look through all 
of it again.

The new patch:

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

Comments inline.

John Coomes skrev 30/10/13 6:28 PM:
> Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
>> Hi Thomas,
>>
>> Thanks for looking at this!
>>
>> Comments inline.
>>
>> ...
>> 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/
>
> Hi Jesper,
>
> Generally looks good; some comments below.
>
> collectorPolicy.hpp:
>
> 	default_gen_alignment() is 64K heapwords, which was the value
> 	from the old ParallelScavengeHeap::intra_heap_alignment().  The
> 	latter applied only to ParallelScavengeHeap, but the former now
> 	applies to CMS and SerialGC and G1, so the alignment for those
> 	GCs has been increased unnecessarily.  There should instead be a
> 	specialized default_gen_alignment() for ParallelScavenge (in
> 	GenerationSizer) with the larger value.

G1 has its own initialize_alignments so it should be good. For the others, the 
changes in the patch will allow CMS and Serial to keep using the smaller alignment.

> 	The assert_*_info() methods should be wrapped in DEBUG_ONLY() to
> 	ensure there's no code in the product vm.

How much do we trust the compilers to remove unused code? In the patch I have 
wrapped the whole methods in DEBUG_ONLY(), but if we think that the compilers 
will remove dead code it would be enough to wrap the calls.
Wrapping the whole methods will however allow the compiler to make sure they are 
not called in a product build.

>
> collectorPolicy.cpp:
>
> - GenCollectorPolicy::initialize_flags()
>
> 	If NewSize > MaxNewSize, the old code would increase MaxNewSize;
> 	the new code will exit w/an error.  That behavior shouldn't be
> 	changed as part of this bug fix, and not this late in jdk8.

Sure, I'll change that back. I do think we should try to be less forgiving when 
the user sets conflicting flags though. (This exit only happens if the user has 
set both NewSize and MaxNewSize.)

> 	There should be an assert that
> 		 _heap_alignment % _gen_alignment == 0
> 	either here or in assert_size_info().

Agreed. Fixed.

>
> 	Syntax nits:
>
> 	There are several local vars with camel-case names that should
> 	be lower-case with underscores (e.g., smallestNewSize should be
> 	smallest_new_size)

OK. Fixed.

> 	The method is big now and should be split (ok to do later, if
> 	necessary).

There are a few more cleanups of these classes planned so I'll put that on my 
todo list.

>
> generationSizer.hpp:
>
> 	You've made initialize_size_info, initialize_flags() and
> 	initialize_size_info() private.  They're protected in
> 	superclasses.

Oops. Fixed.

> 	I think a generationSizer.cpp is needed, instead of making these
> 	methods inline (they're not relevant to performance).

OK. Done.

> 	I don't like that after the page size is selected in
> 	GenerationSizer::initialize_size_info(), the entire sizing
> 	calculation is redone.  It's partially an artifact of the
> 	structure of the old code, so I'm ok with it for now as long as
> 	it's revisited later (but please file an issue).

I agree and I have an idea on how to do it better. I have created an RFE to fix 
this: https://bugs.openjdk.java.net/browse/JDK-8027649

Thanks!
/Jesper

>
> -John
>