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

[Frederic Parain]

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