RFR (S): 8183128: Update RefineCardTableEntryClosure
Thomas Schatzl
thomas.schatzl at oracle.com
Mon Jul 10 12:52:34 UTC 2017
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
More information about the hotspot-gc-dev
mailing list