RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed
jesper.wilhelmsson at oracle.com
Mon Sep 30 20:23:10 UTC 2013
Thanks for looking at this!
I brought the repository up to date and have a new webrev including your
Thomas Schatzl skrev 27/9/13 1:29 PM:
> On Thu, 2013-09-19 at 23:38 +0200, Jesper Wilhelmsson wrote:
>> Could I have a couple of reviews of this cleanup/bugfix.
>> The bug I intended to fix is JDK-8016309  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  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 , 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.).
>> 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.
>> 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.
>> 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"
> - 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?
> - 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
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
> 217 size_t new_gen_size = x > _min_alignment ?
> 218 align_size_down(x, _min_alignment) :
> is just another form of restricted_align_down
> - 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.
> 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.
> - 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!
More information about the hotspot-gc-dev