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

[Jon Masamitsu]

Bengt,

Looks very good.  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."

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


Maybe add a next_live()/set_next_live() to CompactibleSpace to get/set the
pointer.

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.

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
>