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

[Thomas Schatzl]

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!

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

> 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
MAX2((uintx)align_size_down(<something>, _min_alignment),
_min_alignment) can be replaced by using the function
restricted_align_down().

- 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

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

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