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

[Jon Masamitsu]

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

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.

Jon

On 8/5/2015 1:25 AM, Thomas Schatzl wrote:
> Hi all,
>
>    just fyi, I had to update the webrev because of changes in the 8073052
> patch.
> Since there were no reviews of these changes yet, I updated in-place.
>
> There are no functional changes.
>
> Also, if somebody already had a look at the changes, and the question
> why G1ParScanThreadState::allocate_in_next_plab() only uses failed PLAB
> allocation as indicator that heap is full came up, the later patch
> JDK-8067336 "Allow that PLAB allocations at the end of regions are
> flexible" will allow that PLABs can get quite small as they are sized
> flexibly.
>
> I wanted to avoid introducing some even more arbitrary heuristic to
> determine when the heap is full now which will only a bit later be
> superseded again anyway.
>
> In any case current G1ParScanThreadState::allocate_in_next_plab() should
> try harder than the previous one by always trying an inline allocation
> even if it cannot allocate the object (supposedly smaller than a PLAB)
> using a new PLAB.
>
> Thanks,
>    Thomas
>
> On Mon, 2015-08-03 at 14:43 +0200, Thomas Schatzl wrote:
>> Hi,
>>
>>    this change is based on the latest version of the change for 8073052
>> that is out for review, and is still longing for another reviewer :)
>>
>> (I am okay with pushing it with only a single "R"eviewer since it is
>> only refactoring, but if somebody can find the time...)
>>
>> Thanks,
>>    Thomas
>>
>> On Mon, 2015-08-03 at 14:35 +0200, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>    can I have reviews for this small change that improves evacuation
>>> failure handling by short-cutting copy_to_survivor_space() when there is
>>> no more space left?
>>>
>>> In effect this makes the parallel phase during GC have the same
>>> performance as if there is no evacuation failure, and I could not
>>> measure regressions in the normal case either.
>>>
>>> The change implements two things: first, notice when there is no more
>>> space left in the last gen (i.e. old), and if so, short-cut
>>> ParScanThreadState::copy_to_survivor_space(). The other change is to not
>>> try to ask the free list for new regions if it's empty to avoid taking
>>> the lock in the time window between the first few evac failures and when
>>> space is completely empty.
>>>
>>> There is still the somewhat heavyweight cleanup phases after evacuation
>>> failure.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8003237
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8003237/webrev/
>>> Testing:
>>> jprt, lots of performance measurements with and without this change
>>> compiled in
>>>
>>> Thanks,
>>>    Thomas
>>>
>>>
>>
>