RFR (S): 8022872: G1: Use correct GC cause for young GC triggered by humongous allocations

[Bengt Rutisson]

Hi Tony,

On 8/21/13 6:30 PM, Tony Printezis wrote:
> Hi Bengt,
>
> (surprise!)

A nice surprise! But a surprise indeed :)

> This is a useful change. Both webrevs look good. I have a slight 
> preference to the first one. I didn't like the use of 
> _allocation_failure on the second one here:
>
>  984       result = do_collection_pause(word_size, gc_count_before, 
> &succeeded,
>  985           GCCause::_allocation_failure);
>
> given that the allocation didn't really fail - the eden is just full. 
> The other use of _allocation_failure (in the 
> VM_G1CollectorForAllocation closure) really implies that an allocation 
> did fail and G1 is trying to take evasive action.

Thanks for looking at this! This was exactly the kind of feedback I was 
looking for. As I already mentioned I'm also fine with the first webrev 
and with this explanation I'm also in favor of that one. Unless anyone 
else has opinions I'll go with that one. Just need one more review...

Bengt

>
> Tony
>
>
>
>
> On 8/21/13 7:47 AM, Bengt Rutisson wrote:
>> Hi All,
>>
>> Could I have a couple for reviews for this small change?
>>
>> http://cr.openjdk.java.net/~brutisso/8022872/webrev.00/
>>
>> This change makes sure that we report "humongous allocation" as the 
>> GC cause when we trigger a young collection because we are attempting 
>> a humongous allocation.
>>
>> Before the change a log entry for such a young GC looked like this:
>>
>> [GC pause (G1 Evacuation Pause) (young) 1364M->1364M(2048M), 
>> 0.0004200 secs]
>>
>> After the change it looks like this:
>>
>> [GC pause (G1 Humongous Allocation) (young) 1364M->1364M(2047M), 
>> 0.0541270 secs]
>>
>> While doing this change I realized that the GC cause 
>> _g1_inc_collection_pause was only used in one more place. The slow 
>> path for allocations. There is already a cause called 
>> _allocation_failure, so I suggest that we use that one instead. This 
>> will eliminate the need for _g1_inc_collection_pause.
>>
>> In that case the webrev will look like this:
>>
>> http://cr.openjdk.java.net/~brutisso/8022872/webrev_incCause/
>>
>> I prefer this cleanup, but I'm fine with just doing the first one if 
>> we think _g1_inc_collection_pause cause to keep for some reason.
>>
>> Thanks,
>> Bengt
>