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

[Jesper Wilhelmsson]

Hi,

I've got the necessary reviews for the cleanup part so I will start pushing it. 
It has been split into smaller pieces and a couple of new RFEs have been created 
for this:

JDK-8025852 - Remove unnecessary setters in collector policy classes
JDK-8025853 - Remove unnecessary uses of GenerationSizer
JDK-8025854 - Use "young gen" instead of "eden"
JDK-8025855 - Simplify GenRemSet code slightly
JDK-8025856 - Fix typos in the GC code

These are all minor cleanups that happened as a result of the major cleanup of 
the collector policy classes. There are a few cleanups in the big cleanup patch 
that are not covered by these RFEs. Another RFE may be created to cover them.

Thanks,
/Jesper


Jesper Wilhelmsson skrev 30/9/13 10:23 PM:
> Hi Thomas,
>
> Thanks for looking at this!
>
> I brought the repository up to date and have a new webrev including your
> suggestions here:
>
> http://cr.openjdk.java.net/~jwilhelm/8016309/Cleanup/webrev.1/
> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.1/
>
> Comments inline.
>
> Thomas Schatzl skrev 27/9/13 1:29 PM:
>> Hi,
>>
>> On Thu, 2013-09-19 at 23:38 +0200, Jesper Wilhelmsson wrote:
>>> Hi,
>>>
>>> Could I have a couple of reviews of this cleanup/bugfix.
>>>
>>> The bug I intended to fix is JDK-8016309 [1] in which the eden size could end up
>>> with a zero size due to minimum alignment and a too small young gen size.
>>> The alignment was increased in the fix for JDK-6725714 [2] which caused this
>>> bug.
>>>
>>> Fixing this in the current collector policies was quite messy.
>>>
>>> Attempts to fix this issue is actually already implemented in two different
>>> places,
>>> none of them results in a properly sized eden. Since Thomas had a patch
>>> to cleanup the collector policies to fix JDK-7057939 [3], I based my
>>> bugfix on his patch
>>> instead of fighting with the mess in the current collector policies. As Thomas
>>> wasn't actively working on finishing his cleanup patch I also continued that
>>> work to finish it.
>>
>> Thanks a lot for continuing the work!
>
> Thanks for starting it ;-)
>
>>> This cleanup fixes both bugs (JDK-8016309 and JDK-7057939). To extract them to
>>> different patches would be too much work and result in an intermediate state
>>> between the two patches where the ergonomics wouldn't work properly. I have
>>> however split the webrev into two parts:
>>>
>>> The first patch contains cosmetic changes (name changes, being consequent in
>>> using local instance variables vs using getters/setters, fixing typos in
>>> comments etc.).
>>>
>>> http://cr.openjdk.java.net/~jwilhelm/8016309/Cleanup/webrev/
>>>
>>> The second patch contains the major part of the cleanup work in the collector
>>> policies. The goal has been to make the code easier to follow and pull policy
>>> decisions that had leaked into other code back to the collector policy classes.
>>>
>>> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev/
>>>
>>> Please note that the default value of NewSize has changed from 1M to 2M. Since
>>> the minimum alignment was increased to 512K, the minimal possible young gen size
>>
>> ... on 64 bit machines, 256k on 32 bit (previously 64k for at least
>> serial gc I guess)?
>>
>> Shouldn't it be possible to still have a MaxHeapSize (or did you really
>> mean NewSize here?) of 1M for 32bit (I think this has been the previous
>> limit), i.e. each of eden, from, to and old gen a 256k "page"?
>
> I did mean NewSize. The MaxNewSize defaults to max_uintx and is later set
> ergonomically. I didn't want to make the NewSize default value platform
> dependent, but 2M may be a bit big on 32bit platforms..?  I'll check with
> embedded to see how they feel about this.
>
>>> increased to 1.5M. Having a smaller default would be misleading as the
>>> ergonomics would increase it every time.
>>
>> I think we should talk to the embedded people about this, but I think
>> this 1.5M is a limitation for 64 bit only.
>
> Yes.
>
>>
>>> New tests has been added and all existing jtreg arguments tests has been used to
>>> verify that heap sizing ergonomics behaves as expected.
>>>
>>> Minor note:
>>> The method GenCollectedHeap::compute_new_generation_sizes was unused. In the
>>> cleanup patch I have commented it out in case anyone left it there on purpose.
>>> I'd prefer to remove it. (Actually the second patch removes the method unless
>>> someone really wants to keep it.)
>>
>> Here're the results of a first pass through the change: overall I think
>> it's good, but I do need another pass and some more testing. There's
>> already a few things that could be looked at.
>>
>> - In parallelScavengeHeap.cpp:108, it may be good to just pass the
>> collector policy to the constructor of AdjoiningGenerations instead of
>> the bunch of members - but it is fine with me as it is too.
>
> I agree. Updated.
>
>> - collectorPolicy.cpp:91, "was changed" -> "changed"
>
> Fixed.
>
>> - in collectorPolicy.cpp:385 there is a huge list of asserts. Line 329
>> contains a large part of these: is it possible to make a method like
>> "validate_ergonomics()" or so to include the ones that are the same
>> (should be most) and use it in the two places?
>
> I cleaned this up and removed the duplicated code. Thanks for pointing this out!
>
>> - collectorPolicy.cpp:491: is it possible to put the asserts after
>> printing the current min/initial/max_gen0 sizes?
>
> Fixed in all places.
>
>> - could the patch factor out common asserts in the blocks in
>> collectorPolicy.cpp:491 and :623?
>
> Fixed.
>
>> - in G1CollectorPolicy::post_heap_initialize() the heap region size is
>> updated; note that some earlier change already did this, putting the
>> update in heapRegion.cpp:177. I prefer to have these sort of updates
>> collected somewhere (e.g. in post_heap_initialize()), but in any case we
>> do not need to do it twice.
>
> Since setup_heap_region_size() where the first call was made is called before
> initialize_flags() I placed the single check in
> G1CollectorPolicy::initialize_flags where it belongs :-)
>
>> - in collectorPolicy.cpp, the expressions of the form
>> MAX2((uintx)align_size_down(<something>, _min_alignment),
>> _min_alignment) can be replaced by using the function
>> restricted_align_down().
>
> Fixed. Thanks for pointing that out. restricted_align_down was added by Stefan
> quite recently so it wasn't around when I did these changes.
>
>> - more cleanup: in GenCollectorPolicy::scale_by_NewRatio_aligned() the
>> code
>>
>> 217  size_t new_gen_size = x > _min_alignment ?
>> 218                     align_size_down(x, _min_alignment) :
>> _min_alignment;
>>
>> is just another form of restricted_align_down
>
> True. Fixed.
>
>> - in GenCollectorPolicy::initialize_flags(), line
>>
>> 293      uintx smallerMaxNewSize = align_size_down(MaxHeapSize -
>> _min_alignment, _min_alignment);
>>
>> you can set smallerMaxNewSize to MaxHeapSize - _min_alignment directly.
>> MaxHeapSize must be a multiple of _max_alignment, and _max_alignment a
>> multiple of _min_alignment.
>
> Fixed.
>
>> I think the same argument can be applied to line 304 and 306 and
>> possibly 312.
>
> 304 yes. 306 and 312 no. MaxNewSize and _min_gen0_size are not guarantied to be
> aligned to _min_alignment here.
>
>> - more possible uses of restricted_align_down in collectorPolicy.cpp
>> line 530 and 536.
>
> Fixed.
>
>> - I think there is no need to update MaxNewSize again in
>> GenCollectorPolicy::post_heap_initialize() if it has been taken care of
>> earlier already. Maybe just keep the assert.
>
> Yes, I missed to remove the code after having run the tests with the assert.
>
>> Thanks a lot for looking into this,
>
> Thanks for the review!
> /Jesper
>
>> Thomas
>>
>>