http://cr.openjdk.java.net/~stefank/7021322/webrev/

There's a bug in parallel scavenge when array chunking is used. After a
thread has succeeded in forwarding the pointer to a newly copied array,
it might change the length of the old object. This is done as a part of
the load balancing.
If another thread races with the forwarding thread, it might read the
incorrect array length and copy just a part of the array. When it later
sees that the object has already been forwarded and calls
unallocate_object, it uses the original length of the array to determine
the size to unallocate.

The fix is to pass the actual amount of memory that was allocated, to
unallocate_object.

Tested with the failing test.

StefanK

Search Discussions

  • Ramki Ramakrishna at Sep 12, 2011 at 5:14 pm
    Good catch. Fix looks good.

    I am also a bit leery of any unallocate failing. It seems to me that
    each unallocate
    should provably succeed. Thus the "return false" path seems bogus to me,
    and should
    (in my book) be replaced with either an assert(false) or ShouldNotReachHere.
    Then the object filling code becomes redundant and can be removed.

    On the other hand, may be I am missing some reason why the unallocate might
    legitimately fail and the object filling becomes necessary in that case.

    Looks good otherwise.
    -- ramki
    On 9/12/2011 1:49 PM, Stefan Karlsson wrote:
    http://cr.openjdk.java.net/~stefank/7021322/webrev/

    There's a bug in parallel scavenge when array chunking is used. After
    a thread has succeeded in forwarding the pointer to a newly copied
    array, it might change the length of the old object. This is done as a
    part of the load balancing.
    If another thread races with the forwarding thread, it might read the
    incorrect array length and copy just a part of the array. When it
    later sees that the object has already been forwarded and calls
    unallocate_object, it uses the original length of the array to
    determine the size to unallocate.

    The fix is to pass the actual amount of memory that was allocated, to
    unallocate_object.

    Tested with the failing test.

    StefanK
  • Ramki Ramakrishna at Sep 12, 2011 at 5:21 pm
    Sorry, that came out all wrong. The filling, as the comment says, is
    necessary when
    the allocation is not from a PLAB but direct into the space. So that's fine.

    The issue is when the object start is in the current PLAB, but its end
    is not
    equal to the top. It seems as though it should not ever occur that the
    object start is in the PLAB but its end is anywhere other than the top of
    the PLAB. So it would be nice to check for that in the unallocate code.

    Sorry for the incorrect comments last time.
    -- ramki
    On 9/12/2011 5:14 PM, Ramki Ramakrishna wrote:
    Good catch. Fix looks good.

    I am also a bit leery of any unallocate failing. It seems to me that
    each unallocate
    should provably succeed. Thus the "return false" path seems bogus to
    me, and should
    (in my book) be replaced with either an assert(false) or
    ShouldNotReachHere.
    Then the object filling code becomes redundant and can be removed.

    On the other hand, may be I am missing some reason why the unallocate
    might
    legitimately fail and the object filling becomes necessary in that case.

    Looks good otherwise.
    -- ramki
    On 9/12/2011 1:49 PM, Stefan Karlsson wrote:
    http://cr.openjdk.java.net/~stefank/7021322/webrev/

    There's a bug in parallel scavenge when array chunking is used. After
    a thread has succeeded in forwarding the pointer to a newly copied
    array, it might change the length of the old object. This is done as
    a part of the load balancing.
    If another thread races with the forwarding thread, it might read the
    incorrect array length and copy just a part of the array. When it
    later sees that the object has already been forwarded and calls
    unallocate_object, it uses the original length of the array to
    determine the size to unallocate.

    The fix is to pass the actual amount of memory that was allocated, to
    unallocate_object.

    Tested with the failing test.

    StefanK
  • Bengt Rutisson at Sep 12, 2011 at 8:25 pm
    Looks good to me too!

    Impressed that you found the issue and fixed it this fast!

    If you want to you can update the copyright year to 2011 in
    psPromotionLAB.cpp and psPromotionLAB.hpp.

    Bengt
    On 2011-09-13 02:21, Ramki Ramakrishna wrote:
    Sorry, that came out all wrong. The filling, as the comment says, is
    necessary when
    the allocation is not from a PLAB but direct into the space. So that's
    fine.

    The issue is when the object start is in the current PLAB, but its end
    is not
    equal to the top. It seems as though it should not ever occur that the
    object start is in the PLAB but its end is anywhere other than the top of
    the PLAB. So it would be nice to check for that in the unallocate code.

    Sorry for the incorrect comments last time.
    -- ramki
    On 9/12/2011 5:14 PM, Ramki Ramakrishna wrote:
    Good catch. Fix looks good.

    I am also a bit leery of any unallocate failing. It seems to me that
    each unallocate
    should provably succeed. Thus the "return false" path seems bogus to
    me, and should
    (in my book) be replaced with either an assert(false) or
    ShouldNotReachHere.
    Then the object filling code becomes redundant and can be removed.

    On the other hand, may be I am missing some reason why the unallocate
    might
    legitimately fail and the object filling becomes necessary in that case.

    Looks good otherwise.
    -- ramki
    On 9/12/2011 1:49 PM, Stefan Karlsson wrote:
    http://cr.openjdk.java.net/~stefank/7021322/webrev/

    There's a bug in parallel scavenge when array chunking is used.
    After a thread has succeeded in forwarding the pointer to a newly
    copied array, it might change the length of the old object. This is
    done as a part of the load balancing.
    If another thread races with the forwarding thread, it might read
    the incorrect array length and copy just a part of the array. When
    it later sees that the object has already been forwarded and calls
    unallocate_object, it uses the original length of the array to
    determine the size to unallocate.

    The fix is to pass the actual amount of memory that was allocated,
    to unallocate_object.

    Tested with the failing test.

    StefanK
  • Stefan Karlsson at Sep 13, 2011 at 1:00 am

    On 09/13/2011 02:21 AM, Ramki Ramakrishna wrote:
    Sorry, that came out all wrong. The filling, as the comment says, is
    necessary when
    the allocation is not from a PLAB but direct into the space. So that's
    fine.

    The issue is when the object start is in the current PLAB, but its end
    is not
    equal to the top. It seems as though it should not ever occur that the
    object start is in the PLAB but its end is anywhere other than the top of
    the PLAB. So it would be nice to check for that in the unallocate code.
    I replaced:
    assert(object_end <= top(), "Object crosses promotion LAB boundary");
    ...
    if (object_end == top())

    with:
    assert(object_end == top(), "Not matching last allocation");

    thanks for the review.
    StefanK
    Sorry for the incorrect comments last time.
    -- ramki
    On 9/12/2011 5:14 PM, Ramki Ramakrishna wrote:
    Good catch. Fix looks good.

    I am also a bit leery of any unallocate failing. It seems to me that
    each unallocate
    should provably succeed. Thus the "return false" path seems bogus to
    me, and should
    (in my book) be replaced with either an assert(false) or
    ShouldNotReachHere.
    Then the object filling code becomes redundant and can be removed.

    On the other hand, may be I am missing some reason why the unallocate
    might
    legitimately fail and the object filling becomes necessary in that case.

    Looks good otherwise.
    -- ramki
    On 9/12/2011 1:49 PM, Stefan Karlsson wrote:
    http://cr.openjdk.java.net/~stefank/7021322/webrev/

    There's a bug in parallel scavenge when array chunking is used.
    After a thread has succeeded in forwarding the pointer to a newly
    copied array, it might change the length of the old object. This is
    done as a part of the load balancing.
    If another thread races with the forwarding thread, it might read
    the incorrect array length and copy just a part of the array. When
    it later sees that the object has already been forwarded and calls
    unallocate_object, it uses the original length of the array to
    determine the size to unallocate.

    The fix is to pass the actual amount of memory that was allocated,
    to unallocate_object.

    Tested with the failing test.

    StefanK
  • Ramki Ramakrishna at Sep 13, 2011 at 2:03 am

    On 9/13/2011 1:00 AM, Stefan Karlsson wrote:
    I replaced:
    assert(object_end <= top(), "Object crosses promotion LAB boundary");
    ...
    if (object_end == top())

    with:
    assert(object_end == top(), "Not matching last allocation");
    Excellent; thanks!
    -- ramki

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouphotspot-gc-dev @
categoriesopenjdk
postedSep 12, '11 at 1:49p
activeSep 13, '11 at 2:03a
posts6
users3
websiteopenjdk.java.net

People

Translate

site design / logo © 2022 Grokbase