Request for review (XS): 6662086 6u2+, 7b11+: CMS never clears referents when -XX:+ParallelRefProcEnabled

[Y Srinivas Ramakrishna]

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;