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

[Jon Masamitsu]

On 8/6/2015 7:27 AM, Thomas Schatzl wrote:
> 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.

I like the changes.  Looks good.  Thanks for the name change to 
old_is_full like
names.

Jon

>
> Thanks,
>    Thomas
>
>
>
>