RFR: 8075215: SATB buffer processing found reclaimed humongous object

[Bengt Rutisson]

Hi Kim,

On 2015-04-28 19:55, Kim Barrett wrote:
> Please review this fix for a problem with eager reclaim of humongous
> objects.
>
> The elements of SATB buffers are being processed in the same way as
> elements of the mark queue and when scanning objects (e.g. by calling
> CMTask::deal_with_reference).  That no longer works because SATB
> buffers may contain references to reclaimed humongous objects.
>
> Scrubbing the SATB buffers after eager reclaim of humongous objects
> would extend the pause.  We instead take advantage of the tests needed
> for deciding whether a reference needs to be marked being sufficient
> to also filter out stale references to reclaimed humongous objects.
> We just need to be more circumspect when processing the SATB buffers
> and not assume (and assert) the entries are valid objects until those
> tests have been applied, unlike in other cases.
>
> To support this new behavior:
>
> - CMTask::deal_with_reference is now split into an outer part that
> performs tests on an object to determine whether it needs to be shaded
> grey, and an inner part that performs the shading.
>
> - SATB buffer is changed to perform similar tests on possibly stale
> pointers, and once verified to be interesting, then call the shared
> inner part to perform the shading.  To enable the new SATB buffer
> processing, the closure involved is changed from an ObectClosure to a
> new SATBBufferClosure.
>
> Also made ObjPtrQueue::apply_closure_and_empty() public to eliminate
> the need for some friends.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8075215
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8075215/webrev.00/

Over all the fix looks good. Nice work!

A couple of minor style questions. Feel free to ignore them:

concurrentMark.cpp:

2588   CMSATBBufferClosure _cm_satb;

Do you mind calling the variable _cm_satb_cl instead? I think that is 
more consistent with the other two closures declared in the same place, 
_cm_cl and _code_cl.

2594   G1RemarkThreadsClosure(G1CollectedHeap* g1h, CMTask* task) :
2595     _cm_satb(task, g1h), _cm_cl(g1h, g1h->concurrent_mark(), task),
2596     _code_cl(&_cm_cl, !CodeBlobToOopClosure::FixRelocations),

To me this looks like two instance variables are being initialized when 
in fact there are three. I would prefer a line break between the first 
and the second, i.e:

   G1RemarkThreadsClosure(G1CollectedHeap* g1h, CMTask* task) :
     _cm_satb(task, g1h),
     _cm_cl(g1h, g1h->concurrent_mark(), task),
     _code_cl(&_cm_cl, !CodeBlobToOopClosure::FixRelocations),


concurrentMark.inline.hpp

  292 inline void CMTask::make_reference_grey(oop obj, HeapRegion* hr) {
  293   if (_cm->par_mark_and_count(obj, hr, _marked_bytes_array, 
_card_bm)) {
             ...
  341   }
  342 }

I think I would prefer an early exit rather than having the whole method 
nested one indentation level. So, something like:

inline void CMTask::make_reference_grey(oop obj, HeapRegion* hr) {
   bool marked = _cm->par_mark_and_count(obj, hr, _marked_bytes_array, 
_card_bm);
   if (!marked) {
      // Another thread beat us to this object.
      return;
}
   ...
}


satbQueue.cpp

// If G1SATBBufferEnqueueingThresholdPercent == 0 we could skip filtering.

Do you want to fix this too in a separate change? In that case, will you 
file a bug to track that?


Thanks,
Bengt

>
> Testing:
> JPRT
> Aurora ad hoc GC Nightly and VM quicktests with G1 as the collector
> local jtreg with G1 as the collector
> local refworkload with G1 as the collector
>