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

Tue Oct 22 10:59:08 UTC 2013

Hi Jesper,

On 2013-10-22, 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/
> 
> [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/

I think it is better to keep the conversative_max_heap_alignment() in
g1CollectedHeap.cpp, I don't want our header files to grow.

Otherwise, this change looks good.

Thanks,
Erik

> 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
> >>>
> >>>