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

Search Discussions

  • Jon Masamitsu at Dec 28, 2011 at 7:16 am
    Tony,

    Change looks fine. Is it possible for a second thread to come in and take
    the new region just created by the expand?

    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? 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
  • Tony Printezis at Dec 28, 2011 at 8:06 am
    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
  • Tony Printezis at Dec 28, 2011 at 8:49 am
    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
  • Jon Masamitsu at Dec 28, 2011 at 9:38 am
    Ship it!
    On 12/28/2011 8:49 AM, Tony Printezis wrote:
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouphotspot-gc-dev @
categoriesopenjdk
postedDec 28, '11 at 4:04a
activeDec 28, '11 at 9:38a
posts5
users2
websiteopenjdk.java.net

2 users in discussion

Tony Printezis: 3 posts Jon Masamitsu: 2 posts

People

Translate

site design / logo © 2022 Grokbase