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

Tue Oct 22 06:57:53 UTC 2013

Hi,

I managed to break out a few more parts from the main bugfix that can be pushed 
separately. The bugfix patch is still fairly large but I hope that it is clean 
enough to be reviewable at this point.

Parts broken out:

[1] Remove unnecessary code in GenRemSet
     https://bugs.openjdk.java.net/browse/JDK-8026851
     webrev: http://cr.openjdk.java.net/~jwilhelm/8026851/

[2] Use restricted_align_down in collector policy code
     https://bugs.openjdk.java.net/browse/JDK-8026852
     webrev: http://cr.openjdk.java.net/~jwilhelm/8026852/

[3] Prepare GC code for collector policy regression fix
     https://bugs.openjdk.java.net/browse/JDK-8026853
     webrev: http://cr.openjdk.java.net/~jwilhelm/8026853/

The remaining bugfix:

[4] assert(eden_size > 0 && survivor_size > 0) failed: just checking
     https://bugs.openjdk.java.net/browse/JDK-8016309
     webrev: http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.2/

Thanks!
/Jesper

Jesper Wilhelmsson skrev 3/10/13 1:33 PM:
> 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
>>>
>>>