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

[John Coomes]

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.

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

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.

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

	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)

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

generationSizer.hpp:

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

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

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

-John