RFR (M): JDK-8043239: G1: Missing post barrier in processing of j.l.ref.Reference objects

[Bengt Rutisson]

Hi Jon,

Thanks for the review!

On 2014-06-03 00:25, Jon Masamitsu wrote:
> Bengt,
>
> http://cr.openjdk.java.net/~brutisso/8043239/webrev.00/src/share/vm/memory/referenceProcessor.cpp.frames.html 
>
>
> The change to always use "set_next_raw()" here
>
>  520     java_lang_ref_Reference::set_next_raw(_ref, NULL);
>  521   } else {
>  522     java_lang_ref_Reference::set_next(_ref, NULL);
>  523   }
>
> was always the correct thing to use?  Does not have to do with
> extra / unneeded write barrier?

Right. Since we are writing NULL we don't need a post barrier and for G1 
it is important to avoid the post barrier because it will dirty cards in 
a way that will make the card table verification to fail.

>
> Looks good.
>
> Reviewed.

Thanks!

I will push this change, but Thomas suggested to move one of the 
comments that I added to be more visible.

Here's what he suggested:
http://cr.openjdk.java.net/~brutisso/8043239/webrev.00-01.diff/

Since it is only a comment change I will go ahead and push this. Would 
be nice to get nightly testing as soon as possible to be able to 
backport to 8u20. I hope that is ok with you.

Here is the full webrev of what I will push:

http://cr.openjdk.java.net/~brutisso/8043239/webrev.01/

Thanks,
Bengt

>
> Jon
>
> On 06/02/2014 07:30 AM, Bengt Rutisson wrote:
>>
>> Hi all,
>>
>> Can I have a couple of reviews for this change?
>>
>> http://cr.openjdk.java.net/~brutisso/8043239/webrev.00/
>>
>> https://bugs.openjdk.java.net/browse/JDK-8043239
>>
>> As described in the bug report the reference processor was missing a 
>> write barrier call when manipulating the discovered list. This has 
>> always been the case but it was hidden because at the end of the 
>> reference processing we went through the complete discovered list and 
>> dirtied all the missed cards because we did an (unnecessary) write 
>> barrier when we set the next field to point to be a self pointer 
>> pointing back at the reference object itself.
>>
>> The write barrier for setting the next field was removed since it was 
>> not needed, but that revealed the current bug. After some discussions 
>> and prototyping we came to the conclusion that there may be more 
>> barriers missing and that it is difficult to get the dirtying done 
>> the way our verification code assumes. A simpler solution seems to be 
>> to free the reference processing of all barriers and instead just 
>> make sure that we dirty all the right cards in the last pass.
>>
>> The proposed fix thus re-introduces the post barrier when we iterate 
>> over the discovered list. This time it uses the discovered field for 
>> the barrier to be more explicit about what is going on.
>>
>> Testing:
>> JPRT,
>> Kitchensink, 5 days
>> GC test suite
>> SPECjbb2013
>> Ad-hoc aurora run
>> Specific reproducer that illustrated the problem.
>>
>> The specific reproducer was really good to pinpoint the problem but 
>> is hard to turn in to a JTreg test. Many thanks go to StefanK for 
>> helping out with creating the reproducer.
>>
>> Thanks,
>> Bengt
>