RFR: 8230424: Use platform independent code for Thread.interrupt support

[David Holmes]

Thanks for the review Dan!

David

On 18/09/2019 12:13 am, Daniel D. Daugherty wrote:
> On 9/13/19 5:12 AM, David Holmes wrote:
>> webrev: http://cr.openjdk.java.net/~dholmes/8230424/webrev/
> 
> src/hotspot/os/posix/os_posix.cpp
>      The old _sleepEvent code had a check for a NULL _SleepEvent
>      before unparking.
>      The old _ParkEvent code had a check for a NULL _ParkEvent
>      before unparking.
> 
>      Update: The code review invite covered this:
> 
>        > - minor cleanups to the logic of interrupt() as we can never
>        >   have NULL events as we are always guaranteed to be operating
>        >   on a live thread.
> 
> src/hotspot/os/solaris/os_solaris.cpp
>      No comments.
> 
> src/hotspot/os/windows/osThread_windows.cpp
>      No comments.
> 
> src/hotspot/os/windows/osThread_windows.hpp
>      No comments.
> 
> src/hotspot/os/windows/os_windows.cpp
>      Where did the JSR166 support move to?
>      Update: moved to thread.cpp so okay.
> 
> src/hotspot/share/jvmci/jvmciRuntime.cpp
>      No comments.
> 
> src/hotspot/share/prims/jvm.cpp
>      No comments.
> 
> src/hotspot/share/prims/jvmtiEnv.cpp
>      No comments.
> 
> src/hotspot/share/prims/jvmtiRawMonitor.cpp
>      No comments.
> 
> src/hotspot/share/runtime/objectMonitor.cpp
>      No comments (the Self -> jt changes threw me for a second :-)).
> 
> src/hotspot/share/runtime/os.hpp
>      No comments.
> 
> src/hotspot/share/runtime/osThread.cpp
>      No comments.
> 
> src/hotspot/share/runtime/osThread.hpp
>      No comments.
> 
> src/hotspot/share/runtime/thread.cpp
>      No comments.
> 
> src/hotspot/share/runtime/thread.hpp
>      No comments.
> 
> 
> Thumbs up! Robbin flagged the one nit indent issue that I spotted.
> I don't need to see another webrev.
> 
> Dan
> 
> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8230424
>>
>> The next instalment of the interruption logic refactoring.
>>
>> Summary of changes:
>> - the windows-specific actions in os:interrupt and os::is_interrupted 
>> are pushed down into the windows-specific osThread code. This all 
>> relates to the only-used-by-the-JDK _interrupt_event.
>> - the POSIX variant of os::interrupt and os::is_interrupted is inlined 
>> as platform-independent code in the Thread versions and those versions 
>> are themselves moved to JavaThread as instance methods.
>> - minor cleanups to the logic of interrupt() as we can never have NULL 
>> events as we are always guaranteed to be operating on a live thread.
>> - All call sites adjusted as needed.
>>
>> One minor wart in this refactor is that on Windows we will execute an 
>> OrderAccess::release() after setting the interrupt state in the 
>> platform code, and then execute an additional fence() in the shared 
>> code. I don't think this is worth trying to "fix" - and hopefully if 
>> we can get rid of the Windows _interrupt_event this will all go away.
>>
>> If you are wondering why Windows used OrderAccess::release while POSIX 
>> used OrderAccess::fence, I have no idea, and I do not intend to change 
>> either of them. The only safe change would be to use fence() in both 
>> cases, which just makes the Windows wart worse - and unnecessarily so 
>> as we have seen no bugs using the existing release().
>>
>> Testing:
>>  - hotspot runtime
>>  - Tiers 1-3
>>
>> Thanks,
>> David
>