RFR(M/L): 6484982: G1: process references during evacuation pauses
Bengt Rutisson
bengt.rutisson at oracle.com
Mon Sep 12 13:22:05 UTC 2011
Hi John,
Stefan and I have been looking at this webrev today. Overall it looks
good! This will be a great improvement for G1.
We have not quite gotten through all the code yet, but I hope that we've
covered most of the important changes.
Here are some comments:
referenceProcessor.cpp
- Not strictly related to your change, but it would be nice to change
the name of _discoveredSoftRefs to _discoveredRefs and introduce a
separate _discoveredSoftRefs. It is quite confusing that
_discoveredSoftRefs is being used for all reference types.
- Why does iter.make_active() need the pre-barrier? We do
iter.make_referent_alive() just after. That will mark and push the
referent object, right?
- ReferenceProcessor::balance_queues() got convoluted by the extra
levels of if statements. A suggestion is to make a set_discovered
method like this:
set_discovered(oop ref, oop value) {
if (_discovered_list_needs_barrier) {
java_lang_ref_Reference::set_discovered(ref, value);
} else {
java_lang_ref_Reference::set_discovered_raw(ref, value);
}
}
and then us it as:
if (ref_lists[to_idx].head() == NULL) {
// to list is empty. Make a loop at the end.
set_discovered(move_tail, move_tail);
} else {
set_discovered(move_tail, ref_lists[to_idx].head());
}
g1CollectedHeap.cpp
- What does this comment mean?
instanceRefKlass::process_discovered_references does not exist.
// We want to discover references, but not process them yet.
// This mode is disabled in
// instanceRefKlass::process_discovered_references if the
// generation does some collection work, or
// instanceRefKlass::enqueue_discovered_references if the
// generation returns without doing any work.
- The setup of the STW reference processor uses an is_alive_closure.
This seems to be correct and efficient, but why do the other full and
STW reference processors in HotSpot skip this?
- Why is it necessary to go through the references found during CM in
G1CollectedHeap::g1_process_strong_roots?
if
(!_process_strong_tasks->is_task_claimed(G1H_PS_refProcessor_oops_do)) {
// We need to treat the discovered reference lists of the
// concurrent mark ref processor as roots and keep entries
// (which are added by the marking threads) on them live
// until they can be processed at the end of marking.
ref_processor_cm()->weak_oops_do(&buf_scan_non_heap_roots);
}
- The G1_DEBUG define makes the code really difficult to read. Could
this be removed?
- Have not looked in detail at the large new block of code in
g1CollectedHeap.cpp. One question though, would it be possible to
somehow simplify the setup in G1ParTask, G1STWRefProcTaskProxy,
G1ParPreserveCMReferentsTask and process_discovered_references()?
- Did you do any tests with -XX:RefDiscoveryPolicy=1?
Some minor things:
referenceProcessor.hpp
- Would be nice if subclasses_of_ref() could be called something with
"number_of" or similar. That indicates better how it is being used.
thread.cpp
- only bracket changes. Don't include in thins change.
referenceProcessor.cpp
- The code that is commented out in ReferenceProcessor::weak_oops_do()
and ReferenceProcessor::clean_up_discovered_references() could probably
be removed by now.
- You removed inline from void DiscoveredListIterator::remove(), will
that impact performance?
g1CollectedHeap.cpp
- What does "ser" stand for in "stw_rp_disc_ser"?
- Would be nice to move these two lines into enable_discovery(). (May
need a parameter to disable the check for NoRefDiscovery to work properly.)
assert(!ref_processor_stw()->discovery_enabled(), "Precondition");
ref_processor_stw()->verify_no_references_recorded();
- Are these asserts really necessary?
void G1ParScanThreadState::trim_queue() {
assert(_evac_cl != NULL, "not set");
assert(_evac_failure_cl != NULL, "not set");
assert(_partial_scan_cl != NULL, "not set");
- Maybe G1KeepAliveClosure should be called something else. It does not
keep objects alive, it just adjusts the pointer to the forwarded location.
g1CollectedHeap.hpp:
- This comment is a bit confusing:
// Will enqueue any non-live discovered references on the
// STW ref processor's discovered lists.
Maybe "Will enqueue any non-live discovered reference that the STW ref
processor discovered.
heapRegion.hpp
- I guess #if WHASSUP should be removed...
Stefan & Bengt
On 2011-09-09 23: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