RFR: 8151670: Unexpected concurrent refinement deactivation and reactivation

[Mikael Gerdin]

Hi Kim,

On 2016-03-19 00:10, Kim Barrett wrote:
> Please review this change to the concurrent refinement threads to use
> SuspendibleThreadSet::yield, rather than deactivation, in response to
> an STS suspension request. This should mostly reduce the cost of
> coming out of a non-GC safepoint. See the CR for further discussion.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8151670
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8151670/webrev.00/

In
  153 bool 
DirtyCardQueueSet::apply_closure_to_buffer(CardTableEntryClosure* cl,
  154                                                 BufferNode* node,
  155                                                 bool consume,
  156                                                 uint worker_i) {

It seems like the old code set the index to the next unprocessed entry 
when do_card_ptr returned false.

  164     if (!cl->do_card_ptr(card_ptr, worker_i)) {
  165       if (consume) {
  166         size_t new_index = DirtyCardQueue::index_to_byte_index(i + 1);
  167         assert(new_index <= buffer_size(), "invariant");
  168         node->set_index(new_index);
  169       }
  170       return false;


In your version it seems like the index is set to the same entry.

Perhaps, instead of a "break", you wanted
162   for ( ; i < limit && result; ++i) {

I think this would make the setting of the new index work the same as 
the old version of the code.


In
  111 void ConcurrentG1RefineThread::run_service() {

It took me a while to notice the "break" after the call to 
apply_closure_to_completed_buffer.
Perhaps there is a way to restructure the control flow to make this a 
bit more obvious?

One variant could be to save the result of 
apply_closure_to_completed_buffer in a boolean and check boolean in the 
while() loop header.


/Mikael


>
> Testing:
> JPRT, RBT GC Nightly, local specjbb2015.
>
>