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

[David Holmes]

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!

>> 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
>>>
>