RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed
jesper.wilhelmsson at oracle.com
Fri Nov 1 15:35:20 UTC 2013
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:
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.
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:
>> 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.
>>> 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
>>> - 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
>> OK. Fixed.
>>> The method is big now and should be split (ok to do later, if
>> There are a few more cleanups of these classes planned so I'll put that on my
>> todo list.
>>> You've made initialize_size_info, initialize_flags() and
>>> initialize_size_info() private. They're protected in
>> 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
More information about the hotspot-gc-dev