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

[Thomas Schatzl]

Hi Bengt,

  thanks for the review :)

On Fri, 2015-08-07 at 14:17 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> On 2015-08-06 16:39, Thomas Schatzl wrote:
[...]
> 
> 
> 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().

The problem is that all callers of the method need the parameter, and
need to first check whether it should throw away the current plab and
then try direct allocation.
So a split here would require the change to duplicate its code in all
its callers, which seems against coding practices to me. I do think
there is an opportunity here to refactor the method into its simpler
parts.

Since I am going to revisit that method in JDK-8067339 I would like to
defer this change there. This would also give me less headache applying
that change :)

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

Done.

I also undid the rename of the method
G1PLABAllocator::allocate_inline_or_new_plab() to its original name
G1PLABAllocator::allocate_direct_or_new_plab() to meet Jon's comment
about direct vs. inline naming the review for JDK-8073013.

An additional "const" that has been required for the now-removed
has_more_free_regions() method in in the original patch in
heapRegionSet.hpp. I removed that change too.

New changes at:
http://cr.openjdk.java.net/~tschatzl/8003237/webrev.2/ (full)
http://cr.openjdk.java.net/~tschatzl/8003237/webrev.1_to_2/ (diff)

Thanks,
  Thomas