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

[Bengt Rutisson]

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/

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