RFR (S): 8003237: G1: Reduce unnecessary (and failing) allocation attempts when handling an evacuation failure

[Bengt Rutisson]

Hi Thomas,

On 2015-08-06 16:39, Thomas Schatzl wrote:
> (sorry if that mail is received multiple times, getting send error just
> for this mail)
>
> Hi Jon,
>
>    thanks for the review...
>
> On Wed, 2015-08-05 at 09:36 -0700, Jon Masamitsu wrote:
>> Thomas,
>>
>> I think that this method gets called if a new region is allocated
>> via allocate_inline_or_new_plab()
>>
>> "src/share/vm/gc/g1/g1CollectedHeap.cpp"
>>
>> 6436 HeapRegion* G1CollectedHeap::new_gc_alloc_region(size_t
> word_size,
>> 6437                                                  uint count,
>> 6438                                                  InCSetState
> dest) {
>> 6439   assert(FreeList_lock->owned_by_self(), "pre-condition");
>> 6440
>> 6441   if (count < g1_policy()->max_regions(dest)) {
>>
>> Might the g1_policy()->max_regions() conflict with the test
>> in has_more_free_regions() which considers all free regions
>> (even uncommitted).   That is, has_more_free_regions() will
>> allow allocation attempts that will not success because of
>> max_regions().
> No, because the worst case is that we go into the take-the-Mutex case
> too often (which is bad).
>
> However, thinking about this again, I found another option (which is
> much clearer to see after recent refactoring of g1allocator classes): if
> G1AllocRegion::attempt_allocation_locked() fails, we know that there is
> really no more space in the heap left.
>
> In this case, just set an internal flag, instead of relying on the
> has_more_free_regions() from G1CollectedHeap.
>
>> http://cr.openjdk.java.net/~tschatzl/8003237/webrev/src/share/vm/gc/g1/g1ParScanThreadState.hpp.frames.html
>>
>>>     74   // Indicates whether in the last generation (old) there is
> no more space
>>>     75   // available for allocation.
>>>     76   bool _no_more_space_in_old;
>> Would a name like _old_is_full suit you here in place of
>> _no_more_space_in_old?  I like positives instead
>> negatives.
> Done.
>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8003237/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8003237/webrev.0_to_1 (partial)
>
> I think this one looks better, performs the same (tested) and does not
> introduce the additional dependency on G1CollectedHeap.
>
> This also automatically takes into consideration restrictions like in
> G1CollectedHeap::new_gc_alloc_region() without the need to repeat them.
>
> The addition of the new methods as virtual has been intentional as other
> Allocators may have different views on when the heap for the given
> context is full.


This looks good to me. A couple of minor comments:

What do you think of splitting 
G1PLABAllocator::allocate_inline_or_new_plab() up into two methods? One 
that allocates in plab and one that does the inline allocation. That way 
G1ParScanThreadState::copy_to_survivor_space() does not have to pass 
around the plab_refill_failed state. Instead it just first tries PLAB 
and then inline and it knows what happened and can pass the correct 
value to allocate_in_next_plab().

Would you mind calling _last_gen_is_full _old_gen_is_full instead? I 
think it is more clear and almost everywhere where _last_gen_is_full is 
being used there is a comment saying "old gen".

I'm on vacation next week, but I am fine with you push the changes with 
me as a reviewer if the changes contain more or less what you have now 
plus potentiality the updates I proposed.

Thanks,
Bengt

>
> Thanks,
>    Thomas
>