RFR(M/L): 6484982: G1: process references during evacuation pauses

[Stefan Karlsson]

On 09/15/2011 01:16 PM, Bengt Rutisson wrote:
>
> John,
>
> Looks good! Thanks for fixing all of this!
>
> One nit pick about "* Clarified the comment in 
> ReferenceProcessor::weak_oops_do etc to say that the code in the 
> comment is an alternative implmentation of the loop. ":
>
> The comment in referenceProcessor.cpp, line 115, has a "_" at the end. 
> You did not introduce it, but since you are updating the comment maybe 
> you can remove it.
>
> Also, a question regarding this. You have the same comment in 
> ReferenceProcessor::weak_oops_do() and 
> ReferenceProcessor::clean_up_discovered_references(). Both do "for 
> (int i = 0; i < _max_num_q * number_of_subclasses_of_ref(); i++)" so I 
> understand that.
>
> But there are 5 more places in referenceProcessor.cpp that do "for 
> (int i = 0; i < _max_num_q * number_of_subclasses_of_ref(); i++)". 
> Should they also have the same comment? Your call. I would prefer to 
> have it in all places or in none of them.

IIRC, one of the places were we do this iteratoin is in the setup of the 
discovered lists. Maybe move the comment to that place, if you want to 
keep it?

StefanK

