CRR (XS): 7113006: G1: excessive ergo output when an evac failure happens

Tony Printezis tony.printezis at
Wed Dec 28 16:49:09 UTC 2011

Hi all,

New webrev based on what I discussed with Jon below:


On 12/28/2011 11:06 AM, Tony Printezis wrote:
> Hi Jon,
> Thanks for the prompt code review! Inline.
> On 12/28/2011 10:16 AM, Jon Masamitsu wrote:
>> Tony,
>> Change looks fine.   Is it possible for a second thread to come in 
>> and take
>> the new region just created by the expand?
> No. When a to-space region is replaced during a GC (whether it's 
> survivor or old) we take the FreeList_lock (which guards the region 
> free list). In the case of survivors, new_region() will (eventually) 
> be called from attempt_allocation_locked():
> inline HeapWord* G1CollectedHeap::survivor_attempt_allocation(size_t
> word_size) {
>   assert(!isHumongous(word_size),
>          "we should not be seeing humongous-size allocations in this 
> path");
>   HeapWord* result = 
> _survivor_gc_alloc_region.attempt_allocation(word_size,
>                                                       false /* 
> bot_updates */);
>   if (result == NULL) {
>     MutexLockerEx x(FreeList_lock, Mutex::_no_safepoint_check_flag);
>     result = 
> _survivor_gc_alloc_region.attempt_allocation_locked(word_size,
>                                                       false /* 
> bot_updates */);
>   }
>   ...
> (for old regions, the code is similar to the above)
> Also note that the only time new_region() will try to expand the heap 
> (i.e., do_expand == true) is when we allocate a to-space region during 
> GC. That method is also used for mutator allocation regions but in 
> that case do_expand == false. I'll actually put an extra assert to 
> make sure that it's clear in the code.
>> Not part of your change, but is this comment warranted?
>>>   601       // Even though the heap was expanded, it might not have 
>>> reached
>>>   602       // the desired size. So, we cannot assume that the 
>>> allocation
>>>   603       // will succeed.
>> I like comments but this one confused me a bit.  Where are we assuming
>> that the allocation will succeed?
> Apologies, the comment is clearly unclear. :-) After expansion 
> succeeds we call:
>       res = _free_list.remove_head_or_null();
> which will do what you expect (return the head or NULL). We could have 
> called:
>       res = _free_list.remove_head();
> which assumes the free list head is non-NULL. So "we cannot assume 
> that the allocation will succeed" refers to the use of the 
> remove_head_or_null() method, instead of remove_head(). (As I said, 
> you're totally right that this was very obscure.)
> Having said that, I think that if expansion succeeds the free list is 
> also guaranteed to be non-empty. new_region() will always allocate a 
> single region and we always expand the heap by an amount aligned to 
> the heap region size (AFAIK at least). So, I _think_ using 
> remove_head() there is probably OK. But I don't like tempting fate :-) 
> and we just expanded the heap, so the extra check whether head is NULL 
> or not is unlikely to cause any performance issues. So I'll leave the 
> code as is and update the comment.
> I'll send out a new webrev shortly.
> Tony
>> Seems like we're just doing the necessary
>> thing by getting a region off the freelist.  Yes, the result might be 
>> null but
>> we still need to try and get a region.
>> Jon
>> On 12/28/2011 4:04 AM, Tony Printezis wrote:
>>> Hi all,
>>> Can I have a couple of code review for this very small change?
>>> The excessive ergo output was generated by repeated failed heap 
>>> expansion attempts during a GC. I slightly expanded (pun 
>>> unintended!) the scope of the CR to not only stop generating the 
>>> ergo output after the first failed heap expansion but also to not 
>>> re-attempt the heap expansion. If heap expansion fails once, it will 
>>> probably fail again so it's pointless to keep trying. Without doing 
>>> a careful and in-depth performance analysis I see a mild improvement 
>>> in GC times when an alloc failure happens with this fix.
>>> Tony

More information about the hotspot-gc-dev mailing list