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

[John Coomes]

Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
> 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.

Much easier that way, thanks.

> The new patch:
> 
> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.4/
> 
> Comments inline.

Looks good, except something seems to be missing from the webrev.  See
comments below.

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

I see the new default_gen_alignment() in GenerationSizer, which is
fine.  But the webrev doesn't show what happened to the old
implementation in collectorPolicy.hpp - it's not there anymore, but
there's no diff showing that it was removed.

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

Wrapping the calls only would be fine, except you have to make sure
that you find all of them, and you don't get help from the compiler to
do it :-(.

Also, the usual convention has been to use DEBUG_ONLY() for single
lines of code, and 

	#ifdef ASSERT
	...
	#endif // ASSERT

for larger blocks (like whole methods, or groups of methods).  What
you have is ok, but it does make it hard for some editors to do proper
indentation.

-John

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