On 2015-06-24 10:15, Bengt Rutisson wrote:
>
> Hi Per,
>
> Thanks for looking at this!
>
> On 2015-06-24 10:09, Per Liden wrote:
>> Hi Bengt,
>>
>> On 2015-06-24 09:28, Bengt Rutisson wrote:
>>>
>>>
>>> Hi Vitaly,
>>>
>>> On 2015-06-23 23:53, Vitaly Davidovich wrote:
>>>>
>>>> Naive question - could this be converted into a numeric state field
>>>> that indicates the lifecycle (e.g 1 = in progress, 2 = started)?
>>>>
>>>
>>> Good question! That's a much simpler and more stable solution.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~brutisso/8129626/webrev.02/
>>
>> This looks much nicer, and I can't think of any reason why that
>> wouldn't work. One little request though, can we name the states to
>> match the function names, like Idle/Started/InProgress? And
>> clear_in_progress() should probably be set_idle() to align with the rest.
>
> Yes, I was struggling a bit with the naming. What do you think about this?
>
> cr.openjdk.java.net/~brutisso/8129626/webrev.03/
Looks good!
cheers,
/Per
>
> Thanks,
> Bengt
>>
>> cheers,
>> /Per
>>
>>>
>>> Thanks,
>>> Bengt
>>>
>>>
>>>
>>>> sent from my phone
>>>>
>>>> On Jun 23, 2015 5:42 PM, "bill pittore" <bill.pittore at oracle.com
>>>> <mailto:bill.pittore at oracle.com>> wrote:
>>>>
>>>> Generally when you have a storestore on the write side you need a
>>>> loadload on the read side to prevent the second read from floating
>>>> above the first one. The reading thread could read in_progress as
>>>> 0 before it reads starting. Meanwhile the write thread writes 1 to
>>>> in_progress, issues storestore, clears starting. Reading thread
>>>> then reads starting as 0. I don't know if the CGC mutex somehow
>>>> eliminates this issue as I'm not familiar with the code in detail.
>>>>
>>>> bill
>>>>
>>>> On 6/23/2015 4:25 PM, Bengt Rutisson wrote:
>>>>>
>>>>> Hi everyone,
>>>>>
>>>>> Could I have a couple of reviews for this change?
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8129626
>>>>> http://cr.openjdk.java.net/~brutisso/8129626/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Ebrutisso/8129626/webrev.00/>
>>>>>
>>>>> We need to add a barrier between the calls to set_in_progress()
>>>>> and clear_started() to make sure that other threads sees the
>>>>> correct value when they use ConcurrentMarkThread::during_cycle().
>>>>>
>>>>> Thanks to Per and Bertrand for helping out identifying and
>>>>> sorting this out.
>>>>>
>>>>> From the bug report:
>>>>>
>>>>> ConcurrentMarkThread::during_cycle() is implemented as:
>>>>>
>>>>> bool during_cycle() { return started() || in_progress(); }
>>>>>
>>>>> So, it checks both ConcurrentMarkThread::_started and
>>>>> ConcurrentMarkThread::_in_progress and they are meant to overlap.
>>>>> That is, we should not set _started to false until after we have
>>>>> set _in_progress to true. This is done in sleepBeforeNextCycle():
>>>>>
>>>>> void ConcurrentMarkThread::sleepBeforeNextCycle() {
>>>>> // We join here because we don't want to do the
>>>>> "shouldConcurrentMark()"
>>>>> // below while the world is otherwise stopped.
>>>>> assert(!in_progress(), "should have been cleared");
>>>>>
>>>>> MutexLockerEx x(CGC_lock, Mutex::_no_safepoint_check_flag);
>>>>> while (!started() && !_should_terminate) {
>>>>> CGC_lock->wait(Mutex::_no_safepoint_check_flag);
>>>>> }
>>>>>
>>>>> if (started()) {
>>>>> set_in_progress();
>>>>> clear_started();
>>>>> }
>>>>> }
>>>>>
>>>>> On non-TSO platforms there is a risk that the write to
>>>>> _in_progress (from set_in_progress()) is seen by other threads
>>>>> after the write to _started (in clear_started()). In that case
>>>>> there is a window when during_cycle() may return false even
>>>>> though we are in a concurrent cycle.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>>
>>>
>