Mercurial > hg > openjdk > hsx14
changeset 452:00b023ae2d78
6722113: CMS: Incorrect overflow handling during precleaning of Reference lists
Summary: When we encounter marking stack overflow during precleaning of Reference lists, we were using the overflow list mechanism, which can cause problems on account of mutating the mark word of the header because of conflicts with mutator accesses and updates of that field. Instead we should use the usual mechanism for overflow handling in concurrent phases, namely dirtying of the card on which the overflowed object lies. Since precleaning effectively does a form of discovered list processing, albeit with discovery enabled, we needed to adjust some code to be correct in the face of interleaved processing and discovery.
Reviewed-by: apetrusenko, jcoomes
| author | ysr |
|---|---|
| date | Thu, 20 Nov 2008 12:27:41 -0800 |
| parents | b5e603f2e024 |
| children | c96030fff130 |
| files | src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp src/share/vm/memory/referenceProcessor.cpp src/share/vm/runtime/globals.hpp |
| diffstat | 5 files changed, 95 insertions(+), 48 deletions(-) [+] |
line wrap: on
line diff
--- a/src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp Wed Nov 19 14:20:51 2008 -0800 +++ b/src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp Thu Nov 20 12:27:41 2008 -0800 @@ -325,24 +325,30 @@ // For objects in CMS generation, this closure marks // given objects (transitively) as being reachable/live. // This is currently used during the (weak) reference object -// processing phase of the CMS final checkpoint step. +// processing phase of the CMS final checkpoint step, as +// well as during the concurrent precleaning of the discovered +// reference lists. class CMSKeepAliveClosure: public OopClosure { private: CMSCollector* _collector; const MemRegion _span; CMSMarkStack* _mark_stack; CMSBitMap* _bit_map; + bool _concurrent_precleaning; protected: DO_OOP_WORK_DEFN public: CMSKeepAliveClosure(CMSCollector* collector, MemRegion span, - CMSBitMap* bit_map, CMSMarkStack* mark_stack): + CMSBitMap* bit_map, CMSMarkStack* mark_stack, + bool cpc): _collector(collector), _span(span), _bit_map(bit_map), - _mark_stack(mark_stack) { + _mark_stack(mark_stack), + _concurrent_precleaning(cpc) { assert(!_span.is_empty(), "Empty span could spell trouble"); } + bool concurrent_precleaning() const { return _concurrent_precleaning; } virtual void do_oop(oop* p); virtual void do_oop(narrowOop* p); inline void do_oop_nv(oop* p) { CMSKeepAliveClosure::do_oop_work(p); }
--- a/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Wed Nov 19 14:20:51 2008 -0800 +++ b/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Thu Nov 20 12:27:41 2008 -0800 @@ -538,6 +538,7 @@ _survivor_chunk_capacity(0), // -- ditto -- _survivor_chunk_index(0), // -- ditto -- _ser_pmc_preclean_ovflw(0), + _ser_kac_preclean_ovflw(0), _ser_pmc_remark_ovflw(0), _par_pmc_remark_ovflw(0), _ser_kac_ovflw(0), @@ -4388,10 +4389,10 @@ CMSPrecleanRefsYieldClosure yield_cl(this); assert(rp->span().equals(_span), "Spans should be equal"); CMSKeepAliveClosure keep_alive(this, _span, &_markBitMap, - &_markStack); + &_markStack, true /* preclean */); CMSDrainMarkingStackClosure complete_trace(this, - _span, &_markBitMap, &_markStack, - &keep_alive); + _span, &_markBitMap, &_markStack, + &keep_alive, true /* preclean */); // We don't want this step to interfere with a young // collection because we don't want to take CPU @@ -4852,17 +4853,19 @@ // recurrence of that condition. assert(_markStack.isEmpty(), "No grey objects"); size_t ser_ovflw = _ser_pmc_remark_ovflw + _ser_pmc_preclean_ovflw + - _ser_kac_ovflw; + _ser_kac_ovflw + _ser_kac_preclean_ovflw; if (ser_ovflw > 0) { if (PrintCMSStatistics != 0) { gclog_or_tty->print_cr("Marking stack overflow (benign) " - "(pmc_pc="SIZE_FORMAT", pmc_rm="SIZE_FORMAT", kac="SIZE_FORMAT")", + "(pmc_pc="SIZE_FORMAT", pmc_rm="SIZE_FORMAT", kac="SIZE_FORMAT + ", kac_preclean="SIZE_FORMAT")", _ser_pmc_preclean_ovflw, _ser_pmc_remark_ovflw, - _ser_kac_ovflw); + _ser_kac_ovflw, _ser_kac_preclean_ovflw); } _markStack.expand(); _ser_pmc_remark_ovflw = 0; _ser_pmc_preclean_ovflw = 0; + _ser_kac_preclean_ovflw = 0; _ser_kac_ovflw = 0; } if (_par_pmc_remark_ovflw > 0 || _par_kac_ovflw > 0) { @@ -5693,10 +5696,10 @@ ReferenceProcessor* rp = ref_processor(); assert(rp->span().equals(_span), "Spans should be equal"); CMSKeepAliveClosure cmsKeepAliveClosure(this, _span, &_markBitMap, - &_markStack); + &_markStack, false /* !preclean */); CMSDrainMarkingStackClosure cmsDrainMarkingStackClosure(this, _span, &_markBitMap, &_markStack, - &cmsKeepAliveClosure); + &cmsKeepAliveClosure, false /* !preclean */); { TraceTime t("weak refs processing", PrintGCDetails, false, gclog_or_tty); if (rp->processing_is_mt()) { @@ -8302,8 +8305,29 @@ } ) if (simulate_overflow || !_mark_stack->push(obj)) { - _collector->push_on_overflow_list(obj); - _collector->_ser_kac_ovflw++; + if (_concurrent_precleaning) { + // We dirty the overflown object and let the remark + // phase deal with it. + assert(_collector->overflow_list_is_empty(), "Error"); + // In the case of object arrays, we need to dirty all of + // the cards that the object spans. No locking or atomics + // are needed since no one else can be mutating the mod union + // table. + if (obj->is_objArray()) { + size_t sz = obj->size(); + HeapWord* end_card_addr = + (HeapWord*)round_to((intptr_t)(addr+sz), CardTableModRefBS::card_size); + MemRegion redirty_range = MemRegion(addr, end_card_addr); + assert(!redirty_range.is_empty(), "Arithmetical tautology"); + _collector->_modUnionTable.mark_range(redirty_range); + } else { + _collector->_modUnionTable.mark(addr); + } + _collector->_ser_kac_preclean_ovflw++; + } else { + _collector->push_on_overflow_list(obj); + _collector->_ser_kac_ovflw++; + } } } } @@ -8400,6 +8424,8 @@ void CMSDrainMarkingStackClosure::do_void() { // the max number to take from overflow list at a time const size_t num = _mark_stack->capacity()/4; + assert(!_concurrent_precleaning || _collector->overflow_list_is_empty(), + "Overflow list should be NULL during concurrent phases"); while (!_mark_stack->isEmpty() || // if stack is empty, check the overflow list _collector->take_from_overflow_list(num, _mark_stack)) {
--- a/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp Wed Nov 19 14:20:51 2008 -0800 +++ b/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp Thu Nov 20 12:27:41 2008 -0800 @@ -592,6 +592,7 @@ size_t _ser_pmc_preclean_ovflw; size_t _ser_pmc_remark_ovflw; size_t _par_pmc_remark_ovflw; + size_t _ser_kac_preclean_ovflw; size_t _ser_kac_ovflw; size_t _par_kac_ovflw; NOT_PRODUCT(size_t _num_par_pushes;) @@ -1749,21 +1750,30 @@ // work-routine/closure used to complete transitive // marking of objects as live after a certain point // in which an initial set has been completely accumulated. +// This closure is currently used both during the final +// remark stop-world phase, as well as during the concurrent +// precleaning of the discovered reference lists. class CMSDrainMarkingStackClosure: public VoidClosure { CMSCollector* _collector; MemRegion _span; CMSMarkStack* _mark_stack; CMSBitMap* _bit_map; CMSKeepAliveClosure* _keep_alive; + bool _concurrent_precleaning; public: CMSDrainMarkingStackClosure(CMSCollector* collector, MemRegion span, CMSBitMap* bit_map, CMSMarkStack* mark_stack, - CMSKeepAliveClosure* keep_alive): + CMSKeepAliveClosure* keep_alive, + bool cpc): _collector(collector), _span(span), _bit_map(bit_map), _mark_stack(mark_stack), - _keep_alive(keep_alive) { } + _keep_alive(keep_alive), + _concurrent_precleaning(cpc) { + assert(_concurrent_precleaning == _keep_alive->concurrent_precleaning(), + "Mismatch"); + } void do_void(); };
--- a/src/share/vm/memory/referenceProcessor.cpp Wed Nov 19 14:20:51 2008 -0800 +++ b/src/share/vm/memory/referenceProcessor.cpp Thu Nov 20 12:27:41 2008 -0800 @@ -47,7 +47,9 @@ } bool empty() const { return head() == ReferenceProcessor::sentinel_ref(); } size_t length() { return _len; } - void set_length(size_t len) { _len = len; } + void set_length(size_t len) { _len = len; } + void inc_length(size_t inc) { _len += inc; assert(_len > 0, "Error"); } + void dec_length(size_t dec) { _len -= dec; } private: // Set value depending on UseCompressedOops. This could be a template class // but then we have to fix all the instantiations and declarations that use this class. @@ -436,13 +438,13 @@ // The "allow_null_referent" argument tells us to allow for the possibility // of a NULL referent in the discovered Reference object. This typically // happens in the case of concurrent collectors that may have done the - // discovery concurrently or interleaved with mutator execution. + // discovery concurrently, or interleaved, with mutator execution. inline void load_ptrs(DEBUG_ONLY(bool allow_null_referent)); // Move to the next discovered reference. inline void next(); - // Remove the current reference from the list and move to the next. + // Remove the current reference from the list inline void remove(); // Make the Reference object active again. @@ -476,7 +478,6 @@ inline size_t removed() const { return _removed; } ) -private: inline void move_to_next(); private: @@ -553,7 +554,7 @@ oopDesc::store_heap_oop((oop*)_prev_next, _next); } NOT_PRODUCT(_removed++); - move_to_next(); + _refs_list.dec_length(1); } inline void DiscoveredListIterator::move_to_next() { @@ -591,12 +592,13 @@ gclog_or_tty->print_cr("Dropping reference (" INTPTR_FORMAT ": %s" ") by policy", iter.obj(), iter.obj()->blueprint()->internal_name()); } + // Remove Reference object from list + iter.remove(); // Make the Reference object active again iter.make_active(); // keep the referent around iter.make_referent_alive(); - // Remove Reference object from list - iter.remove(); + iter.move_to_next(); } else { iter.next(); } @@ -629,12 +631,13 @@ iter.obj(), iter.obj()->blueprint()->internal_name()); } // The referent is reachable after all. + // Remove Reference object from list. + iter.remove(); // Update the referent pointer as necessary: Note that this // should not entail any recursive marking because the // referent must already have been traversed. iter.make_referent_alive(); - // Remove Reference object from list - iter.remove(); + iter.move_to_next(); } else { iter.next(); } @@ -670,6 +673,7 @@ } else { keep_alive->do_oop((oop*)next_addr); } + iter.move_to_next(); } else { iter.next(); } @@ -832,9 +836,9 @@ } java_lang_ref_Reference::set_discovered(move_tail, ref_lists[to_idx].head()); ref_lists[to_idx].set_head(move_head); - ref_lists[to_idx].set_length(ref_lists[to_idx].length() + refs_to_move); + ref_lists[to_idx].inc_length(refs_to_move); ref_lists[from_idx].set_head(new_head); - ref_lists[from_idx].set_length(ref_lists[from_idx].length() - refs_to_move); + ref_lists[from_idx].dec_length(refs_to_move); } else { ++to_idx; } @@ -923,7 +927,6 @@ void ReferenceProcessor::clean_up_discovered_reflist(DiscoveredList& refs_list) { assert(!discovery_is_atomic(), "Else why call this method?"); DiscoveredListIterator iter(refs_list, NULL, NULL); - size_t length = refs_list.length(); while (iter.has_next()) { iter.load_ptrs(DEBUG_ONLY(true /* allow_null_referent */)); oop next = java_lang_ref_Reference::next(iter.obj()); @@ -941,12 +944,11 @@ ) // Remove Reference object from list iter.remove(); - --length; + iter.move_to_next(); } else { iter.next(); } } - refs_list.set_length(length); NOT_PRODUCT( if (PrintGCDetails && TraceReferenceGC) { gclog_or_tty->print( @@ -1024,7 +1026,7 @@ // We have separate lists for enqueueing so no synchronization // is necessary. refs_list.set_head(obj); - refs_list.set_length(refs_list.length() + 1); + refs_list.inc_length(1); if (_discovered_list_needs_barrier) { _bs->write_ref_field((void*)discovered_addr, current_head); guarantee(false, "Needs to be fixed: YSR"); } @@ -1168,7 +1170,7 @@ _bs->write_ref_field((oop*)discovered_addr, current_head); } list->set_head(obj); - list->set_length(list->length() + 1); + list->inc_length(1); } // In the MT discovery case, it is currently possible to see @@ -1209,45 +1211,48 @@ TraceTime tt("Preclean SoftReferences", PrintGCDetails && PrintReferenceGC, false, gclog_or_tty); for (int i = 0; i < _num_q; i++) { + if (yield->should_return()) { + return; + } preclean_discovered_reflist(_discoveredSoftRefs[i], is_alive, keep_alive, complete_gc, yield); } } - if (yield->should_return()) { - return; - } // Weak references { TraceTime tt("Preclean WeakReferences", PrintGCDetails && PrintReferenceGC, false, gclog_or_tty); for (int i = 0; i < _num_q; i++) { + if (yield->should_return()) { + return; + } preclean_discovered_reflist(_discoveredWeakRefs[i], is_alive, keep_alive, complete_gc, yield); } } - if (yield->should_return()) { - return; - } // Final references { TraceTime tt("Preclean FinalReferences", PrintGCDetails && PrintReferenceGC, false, gclog_or_tty); for (int i = 0; i < _num_q; i++) { + if (yield->should_return()) { + return; + } preclean_discovered_reflist(_discoveredFinalRefs[i], is_alive, keep_alive, complete_gc, yield); } } - if (yield->should_return()) { - return; - } // Phantom references { TraceTime tt("Preclean PhantomReferences", PrintGCDetails && PrintReferenceGC, false, gclog_or_tty); for (int i = 0; i < _num_q; i++) { + if (yield->should_return()) { + return; + } preclean_discovered_reflist(_discoveredPhantomRefs[i], is_alive, keep_alive, complete_gc, yield); } @@ -1256,9 +1261,12 @@ // Walk the given discovered ref list, and remove all reference objects // whose referents are still alive, whose referents are NULL or which -// are not active (have a non-NULL next field). NOTE: For this to work -// correctly, refs discovery can not be happening concurrently with this -// step. +// are not active (have a non-NULL next field). NOTE: When we are +// thus precleaning the ref lists (which happens single-threaded today), +// we do not disable refs discovery to honour the correct semantics of +// java.lang.Reference. As a result, we need to be careful below +// that ref removal steps interleave safely with ref discovery steps +// (in this thread). void ReferenceProcessor::preclean_discovered_reflist(DiscoveredList& refs_list, BoolObjectClosure* is_alive, @@ -1266,7 +1274,6 @@ VoidClosure* complete_gc, YieldClosure* yield) { DiscoveredListIterator iter(refs_list, keep_alive, is_alive); - size_t length = refs_list.length(); while (iter.has_next()) { iter.load_ptrs(DEBUG_ONLY(true /* allow_null_referent */)); oop obj = iter.obj(); @@ -1281,7 +1288,6 @@ } // Remove Reference object from list iter.remove(); - --length; // Keep alive its cohort. iter.make_referent_alive(); if (UseCompressedOops) { @@ -1291,12 +1297,11 @@ oop* next_addr = (oop*)java_lang_ref_Reference::next_addr(obj); keep_alive->do_oop(next_addr); } + iter.move_to_next(); } else { iter.next(); } } - refs_list.set_length(length); - // Close the reachable set complete_gc->do_void();
--- a/src/share/vm/runtime/globals.hpp Wed Nov 19 14:20:51 2008 -0800 +++ b/src/share/vm/runtime/globals.hpp Thu Nov 20 12:27:41 2008 -0800 @@ -1474,7 +1474,7 @@ "CMSPrecleanNumerator:CMSPrecleanDenominator yields convergence" \ " ratio") \ \ - product(bool, CMSPrecleanRefLists1, false, \ + product(bool, CMSPrecleanRefLists1, true, \ "Preclean ref lists during (initial) preclean phase") \ \ product(bool, CMSPrecleanRefLists2, false, \