RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed
thomas.schatzl at oracle.com
Fri Sep 27 04:29:26 PDT 2013
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!
> 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"?
> 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.
- 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?
- collectorPolicy.cpp:491: is it possible to put the asserts after
printing the current min/initial/max_gen0 sizes?
- 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.
- in collectorPolicy.cpp, the expressions of the form
_min_alignment) can be replaced by using the function
- 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 -
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
- 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.
Thanks a lot for looking into this,
More information about the hotspot-gc-dev