RFR: 8151670: Unexpected concurrent refinement deactivation and reactivation (original) (raw)

Kim Barrett kim.barrett at oracle.com
Mon Mar 21 20:39:45 UTC 2016

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::applyclosuretobuffer(CardTableEntryClosure* cl, 154 BufferNode* node, 155 bool consume, 156 uint workeri) { It seems like the old code set the index to the next unprocessed entry when docardptr returned false. 164 if (!cl->docardptr(cardptr, workeri)) { 165 if (consume) { 166 sizet newindex = DirtyCardQueue::indextobyteindex(i + 1); 167 assert(newindex <= buffersize(), "invariant");_ _168 node->setindex(newindex); 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::runservice() { It took me a while to notice the "break" after the call to applyclosuretocompletedbuffer. 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 applyclosuretocompletedbuffer 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.

More information about the hotspot-gc-dev mailing list