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

[Jesper Wilhelmsson]

Thanks for the review John!
/Jesper


John Coomes skrev 1/11/13 5:00 PM:
> Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
>> 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/
>> ...
>
> Looks good to me. :-)
>
> -John
>
>> 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
>>>>>