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

[Jesper Wilhelmsson]

Hi John,

Indeed there was a part missing from the webrev. The webrev script didn't pick 
up all patches in my mercurial queue. I made a new webrev here:

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

Unfortunately I have already merged the latest patches into the big bugfix patch 
so the new webrev is a complete diff.

The change that was missing was that I removed the implementation of 
initialize_alignments in CollectorPolicy and GenCollectorPolicy, and added 
specific initialize_alignment in GenerationSizer and ConcurrentMarkSweepPolicy. 
(G1 already had one.) default_gen_alignment() was moved into GenerationSizer 
since it is specific to parallel scavenge.

In this new webrev I also used #ifdef ASSERT for the assert-methods in 
collectorPolicy.cpp as you suggested.

Thanks!
/Jesper


John Coomes skrev 1/11/13 2:34 PM:
> 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
>>>