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

[David Holmes]

Reviewed.

Thanks,
David

On 23/06/2014 6:29 PM, 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
>>>>>>
>>>>
>>
>