RFR (S): 8183128: Update RefineCardTableEntryClosure

[Thomas Schatzl]

Hi Erik,

  thanks for your review.

On Thu, 2017-07-06 at 14:52 +0200, Erik Helin wrote:
> Hi Thomas,
> 
> On 07/04/2017 10:24 AM, Thomas Schatzl wrote:
> > 
> > Hi all,
> > 
> >   can I get reviews for this change that renames and cleans up the
> > use
> > of RefineCardTableEntryClosure in the code?
> > 
> > RefineCardTableEntryClosure is the closure that is applied by the
> > concurrent refinement threads. This change renames it slightly to
> > indicate its use (G1RefineCardConcurrentlyClosure) and moves it to
> > the G1RemSet files close to the closure that we use for
> > refinement/Update RS during GC.
> great cleanup! Looking at the code, what do you think about moving 
> G1RefineCardConcurrentlyClosure into concurrentG1RefineThread.cpp
> (and make it a private class to ConcurrentG1RefineThread)? AFAICS, 
> ConcurrentG1RefineThread is the only code using this closure.
> 

There are also other users of that closure, e.g. the DCQS's need a
reference to it during initialization.

However by moving the G1RefineCardConcurrentlyClosure and some
refactoring there are (imho) some gains in encapsulation as we
discussed.

> If we do it this way, then we can actually make 
> DirtyCardQueueSet::apply_closure_to_completed_buffer a template
> method,  taking the Closure a template, as in:
> template <typename Closure>
> bool apply_closure_to_completed_buffer(Closure* cl,
>                                         uint worker_i,
>                                         size_t stop_at,
>                                         bool during_pause)
> This means that closures could get inlined, which doesn't mean that
> much for G1RefineCardConcurrentlyClosure, but could give a small
> boost for G1RefineCardClosure (for that to work, 
> G1CollectedHeap::iterate_dirty_card_closure must take a 
> G1RefineCardClosure, but that is ok, because that is the only closure
> type we pass to that method).
> 
> Also, you do not need the forward declaration in G1CollectedHeap, it 
> will not make use of this closure then :)
> 
> If you want to "go the extra mile", then you can also pass a
> G1RemSet* as an argument to the G1RefineCardConcurrentlyClosure
> constructor and store it in a field, to avoid accessing the
> G1CollectedHeap via the singleton:
> G1CollectedHeap::heap()->g1_rem_set()-
> >refine_card_concurrently(card_ptr, 
> worker_i); (plus, G1RefineCardConcurrentlyClosure only needs a
> G1RemSet* pointer anyway ;))

I think these perf improvements should be targeted in a different CR.
:) The change already doubled in size...

Webrevs for current changes:
http://cr.openjdk.java.net/~tschatzl/8183128/webrev.0_to_1 (diff)
http://cr.openjdk.java.net/~tschatzl/8183128/webrev.1 (full)

Thanks,
  Thomas