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

Thu Apr 14 14:36:31 UTC 2016

On 04/14/2016 01:06 AM, 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/
>
> Diff compared to last version:
> http://cr.openjdk.java.net/~brutisso/8073321/webrev.00-01.diff/

Looks good.  Thanks.

>
>>
>> =============  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.

Yes, you're right.

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

I have no problem with leaving until later.

>
>>
>> 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 see you've considered it.  Leave as is.

Looks good.

Jon
>
> (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
>>>
>>
>