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

Thu Apr 14 09:19:15 UTC 2016

Hi Mikael,

On 2016-04-14 11:14, Mikael Gerdin wrote:
> 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.

Yes, I agree, but I could not come up with a better solution either.

>
> Reviewed.

Thanks!

Bengt

>
> /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
>>>>
>>>
>>