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

Tue Oct 22 08:46:42 UTC 2013

Thank you for looking at this Stefan!
/Jesper

Stefan Karlsson skrev 22/10/13 10:01 AM:
>
> On 2013-10-22 08:57, Jesper Wilhelmsson wrote:
>> 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/
>
> Looks good.
>
>>
>> [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/
>
> Looks good.
>
> StefanK
>
>>
>> [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
>>>>>
>>>>>
>