Request for review (XS): 6662086 6u2+, 7b11+: CMS never clears referents when -XX:+ParallelRefProcEnabled
Y Srinivas Ramakrishna
Y.S.Ramakrishna at Sun.COM
Tue May 6 22:55:23 UTC 2008
A small correction. I fixed the synopsis of this CR to read:-
6u4+, 7b11+: CMS never clears referents when -XX:+ParallelRefProcEnabled
reflecting the correct versions in which the regression happened.
The fix has since been verified by the customers that reported the problem
(the most recent, a second report last night), and will be integrated shortly.
-- ramki
----- Original Message -----
From: Y Srinivas Ramakrishna <Y.S.Ramakrishna at Sun.COM>
Date: Thursday, May 1, 2008 4:44 pm
Subject: Request for review (XS): 6662086 6u2+, 7b11+: CMS never clears referents when -XX:+ParallelRefProcEnabled
To: hotspot-gc-dev at openjdk.java.net
> Thanks for your reviews....
>
> Fixed: 6662086 6u2+, 7b11+: CMS never clears referents when -XX:+ParallelRefProcEnabled
>
> This fixes a regression introduced in 6u2, 7b11, because of which CMS
> would not
> correctly clear referents when +ParallelRefProcEnabled. This was because
> the CMSIsAliveClosure used during the processing was using an empty span.
> The symptom would typically manifest as a monotonically growing population
> of referents until a stop-world mark-compact collection would reclaim
> them.
>
> Thanks to Thomas Viessmann and Kevin Walls for debugging assistance,
> and for collecting and analysing instrumentation data at customer site.
>
> Fix Verified: <will be tested>
> Verification Testing: <will be tested at customer site>
> Other testing: <in progress: jprt, gc_test_suite, refworkload>
>
> webrev: http://analemma.sfbay.sun.com/net/spot/workspaces/ysr/hg_gc_fixes/webrev/
>
> Patch:
> =====
> ---
> old/src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp Thu
> May 1 16:05:09 2008
> +++
> new/src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp Thu
> May 1 16:05:08 2008
> @@ -283,7 +283,7 @@
> // processing phase of the CMS final checkpoint step.
> class CMSKeepAliveClosure: public OopClosure {
> CMSCollector* _collector;
> - MemRegion _span;
> + const MemRegion _span;
> CMSMarkStack* _mark_stack;
> CMSBitMap* _bit_map;
> public:
> @@ -292,7 +292,9 @@
> _collector(collector),
> _span(span),
> _bit_map(bit_map),
> - _mark_stack(mark_stack) { }
> + _mark_stack(mark_stack) {
> + assert(!_span.is_empty(), "Empty span could spell trouble");
> + }
>
> void do_oop(oop* p);
> void do_oop_nv(oop* p) { CMSKeepAliveClosure::do_oop(p); }
> ---
> old/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Thu
> May 1 16:05:12 2008
> +++
> new/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp Thu
> May 1 16:05:11 2008
> @@ -520,7 +520,10 @@
> -1 /* lock-free */, "No_lock" /* dummy */),
> _modUnionClosure(&_modUnionTable),
> _modUnionClosurePar(&_modUnionTable),
> - _is_alive_closure(&_markBitMap),
> + // Adjust my span to cover old (cms) gen and perm gen
> + _span(cmsGen->reserved()._union(permGen->reserved())),
> + // Construct the is_alive_closure with _span & markBitMap
> + _is_alive_closure(_span, &_markBitMap),
> _restart_addr(NULL),
> _overflow_list(NULL),
> _preserved_oop_stack(NULL),
> @@ -572,11 +575,6 @@
> _cmsGen->cmsSpace()->set_collector(this);
> _permGen->cmsSpace()->set_collector(this);
>
> - // Adjust my span to cover old (cms) gen and perm gen
> - _span = _cmsGen->reserved()._union(_permGen->reserved());
> - // Initialize the span of is_alive_closure
> - _is_alive_closure.set_span(_span);
> -
> // Allocate MUT and marking bit map
> {
> MutexLockerEx x(_markBitMap.lock(), Mutex::_no_safepoint_check_flag);
> @@ -5493,7 +5491,7 @@
> typedef AbstractRefProcTaskExecutor::ProcessTask ProcessTask;
> CMSCollector* _collector;
> CMSBitMap* _mark_bit_map;
> - MemRegion _span;
> + const MemRegion _span;
> OopTaskQueueSet* _task_queues;
> ParallelTaskTerminator _term;
> ProcessTask& _task;
> @@ -5510,7 +5508,10 @@
> _collector(collector), _span(span), _mark_bit_map(mark_bit_map),
> _task_queues(task_queues),
> _term(total_workers, task_queues)
> - { }
> + {
> + assert(_collector->_span.equals(_span) && !_span.is_empty(),
> + "Inconsistency in _span");
> + }
>
> OopTaskQueueSet* task_queues() { return _task_queues; }
>
> @@ -5531,7 +5532,8 @@
> _mark_bit_map, work_queue(i));
> CMSParDrainMarkingStackClosure par_drain_stack(_collector, _span,
> _mark_bit_map, work_queue(i));
> - CMSIsAliveClosure is_alive_closure(_mark_bit_map);
> + assert(_collector->_span.equals(_span), "Inconsistency in _span");
> + CMSIsAliveClosure is_alive_closure(_span, _mark_bit_map);
> _task.work(i, is_alive_closure, par_keep_alive, par_drain_stack);
> if (_task.marks_oops_alive()) {
> do_work_steal(i, &par_drain_stack, &par_keep_alive,
> ---
> old/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp Thu
> May 1 16:05:16 2008
> +++
> new/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp Thu
> May 1 16:05:15 2008
> @@ -435,23 +435,22 @@
> // if the object is "live" (reachable). Used in weak
> // reference processing.
> class CMSIsAliveClosure: public BoolObjectClosure {
> - MemRegion _span;
> + const MemRegion _span;
> const CMSBitMap* _bit_map;
>
> friend class CMSCollector;
> - protected:
> - void set_span(MemRegion span) { _span = span; }
> public:
> - CMSIsAliveClosure(CMSBitMap* bit_map):
> - _bit_map(bit_map) { }
> -
> CMSIsAliveClosure(MemRegion span,
> CMSBitMap* bit_map):
> _span(span),
> - _bit_map(bit_map) { }
> + _bit_map(bit_map) {
> + assert(!span.is_empty(), "Empty span could spell trouble");
> + }
> +
> void do_object(oop obj) {
> assert(false, "not to be invoked");
> }
> +
> bool do_object_b(oop obj);
> };
>
> @@ -600,7 +599,7 @@
> // ("Weak") Reference processing support
> ReferenceProcessor* _ref_processor;
> CMSIsAliveClosure _is_alive_closure;
> - // keep this textually after _markBitMap; c'tor dependency
> + // keep this textually after _markBitMap and _span; c'tor dependency
>
> ConcurrentMarkSweepThread* _cmsThread; // the thread doing
> the work
> ModUnionClosure _modUnionClosure;
>
More information about the hotspot-gc-dev
mailing list