RFR: 8031703 - Missing post-barrier in ReferenceProcessor

[Per Liden]

Hi Tony,

On 2014-02-04 04:20, Tony Printezis wrote:
> Hi Per,
>
> (apologies for not replying earlier; we had a small firedrill at the 
> end of last week)

No problem.

>
> I went over the code one more time and I think your assessment is 
> correct. But, could I recommend that you at least add a comment to 
> that effect where the DiscoveredListIterator iter(...) is declared? 

Hmm, which one? The iterator is instanciated in several places. I'd 
suggest I add a comment like this to the remove() function, where the 
pre-barriers is omitted. Ok?

> Also, maybe, rename the discovered_list_needs_barrier parameter as 
> ..._needs_post_barrier to make its role a bit more explicit?

Good idea, will add "post".

Updated webrev here: http://cr.openjdk.java.net/~pliden/8031703/webrev.1/

/Per

>
> Tony
>
> On 1/30/14, 9:19 AM, Per Liden wrote:
>> Hi Tony,
>>
>> On 01/29/2014 11:06 PM, Tony Printezis wrote:
>>> Per,
>>>
>>> Good catch. Quick question: Is there a reason why pre-barriers are not
>>> needed here?
>>
>> Thanks for looking at it.
>>
>> My reason for not needing a pre-barrier there goes like this:
>>
>> The Reference was inserted into the discovered list (ref_list) 
>> because it was discovered during evacuation or marking and it's thus 
>> strongly reachable from some other place and is marked (or will be 
>> marked) live. If we later remove the Reference from the discovered 
>> list we don't need to enqueue it for SATB because we know it has 
>> already been found/marked, that's how it ended up in the discovered 
>> list in the first place.
>>
>> Makes sense?
>>
>> Also note that if we had a pre-barrier there it would cause
>> verify_no_cset_oops() to fail if we get an young collection during 
>> concurrent mark.
>>
>> cheers,
>> /Per
>>
>>>
>>> Tony
>>>
>>> On 1/28/14, 4:57 AM, Per Liden wrote:
>>>> http://cr.openjdk.java.net/~pliden/8031703/webrev.0/
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8031703
>>>>
>>>> Summary:  In ReferenceProcessor::process_discovered_references() the
>>>> lists of discovered references are pruned (a reference who's referent
>>>> is still alive is unlinked, etc). During this pruning the
>>>> Reference.discovered field is rewritten without a post-barrier (in
>>>> DiscoveredListIterator::remove()), which means we fail to dirty the
>>>> appropriate card. This only affect G1's CM reference processor, which
>>>> is the only case where the reference processor needs post-barriers for
>>>> the discovered list (i.e. _discovered_list_needs_barrier is true). The
>>>> window for this to happen was widened after fixing JDK-8029255. Before
>>>> that fix the appropriate card was usually dirtied by pure luck because
>>>> of a write (in a different context) to the Reference.next field in the
>>>> same object. However, if the object was unlucky and the
>>>> next/discovered fields spanned two different cards the problem would
>>>> have appeared. The patch also corrects the post-barrier in
>>>> ReferenceProcessor::set_discovered(), which used an incorrect field
>>>> offset or 0 instead of discovered_offset.
>>>>
>>>> The problem could be easily reproduced by running Kitchensink on a
>>>> 32-bit x86 JVM with G1. The bug is however not 32-bit related, it just
>>>> happened to expose the problem more frequently.
>>>>
>>>> Testing:
>>>> Kitchensink 32-bit x86 JVM with G1 (6 hours)
>>>> Kitchensink 64-bit x86 JVM with G1 (30 minutes)
>>>> Kitchensink 32-bit x86 JVM with CMS (30 minutes)
>>>>
>>>> /Per
>>>>
>>>
>>
>