RFR (M): 8087198: G1 card refinement: batching, sorting

[Man Cao]

Thanks for the suggestions. I agree with all of them and addressed them:
https://cr.openjdk.java.net/~manc/8087198/webrev.03/
https://cr.openjdk.java.net/~manc/8087198/webrev.02-03.inc/

I have rerun correctness tests and they passed.

src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp
>  321   bool refine() {
>  322     size_t first_clean_index = clean_cards();
> Is it worth checking whether the cleaned buffer is now empty, skipping
> the rest altogether if so?  I don't know how often that actually
> happens, and the rest isn't *that* expensive in the empty buffer case.
> So I'm going to guess not worth the extra test, but something to consider.


I did a quick experiment to count the percentage of completely discarded
buffers
versus all refined buffers, using the default parameters
(+G1UseAdaptiveConcRefinement, G1UpdateBufferSize=256).
In some cases the percentage is not small:
BigRamTester: 10-30% during the phase of filling up the hashmap. Close to
zero during the phase of random accessing the hashmap.
DaCapo h2: could be 20-30% just before the full GC between two iterations.
Close to zero during the iteration.

So I added check. With the future change for the epoch synchronization for
JDK-8226731,
we definitely want to avoid unnecessary epoch synchronization as much as
possible.

-Man


On Thu, Nov 21, 2019 at 12:37 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On Nov 19, 2019, at 9:26 PM, Man Cao <manc at google.com> wrote:
> >
> > Hi all,
> >
> > Thanks! I have addressed all comments:
> > Full: http://cr.openjdk.java.net/~manc/8087198/webrev.02/
> > Incremental: http://cr.openjdk.java.net/~manc/8087198/webrev.01-02.inc/
> >
> >
> > I also changed G1RemSet::clean_card_before_refine() to take a
> "CardValue**"
> > parameter instead of "CardValue*&" to make it more obvious that it can
> > modify
> > the card pointer.
>
> Thanks for that change; that seems to have made the code nicer.
>
> This is looking pretty good.  Just a couple of possible improvements
> and questions.
>
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp
>  247     QuickSort::sort(&_node_buffer[start_index],
> ...
>  250                     true);
>
> I *think* a false value for idempotent is better here.
>
> We don't care about reordering of equal values, so there's no
> correctness issue either way.  A true value will avoid unnecessary
> swaps of equal entries, at the cost of an extra comparison.  The
> comparison is very cheap, so if equal entries were somewhat common
> that could be a win.  But because of the dirty-card-based filtering, I
> think equal entries are uncommon, making the extra comparisons
> wasteful.
>
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp
>  289   bool refine_cleaned_cards(size_t start_index) {
>  290     for (size_t i = start_index; i < _node_buffer_size; ++i) {
>  291       if (SuspendibleThreadSet::should_yield()) {
>  292         redirty_unrefined_cards(i);
>  293         _node->set_index(i);
>  294         return false;
>  295       }
>  296       _g1rs->refine_card_concurrently(_node_buffer[i], _worker_id);
>  297       (*_total_refined_cards)++;
>
>  298     }
>  299     _node->set_index(_node_buffer_size);
>  300     return true;
>  301   }
>
> It would be better to bulk increment *_total_refined_cards at the end,
> rather than on each iteration.  Maybe something like this:
>
> bool refine_cleaned_cards(size_t start_index) {
>   bool result = true;
>   size_t i = start_index;
>   for ( ; i < _node_buffer_size; ++i) {
>     if (SuspendibleThreadSet::should_yield()) {
>       redirty_unrefined_cards(i);
>       result = false;
>       break;
>     }
>     _g1rs->refine_card_concurrently(_node_buffer[i], _worker_id);
>   }
>   _node->set_index(i);
>   *_total_refined_cards += i - start_index;
>   return result;
> }
>
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1DirtyCardQueue.cpp
>  321   bool refine() {
>  322     size_t first_clean_index = clean_cards();
>
> Is it worth checking whether the cleaned buffer is now empty, skipping
> the rest altogether if so?  I don't know how often that actually
> happens, and the rest isn't *that* expensive in the empty buffer case.
> So I'm going to guess not worth the extra test, but something to consider.
>
>
> ------------------------------------------------------------------------------
>
>