Wed, 11 Jun 2014 10:46:47 +0200
8043239: G1: Missing post barrier in processing of j.l.ref.Reference objects
Summary: Removed all write barriers during reference processing and added explicit write barriers when iterating through the discovered list.
Reviewed-by: pliden, jmasa, tschatzl
1.1 --- a/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Tue Feb 11 13:29:53 2014 +0100 1.2 +++ b/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Wed Jun 11 10:46:47 2014 +0200 1.3 @@ -310,8 +310,7 @@ 1.4 _cmsGen->refs_discovery_is_mt(), // mt discovery 1.5 (int) MAX2(ConcGCThreads, ParallelGCThreads), // mt discovery degree 1.6 _cmsGen->refs_discovery_is_atomic(), // discovery is not atomic 1.7 - &_is_alive_closure, // closure for liveness info 1.8 - false); // next field updates do not need write barrier 1.9 + &_is_alive_closure); // closure for liveness info 1.10 // Initialize the _ref_processor field of CMSGen 1.11 _cmsGen->set_ref_processor(_ref_processor); 1.12
2.1 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Tue Feb 11 13:29:53 2014 +0100 2.2 +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Wed Jun 11 10:46:47 2014 +0200 2.3 @@ -2258,12 +2258,9 @@ 2.4 // degree of mt discovery 2.5 false, 2.6 // Reference discovery is not atomic 2.7 - &_is_alive_closure_cm, 2.8 + &_is_alive_closure_cm); 2.9 // is alive closure 2.10 // (for efficiency/performance) 2.11 - true); 2.12 - // Setting next fields of discovered 2.13 - // lists requires a barrier. 2.14 2.15 // STW ref processor 2.16 _ref_processor_stw = 2.17 @@ -2278,12 +2275,9 @@ 2.18 // degree of mt discovery 2.19 true, 2.20 // Reference discovery is atomic 2.21 - &_is_alive_closure_stw, 2.22 + &_is_alive_closure_stw); 2.23 // is alive closure 2.24 // (for efficiency/performance) 2.25 - false); 2.26 - // Setting next fields of discovered 2.27 - // lists does not require a barrier. 2.28 } 2.29 2.30 size_t G1CollectedHeap::capacity() const {
3.1 --- a/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp Tue Feb 11 13:29:53 2014 +0100 3.2 +++ b/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp Wed Jun 11 10:46:47 2014 +0200 3.3 @@ -1638,8 +1638,7 @@ 3.4 refs_discovery_is_mt(), // mt discovery 3.5 (int) ParallelGCThreads, // mt discovery degree 3.6 refs_discovery_is_atomic(), // atomic_discovery 3.7 - NULL, // is_alive_non_header 3.8 - false); // write barrier for next field updates 3.9 + NULL); // is_alive_non_header 3.10 } 3.11 } 3.12
4.1 --- a/src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.cpp Tue Feb 11 13:29:53 2014 +0100 4.2 +++ b/src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.cpp Wed Jun 11 10:46:47 2014 +0200 4.3 @@ -853,8 +853,7 @@ 4.4 true, // mt discovery 4.5 (int) ParallelGCThreads, // mt discovery degree 4.6 true, // atomic_discovery 4.7 - &_is_alive_closure, // non-header is alive closure 4.8 - false); // write barrier for next field updates 4.9 + &_is_alive_closure); // non-header is alive closure 4.10 _counters = new CollectorCounters("PSParallelCompact", 1); 4.11 4.12 // Initialize static fields in ParCompactionManager.
5.1 --- a/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp Tue Feb 11 13:29:53 2014 +0100 5.2 +++ b/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp Wed Jun 11 10:46:47 2014 +0200 5.3 @@ -861,8 +861,7 @@ 5.4 true, // mt discovery 5.5 (int) ParallelGCThreads, // mt discovery degree 5.6 true, // atomic_discovery 5.7 - NULL, // header provides liveness info 5.8 - false); // next field updates do not need write barrier 5.9 + NULL); // header provides liveness info 5.10 5.11 // Cache the cardtable 5.12 BarrierSet* bs = Universe::heap()->barrier_set();
6.1 --- a/src/share/vm/memory/referenceProcessor.cpp Tue Feb 11 13:29:53 2014 +0100 6.2 +++ b/src/share/vm/memory/referenceProcessor.cpp Wed Jun 11 10:46:47 2014 +0200 6.3 @@ -96,12 +96,10 @@ 6.4 bool mt_discovery, 6.5 uint mt_discovery_degree, 6.6 bool atomic_discovery, 6.7 - BoolObjectClosure* is_alive_non_header, 6.8 - bool discovered_list_needs_post_barrier) : 6.9 + BoolObjectClosure* is_alive_non_header) : 6.10 _discovering_refs(false), 6.11 _enqueuing_is_done(false), 6.12 _is_alive_non_header(is_alive_non_header), 6.13 - _discovered_list_needs_post_barrier(discovered_list_needs_post_barrier), 6.14 _processing_is_mt(mt_processing), 6.15 _next_id(0) 6.16 { 6.17 @@ -340,10 +338,18 @@ 6.18 // (java.lang.ref.Reference.discovered), self-loop their "next" field 6.19 // thus distinguishing them from active References, then 6.20 // prepend them to the pending list. 6.21 + // 6.22 + // The Java threads will see the Reference objects linked together through 6.23 + // the discovered field. Instead of trying to do the write barrier updates 6.24 + // in all places in the reference processor where we manipulate the discovered 6.25 + // field we make sure to do the barrier here where we anyway iterate through 6.26 + // all linked Reference objects. Note that it is important to not dirty any 6.27 + // cards during reference processing since this will cause card table 6.28 + // verification to fail for G1. 6.29 + // 6.30 // BKWRD COMPATIBILITY NOTE: For older JDKs (prior to the fix for 4956777), 6.31 // the "next" field is used to chain the pending list, not the discovered 6.32 // field. 6.33 - 6.34 if (TraceReferenceGC && PrintGCDetails) { 6.35 gclog_or_tty->print_cr("ReferenceProcessor::enqueue_discovered_reflist list " 6.36 INTPTR_FORMAT, (address)refs_list.head()); 6.37 @@ -351,7 +357,7 @@ 6.38 6.39 oop obj = NULL; 6.40 oop next_d = refs_list.head(); 6.41 - if (pending_list_uses_discovered_field()) { // New behaviour 6.42 + if (pending_list_uses_discovered_field()) { // New behavior 6.43 // Walk down the list, self-looping the next field 6.44 // so that the References are not considered active. 6.45 while (obj != next_d) { 6.46 @@ -365,15 +371,15 @@ 6.47 assert(java_lang_ref_Reference::next(obj) == NULL, 6.48 "Reference not active; should not be discovered"); 6.49 // Self-loop next, so as to make Ref not active. 6.50 - // Post-barrier not needed when looping to self. 6.51 java_lang_ref_Reference::set_next_raw(obj, obj); 6.52 - if (next_d == obj) { // obj is last 6.53 - // Swap refs_list into pendling_list_addr and 6.54 + if (next_d != obj) { 6.55 + oopDesc::bs()->write_ref_field(java_lang_ref_Reference::discovered_addr(obj), next_d); 6.56 + } else { 6.57 + // This is the last object. 6.58 + // Swap refs_list into pending_list_addr and 6.59 // set obj's discovered to what we read from pending_list_addr. 6.60 oop old = oopDesc::atomic_exchange_oop(refs_list.head(), pending_list_addr); 6.61 - // Need post-barrier on pending_list_addr above; 6.62 - // see special post-barrier code at the end of 6.63 - // enqueue_discovered_reflists() further below. 6.64 + // Need post-barrier on pending_list_addr. See enqueue_discovered_ref_helper() above. 6.65 java_lang_ref_Reference::set_discovered_raw(obj, old); // old may be NULL 6.66 oopDesc::bs()->write_ref_field(java_lang_ref_Reference::discovered_addr(obj), old); 6.67 } 6.68 @@ -496,20 +502,15 @@ 6.69 // pre-barrier here because we know the Reference has already been found/marked, 6.70 // that's how it ended up in the discovered list in the first place. 6.71 oop_store_raw(_prev_next, new_next); 6.72 - if (_discovered_list_needs_post_barrier && _prev_next != _refs_list.adr_head()) { 6.73 - // Needs post-barrier and this is not the list head (which is not on the heap) 6.74 - oopDesc::bs()->write_ref_field(_prev_next, new_next); 6.75 - } 6.76 NOT_PRODUCT(_removed++); 6.77 _refs_list.dec_length(1); 6.78 } 6.79 6.80 // Make the Reference object active again. 6.81 void DiscoveredListIterator::make_active() { 6.82 - // For G1 we don't want to use set_next - it 6.83 - // will dirty the card for the next field of 6.84 - // the reference object and will fail 6.85 - // CT verification. 6.86 + // The pre barrier for G1 is probably just needed for the old 6.87 + // reference processing behavior. Should we guard this with 6.88 + // ReferenceProcessor::pending_list_uses_discovered_field() ? 6.89 if (UseG1GC) { 6.90 HeapWord* next_addr = java_lang_ref_Reference::next_addr(_ref); 6.91 if (UseCompressedOops) { 6.92 @@ -517,10 +518,8 @@ 6.93 } else { 6.94 oopDesc::bs()->write_ref_field_pre((oop*)next_addr, NULL); 6.95 } 6.96 - java_lang_ref_Reference::set_next_raw(_ref, NULL); 6.97 - } else { 6.98 - java_lang_ref_Reference::set_next(_ref, NULL); 6.99 } 6.100 + java_lang_ref_Reference::set_next_raw(_ref, NULL); 6.101 } 6.102 6.103 void DiscoveredListIterator::clear_referent() { 6.104 @@ -546,7 +545,7 @@ 6.105 OopClosure* keep_alive, 6.106 VoidClosure* complete_gc) { 6.107 assert(policy != NULL, "Must have a non-NULL policy"); 6.108 - DiscoveredListIterator iter(refs_list, keep_alive, is_alive, _discovered_list_needs_post_barrier); 6.109 + DiscoveredListIterator iter(refs_list, keep_alive, is_alive); 6.110 // Decide which softly reachable refs should be kept alive. 6.111 while (iter.has_next()) { 6.112 iter.load_ptrs(DEBUG_ONLY(!discovery_is_atomic() /* allow_null_referent */)); 6.113 @@ -586,7 +585,7 @@ 6.114 BoolObjectClosure* is_alive, 6.115 OopClosure* keep_alive) { 6.116 assert(discovery_is_atomic(), "Error"); 6.117 - DiscoveredListIterator iter(refs_list, keep_alive, is_alive, _discovered_list_needs_post_barrier); 6.118 + DiscoveredListIterator iter(refs_list, keep_alive, is_alive); 6.119 while (iter.has_next()) { 6.120 iter.load_ptrs(DEBUG_ONLY(false /* allow_null_referent */)); 6.121 DEBUG_ONLY(oop next = java_lang_ref_Reference::next(iter.obj());) 6.122 @@ -623,7 +622,7 @@ 6.123 OopClosure* keep_alive, 6.124 VoidClosure* complete_gc) { 6.125 assert(!discovery_is_atomic(), "Error"); 6.126 - DiscoveredListIterator iter(refs_list, keep_alive, is_alive, _discovered_list_needs_post_barrier); 6.127 + DiscoveredListIterator iter(refs_list, keep_alive, is_alive); 6.128 while (iter.has_next()) { 6.129 iter.load_ptrs(DEBUG_ONLY(true /* allow_null_referent */)); 6.130 HeapWord* next_addr = java_lang_ref_Reference::next_addr(iter.obj()); 6.131 @@ -666,7 +665,7 @@ 6.132 OopClosure* keep_alive, 6.133 VoidClosure* complete_gc) { 6.134 ResourceMark rm; 6.135 - DiscoveredListIterator iter(refs_list, keep_alive, is_alive, _discovered_list_needs_post_barrier); 6.136 + DiscoveredListIterator iter(refs_list, keep_alive, is_alive); 6.137 while (iter.has_next()) { 6.138 iter.update_discovered(); 6.139 iter.load_ptrs(DEBUG_ONLY(false /* allow_null_referent */)); 6.140 @@ -782,13 +781,6 @@ 6.141 bool _clear_referent; 6.142 }; 6.143 6.144 -void ReferenceProcessor::set_discovered(oop ref, oop value) { 6.145 - java_lang_ref_Reference::set_discovered_raw(ref, value); 6.146 - if (_discovered_list_needs_post_barrier) { 6.147 - oopDesc::bs()->write_ref_field(java_lang_ref_Reference::discovered_addr(ref), value); 6.148 - } 6.149 -} 6.150 - 6.151 // Balances reference queues. 6.152 // Move entries from all queues[0, 1, ..., _max_num_q-1] to 6.153 // queues[0, 1, ..., _num_q-1] because only the first _num_q 6.154 @@ -846,9 +838,9 @@ 6.155 // Add the chain to the to list. 6.156 if (ref_lists[to_idx].head() == NULL) { 6.157 // to list is empty. Make a loop at the end. 6.158 - set_discovered(move_tail, move_tail); 6.159 + java_lang_ref_Reference::set_discovered_raw(move_tail, move_tail); 6.160 } else { 6.161 - set_discovered(move_tail, ref_lists[to_idx].head()); 6.162 + java_lang_ref_Reference::set_discovered_raw(move_tail, ref_lists[to_idx].head()); 6.163 } 6.164 ref_lists[to_idx].set_head(move_head); 6.165 ref_lists[to_idx].inc_length(refs_to_move); 6.166 @@ -982,7 +974,7 @@ 6.167 6.168 void ReferenceProcessor::clean_up_discovered_reflist(DiscoveredList& refs_list) { 6.169 assert(!discovery_is_atomic(), "Else why call this method?"); 6.170 - DiscoveredListIterator iter(refs_list, NULL, NULL, _discovered_list_needs_post_barrier); 6.171 + DiscoveredListIterator iter(refs_list, NULL, NULL); 6.172 while (iter.has_next()) { 6.173 iter.load_ptrs(DEBUG_ONLY(true /* allow_null_referent */)); 6.174 oop next = java_lang_ref_Reference::next(iter.obj()); 6.175 @@ -1071,16 +1063,6 @@ 6.176 // The last ref must have its discovered field pointing to itself. 6.177 oop next_discovered = (current_head != NULL) ? current_head : obj; 6.178 6.179 - // Note: In the case of G1, this specific pre-barrier is strictly 6.180 - // not necessary because the only case we are interested in 6.181 - // here is when *discovered_addr is NULL (see the CAS further below), 6.182 - // so this will expand to nothing. As a result, we have manually 6.183 - // elided this out for G1, but left in the test for some future 6.184 - // collector that might have need for a pre-barrier here, e.g.:- 6.185 - // oopDesc::bs()->write_ref_field_pre((oop* or narrowOop*)discovered_addr, next_discovered); 6.186 - assert(!_discovered_list_needs_post_barrier || UseG1GC, 6.187 - "Need to check non-G1 collector: " 6.188 - "may need a pre-write-barrier for CAS from NULL below"); 6.189 oop retest = oopDesc::atomic_compare_exchange_oop(next_discovered, discovered_addr, 6.190 NULL); 6.191 if (retest == NULL) { 6.192 @@ -1089,9 +1071,6 @@ 6.193 // is necessary. 6.194 refs_list.set_head(obj); 6.195 refs_list.inc_length(1); 6.196 - if (_discovered_list_needs_post_barrier) { 6.197 - oopDesc::bs()->write_ref_field((void*)discovered_addr, next_discovered); 6.198 - } 6.199 6.200 if (TraceReferenceGC) { 6.201 gclog_or_tty->print_cr("Discovered reference (mt) (" INTPTR_FORMAT ": %s)", 6.202 @@ -1242,24 +1221,14 @@ 6.203 if (_discovery_is_mt) { 6.204 add_to_discovered_list_mt(*list, obj, discovered_addr); 6.205 } else { 6.206 - // If "_discovered_list_needs_post_barrier", we do write barriers when 6.207 - // updating the discovered reference list. Otherwise, we do a raw store 6.208 - // here: the field will be visited later when processing the discovered 6.209 - // references. 6.210 + // We do a raw store here: the field will be visited later when processing 6.211 + // the discovered references. 6.212 oop current_head = list->head(); 6.213 // The last ref must have its discovered field pointing to itself. 6.214 oop next_discovered = (current_head != NULL) ? current_head : obj; 6.215 6.216 - // As in the case further above, since we are over-writing a NULL 6.217 - // pre-value, we can safely elide the pre-barrier here for the case of G1. 6.218 - // e.g.:- oopDesc::bs()->write_ref_field_pre((oop* or narrowOop*)discovered_addr, next_discovered); 6.219 assert(discovered == NULL, "control point invariant"); 6.220 - assert(!_discovered_list_needs_post_barrier || UseG1GC, 6.221 - "For non-G1 collector, may need a pre-write-barrier for CAS from NULL below"); 6.222 oop_store_raw(discovered_addr, next_discovered); 6.223 - if (_discovered_list_needs_post_barrier) { 6.224 - oopDesc::bs()->write_ref_field((void*)discovered_addr, next_discovered); 6.225 - } 6.226 list->set_head(obj); 6.227 list->inc_length(1); 6.228 6.229 @@ -1353,7 +1322,7 @@ 6.230 OopClosure* keep_alive, 6.231 VoidClosure* complete_gc, 6.232 YieldClosure* yield) { 6.233 - DiscoveredListIterator iter(refs_list, keep_alive, is_alive, _discovered_list_needs_post_barrier); 6.234 + DiscoveredListIterator iter(refs_list, keep_alive, is_alive); 6.235 while (iter.has_next()) { 6.236 iter.load_ptrs(DEBUG_ONLY(true /* allow_null_referent */)); 6.237 oop obj = iter.obj();
7.1 --- a/src/share/vm/memory/referenceProcessor.hpp Tue Feb 11 13:29:53 2014 +0100 7.2 +++ b/src/share/vm/memory/referenceProcessor.hpp Wed Jun 11 10:46:47 2014 +0200 7.3 @@ -99,7 +99,6 @@ 7.4 oop _referent; 7.5 OopClosure* _keep_alive; 7.6 BoolObjectClosure* _is_alive; 7.7 - bool _discovered_list_needs_post_barrier; 7.8 7.9 DEBUG_ONLY( 7.10 oop _first_seen; // cyclic linked list check 7.11 @@ -113,8 +112,7 @@ 7.12 public: 7.13 inline DiscoveredListIterator(DiscoveredList& refs_list, 7.14 OopClosure* keep_alive, 7.15 - BoolObjectClosure* is_alive, 7.16 - bool discovered_list_needs_post_barrier = false): 7.17 + BoolObjectClosure* is_alive): 7.18 _refs_list(refs_list), 7.19 _prev_next(refs_list.adr_head()), 7.20 _prev(NULL), 7.21 @@ -128,8 +126,7 @@ 7.22 #endif 7.23 _next(NULL), 7.24 _keep_alive(keep_alive), 7.25 - _is_alive(is_alive), 7.26 - _discovered_list_needs_post_barrier(discovered_list_needs_post_barrier) 7.27 + _is_alive(is_alive) 7.28 { } 7.29 7.30 // End Of List. 7.31 @@ -230,14 +227,6 @@ 7.32 // other collectors in configuration 7.33 bool _discovery_is_mt; // true if reference discovery is MT. 7.34 7.35 - // If true, setting "next" field of a discovered refs list requires 7.36 - // write post barrier. (Must be true if used in a collector in which 7.37 - // elements of a discovered list may be moved during discovery: for 7.38 - // example, a collector like Garbage-First that moves objects during a 7.39 - // long-term concurrent marking phase that does weak reference 7.40 - // discovery.) 7.41 - bool _discovered_list_needs_post_barrier; 7.42 - 7.43 bool _enqueuing_is_done; // true if all weak references enqueued 7.44 bool _processing_is_mt; // true during phases when 7.45 // reference processing is MT. 7.46 @@ -382,11 +371,6 @@ 7.47 void enqueue_discovered_reflists(HeapWord* pending_list_addr, AbstractRefProcTaskExecutor* task_executor); 7.48 7.49 protected: 7.50 - // Set the 'discovered' field of the given reference to 7.51 - // the given value - emitting post barriers depending upon 7.52 - // the value of _discovered_list_needs_post_barrier. 7.53 - void set_discovered(oop ref, oop value); 7.54 - 7.55 // "Preclean" the given discovered reference list 7.56 // by removing references with strongly reachable referents. 7.57 // Currently used in support of CMS only. 7.58 @@ -427,8 +411,7 @@ 7.59 bool mt_processing = false, uint mt_processing_degree = 1, 7.60 bool mt_discovery = false, uint mt_discovery_degree = 1, 7.61 bool atomic_discovery = true, 7.62 - BoolObjectClosure* is_alive_non_header = NULL, 7.63 - bool discovered_list_needs_post_barrier = false); 7.64 + BoolObjectClosure* is_alive_non_header = NULL); 7.65 7.66 // RefDiscoveryPolicy values 7.67 enum DiscoveryPolicy {