RFR: 8151670: Unexpected concurrent refinement deactivation and reactivation

[Mikael Gerdin]

Hi Kim,

On 2016-03-23 08:41, Kim Barrett wrote:
>> On Mar 22, 2016, at 12:45 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
>>
>> Kim,
>>
>> Looks correct.  I assume there will be a new version with a few
>> changes from Mikael's comments.
>
> Thanks Jon.  Replies below.
>
> New webrev:
> full: http://cr.openjdk.java.net/~kbarrett/8151670/webrev.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8151670/webrev.01.inc/

I'm ok with this version.

One thing, though.
  178 inline void assert_fully_consumed(BufferNode* node, size_t 
buffer_size) {
Moving assertions to inline functions is problematic because on Windows 
we cannot print the stack trace from an assertion context, so if this 
assert fires off on Windows there is no way to know which caller 
triggered it since the line number will always point to the inline function.

Would you be ok with making this a macro or passing in __LINE__ to 
assert_fully_consumed and print something like
"Caller at " __FILE__ ":%d", line

I don't need to see a new webrev for either of those approaches if you 
decide to follow them.

/Mikael

>
>> Couple of minor points.
>>
>> http://cr.openjdk.java.net/~kbarrett/8151670/webrev.00/src/share/vm/gc/g1/dirtyCardQueue.hpp.frames.html
>>
>> Extra "to" (end of 84 and beginning of 85).
>>>    84   // function.  If "consume" is true, the node's index is updated to
>>> 85 // to exclude the processed elements, e.g. up to the element for
>
> Fixed.
>
>> http://cr.openjdk.java.net/~kbarrett/8151670/webrev.00/src/share/vm/gc/g1/dirtyCardQueue.cpp.frames.html
>>
>> Instead of
>>
>>> 186 assert(node->index() == buffer_size(), "apply said fully consumed");
>>
>> maybe something a little more wordy
>>
>> assert (node->index() == buffer_size(), "Buffer was not fully processed as claimed: index " SIZE_FORMAT " buffer_size " SIZE_FORMAT, node->index(), buffer_size());
>>
>> Would this assertion actually fit better at the end of
>>
>> 153 bool DirtyCardQueueSet::apply_closure_to_buffer(CardTableEntryClosure* cl,
>>
>> 174   }
>>
>> assert (!result || node->index() == buffer_size(), "Buffer was not fully processed as claimed: index " SIZE_FORMAT " buffer_size " SIZE_FORMAT, node->index(), buffer_size());
>> 175 return result;
>> 176 }
>>
>> so that you don't need it in apply_closure_to_completed_buffer()
>> and mut_process_buffer()?
>
> I added an assertion helper that provides more information, and called from the two
> with the previous limited content asserts.  I don’t think apply_closure_to_buffer is the
> right place for it; it’s the other two places that are making decisions that will be wrong
> if that assertion doesn’t hold.
>