RFR (S): 8183128: Update RefineCardTableEntryClosure

[Erik Helin]

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.

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 ;))

Thanks,
Erik

> This change is dependent on "JDK-8183226: Remembered set summarization
> accesses not fully initialized java thread DCQS" which is also
> currently out for review - that change reorganizes G1CollectedHeap
> initialization so that the change can actually move the closure.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8183128
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8183128/webrev/
> Testing:
> jprt, local benchmarks
>
> Thanks,
>   Thomas
>