RFR (S): 8230423: Move os::sleep to JavaThread::sleep

[Daniel D. Daugherty]

On 9/11/19 7:21 PM, David Holmes wrote:
> On 12/09/2019 8:26 am, Daniel D. Daugherty wrote:
>> On 9/11/19 6:19 PM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> Are you okay with this?
>>
>> Yes... at the bottom of that review I put:
>>
>>> Thumbs up! I don't need to see a new webrev if you decide to
>>> fix the nits. 
>>
>> so I didn't think I needed to reply to your filling in
>> the query about interrupts and JavaThread*...
>
> I wanted to be sure my answer didn't prompt additional concerns.

None from me.

Dan


>
> Thanks,
> David
> -----
>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 10/09/2019 7:41 am, David Holmes wrote:
>>>> Hi Dan,
>>>>
>>>> Thanks for taking a look.
>>>>
>>>> On 10/09/2019 3:27 am, Daniel D. Daugherty wrote:
>>>>> On 9/9/19 1:21 AM, David Holmes wrote:
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8230423
>>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8230423/webrev/
>>>>>
>>>>> src/hotspot/cpu/x86/rdtsc_x86.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/os/posix/os_posix.cpp
>>>>>      L648:   assert(thread->is_Java_thread(), "invariant");
>>>>>      L649:   JavaThread* jt = (JavaThread*) thread;
>>>>>          I was expecting this change with your "interrupt" bug.
>>>>>          Are we ready for this assert() now?
>>>>>
>>>>> src/hotspot/os/windows/os_windows.cpp
>>>>>      L3600:   assert(thread->is_Java_thread(), "invariant");
>>>>>      L3601:   JavaThread* jt = (JavaThread*) thread;
>>>>>          I was expecting this change with your "interrupt" bug.
>>>>>          Are we ready for this assert() now?
>>>>
>>>> It is already true that only JavaThreads use the interrupt 
>>>> facility. The forthcoming changes to the interrupt just make that 
>>>> more obvious:
>>>>
>>>> os::interrupt is-called-from
>>>>    Thread::interrupt is called from
>>>>      JVM_Interrupt  // java.lang.Thread interrupt
>>>>      JvmtiEnv::InterruptThread // JVM TI InterruptThread
>>>>      JavaThread::send_thread_stop
>>>>
>>>> so always applied to a JavaThread.
>>>>
>>>>> src/hotspot/share/gc/shenandoah/shenandoahPacer.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/jvmci/jvmciCompiler.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/prims/jvm.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/runtime/os.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/runtime/os.hpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/runtime/thread.cpp
>>>>>      L1701:   _SleepEvent  = ParkEvent::Allocate(this);
>>>>>      L1815:   _SleepEvent  = NULL;
>>>>>          nit - please delete extra blank before '='.
>>>>>
>>>>>      L3403 - nit - please delete extra blank line.
>>>>
>>>> Nits will be fixed.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> src/hotspot/share/runtime/thread.hpp
>>>>>      No comments.
>>>>>
>>>>> test/hotspot/gtest/gc/g1/test_g1FreeIdSet.cpp
>>>>>      No comments.
>>>>>
>>>>> test/hotspot/gtest/gc/shared/test_ptrQueueBufferAllocator.cpp
>>>>>      No comments.
>>>>>
>>>>> test/hotspot/gtest/utilities/test_singleWriterSynchronizer.cpp
>>>>>      No comments.
>>>>>
>>>>>
>>>>> Thumbs up! I don't need to see a new webrev if you decide to
>>>>> fix the nits.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>> This is the next step in the sleep/interrupt support code 
>>>>>> reshuffle. Now that os::sleep is platform independent and only 
>>>>>> applicable to JavaThreads, we can move it to JavaThread as an 
>>>>>> instance method.
>>>>>>
>>>>>> Summary of changes:
>>>>>> - os::sleep moved to JavaThread::sleep as instance method
>>>>>> - signature changed to return bool - true means timed-out; false 
>>>>>> means interrupted
>>>>>> - rearranged the sleep code slightly to remove the initial 
>>>>>> back-to-back nanoTime() calls - as suggested by Roger.
>>>>>> - _SleepEvent moved from Thread to JavaThread
>>>>>> - minor changes to os::interrupt to account for move to 
>>>>>> JavaThread (this code will also be moved to JavaThread in the 
>>>>>> next fix)
>>>>>> - call sites changed from os::sleep(t,ms) to t->sleep(ms)
>>>>>>
>>>>>> Testing:
>>>>>>  - tiers 1-3
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>
>>