>
> Thanks,
> Bengt
>
>
> On 2011-09-15 00:24, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> A new webrev that incorporates (most of) the suggestions made by 
>> everyone can be found at: 
>> http://cr.openjdk.java.net/~johnc/6484982/webrev.4/
>>
>> Summary:
>>  * Added explanation about the _is_alive_non_header value being 
>> optional.
>>  * removed the G1_DEBUG code
>>  * Clarified the comment in ReferenceProcessor::weak_oops_do etc to 
>> say that the code in the comment is an alternative implmentation of 
>> the loop.
>>  * Moved the assert that _discovering_refs is false and that there 
>> are no refs discovered into ReferenceProcessor::enable_discovery. 
>> Bengt/Stefan  were correct in that I needed to flags to 
>> enable_discovery which had a knock on effect.
>>  * Added a helper routine to set the discovered field for use in 
>> ReferenceProcessor::balance_queues(). Note that in the other calls to 
>> set_discovered we either really need the barrier or we don't care 
>> except when enqueuing on to a discovered list or balancing the 
>> discovered lists, where we explicitly do not want a barrier.
>>
>> I'll be submitting RFEs for the other valid suggestions that were made.
>>
>> Sanity Testing: The entire GC test suite with G1 with a low marking 
>> threshold; for the other collectors (serial, parallel, par old, CMS 
>> w/ ParNew, CMS w/o ParNew) I ran the GC test suite version of specjvm98.
>>
>> Thanks,
>>
>> JohnC
>>
>> On 09/09/11 14:19, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> I have sync'ed these changes up to and including the changeset 
>>> 2660:3bddbf0f57d6 which was for "7087717: G1: make the 
>>> G1PrintRegionLivenessInfo parameter diagnostic" including merging 
>>> with Stefan's and Ramki's reference processor changes. As part of 
>>> merging I changed to code that preserves objects that referenced 
>>> from the discovered lists of the concurrent mark's reference 
>>> processor to use the DiscoveredListIterator class. This necessitated 
>>> moving DiscoveredListIterator and some of its methods 
>>> referenceProcessor.hpp.
>>>
>>> The new webrev can be found at: 
>>> http://cr.openjdk.java.net/~johnc/6484982/webrev.3/
>>>
>>> Testing: the GC test suite with a marking threshold of 20% (with 
>>> heap verification enabled) and KitchenSink.
>>>
>>> JohnC
>>>
>>>
>>> On 08/17/11 11:15, John Cuthbertson wrote:
>>>> Hi Everyone,
>>>>
>>>> A new webrev for these changes can be found at: 
>>>> http://cr.openjdk.java.net/~johnc/6484982/webrev.2/
>>>>
>>>> The changes in this webrev reverse the order of preserving objects 
>>>> referenced from the concurrent mark ref processor's discovered 
>>>> lists and processing references discovered by the STW ref 
>>>> processor. Preserving the objects referenced from the concurrent 
>>>> mark ref processor's discovered lists comes first so that any 
>>>> object that needs to be copied is done so before reference processing.
>>>>
>>>> JohnC
>>>>
>>>> On 08/03/11 10:44, John Cuthbertson wrote:
>>>>> Hi Everyone,
>>>>>
>>>>> A new webrev incorporating some feedback from Ramki can be found 
>>>>> at: http://cr.openjdk.java.net/~johnc/6484982/webrev.1/
>>>>>
>>>>> Thanks,
>>>>>
>>>>> JohnC
>>>>>
>>>>> On 06/23/11 14:35, John Cuthbertson wrote:
>>>>>> Hi Everyone,
>>>>>>
>>>>>> I would like to get a couple of volunteers to review the code 
>>>>>> changes for this CR - the webrev can be found at 
>>>>>> http://cr.openjdk.java.net/~johnc/6484982/webrev.0/
>>>>>>
>>>>>> Summary:
>>>>>> G1 now contains 2 instances of the reference processor class - 
>>>>>> one for concurrent marking and the other for STW GCs (both full 
>>>>>> and incremental evacuation pauses). For evacuation pauses, during 
>>>>>> object scanning and RSet scanning I embed the STW reference 
>>>>>> processor into the OopClosures used to scan objects. This causes 
>>>>>> reference objects to be 'discovered' by the reference processor. 
>>>>>> Towards the end of the evacuation pause (just prior to retiring 
>>>>>> the the GC alloc regions) I have added the code to process these 
>>>>>> discovered reference objects, preserving (and copying) referent 
>>>>>> objects (and their reachable graphs) as appropriate. The code 
>>>>>> that does this makes extensive use of the existing copying oop 
>>>>>> closures and the G1ParScanThreadState structure (to handle 
>>>>>> to-space allocation).
>>>>>>
>>>>>> The code changes also include a couple of fixes that were exposed 
>>>>>> by the reference processing:
>>>>>>  * In satbQueue.cpp, the routine 
>>>>>> SATBMarkQueueSet::par_iterate_closure_all_threads() was claiming 
>>>>>> all JavaThreads (giving them one parity value) but skipping the 
>>>>>> VMThread. In a subsequent call to 
>>>>>> Thread::possibly_parallel_oops_do, the Java threads were 
>>>>>> successfully claimed but the VMThread was not. This could cause 
>>>>>> the VMThread's handle area to be skipped during the root scanning.
>>>>>>  * There were a couple of assignments to the discovered field of 
>>>>>> Reference objects that were not guarded by 
>>>>>> _discovery_needs_barrier resulting in the G1 C++ write-barrier to 
>>>>>> dirty the card spanning the Reference object's discovered field. 
>>>>>> This was causing the card table verification (during card table 
>>>>>> clearing) to fail.
>>>>>>  * There were also a couple of assignments of NULL to the next 
>>>>>> field of Reference objects causing the same symptom.
>>>>>>
>>>>>> Testing: The GC test suite (32/64 bit) (+UseG1GC, +UseG1GC 
>>>>>> +ExplicitGCInvokesConcurrent, +UseG1GC 
>>>>>> InitiatingHeapOccupancyPercent=5, +UseG1GC 
>>>>>> +ParallelRefProcEnabled), KitchenSink (48 hour runs with 
>>>>>> +UseG1GC, +UseG1GC +ExplicitGCInvokesConcurrent), OpenDS 
>>>>>> (+UseG1GC, +UseG1GC +ParallelRefProcEnabled), nsk GC and compiler 
>>>>>> tests, and jprt. Testing was conducted with the 
>>>>>> _is_alive_non_header field in the STW ref procssor both cleared 
>>>>>> and set (when cleared, more reference objects are 'discovered').
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> JohnC
>>>>>
>>>>
>>>
>>
>