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

[David Holmes]

Hi Robbin,

Thanks for taking a look at this.

On 17/09/2019 10:05 pm, Robbin Ehn wrote:
> Hi David,
> 
> Nit, double indent on if-statement here:
> src/hotspot/share/runtime/thread.cpp
> +void JavaThread::interrupt() {
> +  debug_only(check_for_dangling_thread_pointer(this);)
> +
> +    if (!osthread()->interrupted()) {

Fixed. emacs doesn't like those debug_only() constructs and keeps 
"fixing" the indent on the next line. :)

>> 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().
> 
> unpark() do Atomic::xchg, so fence is not needed.
> SetEvent is a syscall, so release is not needed.
> No need to change, you can leave them as is.

I was assuming the key thing was to "flush" the write to the interrupted 
state ASAP to minimise the chance of a race resulting in multiple calls 
to SetEvent/unpark. But it seems to have just been copied through 
history. This dates back to the early Solaris code, where interrupts 
also involved signals, and release semantics were used to write the 
interrupted state. That morphed into an atomic::membar which later 
evolved into OrderAccess::fence(). At the same time OrderAccess was 
introduced the Windows code was updated to "use release like Solaris" - 
even though Solaris was already using a fence(). Linux inherited the 
fence from Solaris; BSD/AIX inherited the fence from Linux and 
eventually it ended up as the POSIX form.

> Looks good, thanks! (no need for an update)

Thanks for the review!

David
-----

> /Robbin
> 
> 
>>
>> Testing:
>>   - hotspot runtime
>>   - Tiers 1-3
>>
>> Thanks,
>> David