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

Bengt Rutisson bengt.rutisson at oracle.com
Thu Apr 14 15:10:46 UTC 2016


Hi Jon,

Thanks for the review!

Bengt

On 2016-04-14 16:36, Jon Masamitsu wrote:
>
>
> 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
>>>>
>>>
>>
>



More information about the hotspot-gc-dev mailing list