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