RFR: JDK-8073321: assert(q > prev_q) failed: we should be moving forward through memory

Thu Apr 14 09:14:36 UTC 2016

Hi Bengt,

On 2016-04-14 10:06, Bengt Rutisson wrote:
>
> Hi Jon,
>
> Thanks for looking at this!
>
> On 2016-04-14 05:09, Jon Masamitsu wrote:
>> Bengt,
>>
>> Looks very good.
>
> Thanks! Glad you like it!
>
>> A few comments.
>>
>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_amd_compact-refactor/src/share/vm/gc/shared/space.inline.hpp.frames.html
>>
>>
>>  319       // mark is pointer to next marked oop
>>
>> I think the comment was more meaningful when the previous
>> code accessed the mark word.  Maybe something like
>>
>> " The first word of the dead object contains a pointer to the next
>> live object
>> or end of space."
>
> I changed to your suggested wording. Updated webrev:
> http://cr.openjdk.java.net/~brutisso/8073321/webrev.01/

This looks good to me.
The early return in scan_and_compact is a bit unfortunate in my opinion 
but I don't have any good suggestion for how it should be improved.

Reviewed.

/Mikael

>
> Diff compared to last version:
> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-01.diff/
>
>>
>> =============  Begin of throwaway comment
>>
>> And for symmetry consider using something other
>> than forward_to() at
>>
>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00/src/share/vm/gc/shared/space.cpp.frames.html
>>
>>
>>  392   // store the forwarding pointer into the mark word
>>  393   if ((HeapWord*)q != compact_top) {
>>  394     q->forward_to(oop(compact_top));
>>
>
> I think this is actually the correct use of
> encode_pointer_as_mark()/decode_pointer(). What I tried to achieve with
> changing that we store pointer to the next live object in the mark word
> was that encode_pointer_as_mark()/decode_pointer() should only be used
> for forwarded objects. So, I'd prefer to leave this as is.
>
>>
>> Maybe add a next_live()/set_next_live() to CompactibleSpace to get/set
>> the
>> pointer.
>
> That's a good idea, but I prefer to leave that for a separate change.
>
>>
>> That's a throw-away comment.
>>
>> =============  End of throwaway comment
>>
>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00/src/share/vm/gc/shared/space.cpp.frames.html
>>
>>
>>  393   if ((HeapWord*)q != compact_top) {
>>  394     q->forward_to(oop(compact_top));
>>
>> Could you add an assertion here that compaction_top > q?
>> This is one of the odd places where there can be
>> surprises with the order of the address.  I'm thinking of compacting
>> from one survivor space to the other.
>
> Did you mean something like this?
>
>      assert(compact_top > q, "Compacting in the wrong direction:
> compact_top: " PTR_FORMAT " q: " PTR_FORMAT, p2i(compact_top), p2i(q));
>
> Unfortunately that is not always true because in SerialGC the young
> generation is below the old generation and we compact object from young
> into old. It also does not hold for G1, which can compact from a region
> at a lower address into a region at a higher address.
>
> (I've tried adding the assert above and it fails with both Serial and G1.)
>
> Thanks,
> Bengt
>
>
>
>>
>> Jon
>>
>>
>>
>> On 04/13/2016 07:54 AM, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> Could I have a couple of reviews for this change?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8073321
>>>
>>> Background:
>>>
>>> The assert happens in scan_and_adjust_pointers() and it could happen
>>> if scan_and_forward() is not behaving properly. What could happen is
>>> that the all object up to _first_dead should not be moved, which
>>> means that they should all be "unmarked" even though they are under
>>> the _first_dead value. However, the code just checks the first
>>> object. So, if scan_and_forward() would for some reason happen to
>>> mark the first object but not the rest we will read a mark word
>>> instead of a pointer to a live object in scan_and_adjust_pointers().
>>>
>>> I have not been able to reproduce this problem. But I changed the
>>> code in scan_and_adjust_pointers() to not just check the first
>>> object, but instead treat all objects up to _first_dead as non-moved
>>> objects. This should make the code more stable.
>>>
>>> To be able to understand the code while I was trying to look for a
>>> problem in it I had to rename some variables and make some
>>> refactorings. I think it is worth pushing this renamed and refactored
>>> code. And I think it is worth closing JDK-8073321 even if I have not
>>> been able to prove that my patch actually fixes the specific issue in
>>> the bug report. The problem has only ever happened twice so I think
>>> that with the better assert message added by JDK-8153203 we have a
>>> better chance of catching any other problems if it happens again and
>>> JDK-8073321 is closed.
>>>
>>> Here are some incremental webrevs to make it easier to review this
>>> change:
>>>
>>> Renaming variables in scan_and_adjust_pointers():
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_and_adjust_pointers-rename/
>>>
>>>
>>> The fix to make sure to treat all objects below _first_dead correctly:
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_and_adjust_pointers-refactor/
>>>
>>>
>>> Renaming variable in scan_and_forward() (and some small refactoring):
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_and_forward-rename/
>>>
>>>
>>> Refactoring of scan_and_forward(), in particular introducing a
>>> DeadSpacer class to handle the special case for the non-G1 case:
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_and_forward-refactor/
>>>
>>> (BTW, I would very much appreciate suggestions for a better name for
>>> DeadSpacer.)
>>>
>>> Renaming variables in scan_and_compact():
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_and_compact-rename/
>>>
>>>
>>> Refactoring scan_and_compact():
>>> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-scan_amd_compact-refactor/
>>>
>>>
>>> Thanks,
>>> Bengt
>>>
>>
>