[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed
From:       Jesper Wilhelmsson <jesper.wilhelmsson () oracle ! com>
Date:       2013-09-30 20:23:10
Message-ID: 5249DDAE.3050700 () oracle ! com
[Download RAW message or body]

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
>
>
[prev in list] [next in list] [prev in thread] [next in thread]