CRR (XS): 7113006: G1: excessive ergo output when an evac failure happens
Tony Printezis
tony.printezis at oracle.com
Wed Dec 28 16:49:09 UTC 2011
Hi all,
New webrev based on what I discussed with Jon below:
http://cr.openjdk.java.net/~tonyp/7113006/webrev.1/
Tony
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?
>>>
>>> http://cr.openjdk.java.net/~tonyp/7113006/webrev.0/
>>>
>>> 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