RFR: JDK-8043630 Method os::yield_all() should be removed
frederic.parain at oracle.com
Fri May 23 12:05:50 UTC 2014
On 23/05/2014 05:52, David Holmes wrote:
> Hi Fred,
> On 22/05/2014 11:47 PM, frederic parain wrote:
>> 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.
>>> You changed yield_all to yield but on Solaris (actually posix) that
>>> still introduces the thread-state-transition so the
>>> would still seem to be needed.
>> Good catch! I should have replaced yield_all() with NakedYield()
>> instead of yield(). Fixed and webrev updated:
> 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
>>> 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!
Solaris implementation of os::yield() should call thr_yield() instead of
(to be consistent with other Posix platforms). But in this case, it is
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.
>>> 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:
> 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.
>>> 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.
>>>> 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.
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email:Frederic.Parain at oracle.com
More information about the hotspot-runtime-dev