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

Thu Sep 15 19:55:43 UTC 2011

John,

On 2011-09-15 18:33, John Cuthbertson wrote:
> Hi Stefan, Bengt,
>
> Stefan - I think that is a great idea but I'm just going to remove it 
> from the 2 routines. I agree with Bengt - it should be either in all 
> or none. And it may need updating again when _discoveredSoftRefs is 
> renamed.

Sounds good to me.

Bengt

>
> Thanks again for looking at the code.
>
> JohnC
>
> On 09/15/11 04:38, Stefan Karlsson wrote:
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>