RFR: 8151670: Unexpected concurrent refinement deactivation and reactivation

[Kim Barrett]

> On Mar 21, 2016, at 4:15 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
> 
> Hi Kim,

Thanks for reviewing.

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

Well spotted.  This change was intentional, and should have been
discussed in the RFR, but I forgot to mention it.  (In my defense, I
had this change ready to go and then discovered JDK-8152196 and had to
address that first, and forgot about this when I came back.)

Setting the index past the entry for which the closure returned false
always bothered me.  It assumes that the closure fully processed the
entry, even though it is requesting an early termination of the
iteration.  While it is safe to re-process an entry, it is clearly not
safe to leave one unprocessed.  So every time I looked at the old code
I felt a need to ensure that all closures did things in the right
order.  The change eliminates that concern.

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

Would it help to change the comment to be more prescriptive?  Such as

  Deactivate because number of buffers fell below threshold.

Or split the if-then-else-if into two distinct ifs?  I'm not keen on
introducing a boolean flag whose assignment is far away from its use,
as would be the case if it were checked in the while loop test.