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

[Frederic Parain]

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