RFR: 8151670: Unexpected concurrent refinement deactivation and reactivation

Mikael Gerdin mikael.gerdin at oracle.com
Tue Mar 22 12:52:43 UTC 2016


Hi Kim,

On 2016-03-21 21:39, Kim Barrett wrote:
>> 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.

Ok, that approach makes sense to me.

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

Splitting it into distinct ifs would be ok with me.

/Mikael

>



More information about the hotspot-gc-dev mailing list