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