RFR(M/L): 6484982: G1: process references during evacuation pauses
John Cuthbertson
john.cuthbertson at oracle.com
Thu Sep 15 16:33:03 UTC 2011
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.
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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list