RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Tue Oct 22 11:48:38 UTC 2013
Thanks for the reviews Mikael!
/Jesper
Mikael Gerdin skrev 22/10/13 12:18 PM:
> Jesper,
>
> On Tuesday 22 October 2013 08.57.53 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/
>
> Code change 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/
>
> Code change looks good.
>
> /Mikael
>
> >
>
> > [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
>
More information about the hotspot-gc-dev
mailing list