RFR: 8031703 - Missing post-barrier in ReferenceProcessor

[Tony Printezis]

Per,

On 2/4/14, 5:15 AM, Per Liden wrote:
>>
>> 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?

I meant the one in G1 where the "needs post barrier" flag is set to true 
(apologies for the confusion). But, putting it in the remove() function 
is fine.

>> 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".

Thanks and looks good!

Tony

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

-- 
Tony Printezis | JVM/GC Engineer / VM Team | Twitter

@TonyPrintezis
tprintezis at twitter.com