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

Thu Sep 15 11:16:57 UTC 2011

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.

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