RFR: JDK-8043630 Method os::yield_all() should be removed

[Frederic Parain]

Thanks for the review.

Fred

On 06/23/2014 12:00 PM, David Simms wrote:
>
> It looks good to me.
>
> I see sleep for 0 wasn't really an option. And os::naked_short_sleep(1)
> is only used after limited spinning, nice.
>
> /David Simms
>
> On 2014-06-23 10:29, Frederic Parain wrote:
>> I've created JDK-8047714 to keep track of the os::yield()
>> issue on Solaris. However, I still need two reviews for
>> the JDK-8043630 fix.
>>
>> http://cr.openjdk.java.net/~fparain/8043630/webrev.01/
>>
>> Thank you,
>>
>> Fred
>>
>> On 05/23/2014 02:05 PM, Frederic Parain wrote:
>>>
>>> On 23/05/2014 05:52, David Holmes wrote:
>>>> Hi Fred,
>>>>
>>>> On 22/05/2014 11:47 PM, frederic parain wrote:
>>>>> David,
>>>>>
>>>>> Thank you for reviewing, my answers are
>>>>> in-lined below.
>>>>>
>>>>> On 22/05/2014 14:07, David Holmes wrote:
>>>>>> Hi Fred,
>>>>>>
>>>>>> Generally good to see this go but a couple of queries.
>>>>>>
>>>>>> src/share/vm/prims/jni.cpp
>>>>>>
>>>>>> You changed yield_all to yield but on Solaris (actually posix) that
>>>>>> still introduces the thread-state-transition so the
>>>>>> ThreadInVMfromNative
>>>>>> would still seem to be needed.
>>>>>
>>>>> Good catch! I should have replaced yield_all() with NakedYield()
>>>>> instead of yield(). Fixed and webrev updated:
>>>>>
>>>>> http://cr.openjdk.java.net/~fparain/8043630/webrev.01/
>>>>
>>>> Ok. This makes me wonder about NakedYield on Windows though.
>>>>
>>>>> Note: I've launched a new JPRT job to test jdk_nio test suite.
>>>>>
>>>>>> My memory is failing me here as I don't recall the details of the
>>>>>> switch
>>>>>> to using os::posix::sleep on Solaris. But I note that in doing so we
>>>>>> have lost the semantics of sleep(0) that os::yield was relying on
>>>>>> - we
>>>>>> lost the thr_yield call!
>>>>>
>>>>> The Solaris implementation of sleep() has been merged with other
>>>>> Posix implementation to fix CR 6546236 (the race condition between
>>>>> Thread.interrupt() and Thread.sleep()).
>>>>
>>>> Right - now I recall. The problem is that in doing so we lost the
>>>> yield affect of sleep(0) !! So Solaris os::yield is now a no-op!
>>>
>>> That's right.
>>> Solaris implementation of os::yield() should call thr_yield() instead of
>>> os::sleep(0)
>>> (to be consistent with other Posix platforms). But in this case, it is
>>> not justified
>>> anymore to maintain both os::yield() and os::NakedYield(), as they will
>>> have
>>> identical implementation for all platforms.
>>> However, this should be fixed in a different CR.
>>>
>>> Fred
>>>
>>>>
>>>>>> src/share/vm/services/memTracker.hpp
>>>>>>
>>>>>> Aside: I'm curious to know why NakedYield does not work as well on
>>>>>> Windows - does it yield to the "wrong" thread?
>>>>>
>>>>> Zhengyu already answered this one.
>>>>
>>>> Yes though it raises more questions for me :) Although SwitchToThread
>>>> explicitly states that it only switches to a thread ready to run on
>>>> the same processor, I'm not convinced that Sleep(0) necessarily does
>>>> anything different. Moreover I'm not sure I see any situation where
>>>> SwitchToThread is actually what we want. SwitchToThread implies
>>>> per-processor run queues, but the scheduling priorities documentation:
>>>>
>>>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms685100%28v=vs.85%29.aspx
>>>>
>>>>
>>>>
>>>> suggests global run queues. I think the bottom line here is that we
>>>> aren't certain exactly what SwitchToThread or Sleep(0) will actually
>>>> do.
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> Thanks,
>>>>>
>>>>> Fred
>>>>>
>>>>>> On 22/05/2014 9:22 PM, frederic parain wrote:
>>>>>>> Please review the following change to remove
>>>>>>> the os::yield_all() method. This method has
>>>>>>> been source of issues for a long time and it's
>>>>>>> time to get rid of it.
>>>>>>>
>>>>>>> CR:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8043630
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~fparain/8043630/webrev.00/
>>>>>>>
>>>>>>> This changeset has been tested with JPRT (builds and
>>>>>>> tests), vm.quick.testlist, JDK jdk_core.
>>>>>>> I also ran refworkload benchmarks suite which didn't
>>>>>>> show any significant regression.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Fred
>>>>>>>
>>>>>
>>>
>>
>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com