On 2015-06-24 11:27, Bertrand Delsart wrote:
> Looks good... and much simpler :-)
Thanks, Bertrand!
Bengt
>> Bertrand.
>> On 24/06/2015 10:31, Bengt Rutisson wrote:
>>>>>> On 2015-06-24 10:34, Per Liden wrote:
>>> 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!
>>>> Thanks Per!
>>>> Bengt
>>>>>>>> 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
>>>>>>>>>>>>>>>>>>>>>