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

[Kim Barrett]

> 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.

------------------------------------------------------------------------------