RFR (S): 8003135: HotSpot inlines and hoists the Thread.currentThread().isInterrupted() out of the loop

[Vladimir Ivanov]

John,

Thanks for the review!

 >> Proposed fix is cleaner that [1] (no explicit memory state management).
 >> The place for the barrier is deliberately chosen earlier than 
necessary and it blocks hoisting of some loop invariants when 
Thread.isInterrupted() is called in a loop. But I think it's a fair 
price since I don't think that Thread.isInterrupted(Z)Z performance in 
tight loops is critical.
 >
 > I don't see any new barrier in the simple fix.  Where is it?  It 
looks like the load of thr._interrupted can be hoisted under the same 
circumstances as before.  If the slow path is somehow turned into a loop 
exit (via uncommon trap, maybe), then the memory state will fold up and 
the bit load can be hoisted.

I placed it at the very beginning of the function.

I'm not very comfortable with memory state management so far, but I 
don't see how it differs from "fast" fix. Memory graph seems almost the 
same between these 2 versions. The only difference is that the check of 
current thread is after the barrier.

I'll experiment with converting slow call into uncommon trap and see how 
it works.

 > It seems like your "fast" fix is more robust, for this reason.
 >
 > Also, it's not your bug, but I'm not convinced that "slow_val = 
intcon(1)" is correct.  I think another thread could come in and clear 
the interrupted bit, and this call to the native function could return 
false.
Good catch! You are right, there's a window (though it's very small) 
when another thread can interrupt the thread. I didn't like 
known_current_thread variable and now I have a reason to completely 
remove it :-)

Best regards,
Vladimir Ivanov

>> Proposed fix is cleaner that [1] (no explicit memory state management).
>> The place for the barrier is deliberately chosen earlier than necessary and it blocks hoisting of some loop invariants when Thread.isInterrupted() is called in a loop. But I think it's a fair price since I don't think that Thread.isInterrupted(Z)Z performance in tight loops is critical.
>
> I don't see any new barrier in the simple fix.  Where is it?  It looks like the load of thr._interrupted can be hoisted under the same circumstances as before.  If the slow path is somehow turned into a loop exit (via uncommon trap, maybe), then the memory state will fold up and the bit load can be hoisted.
I placed it at the beginning of the function.

> It seems like your "fast" fix is more robust, for this reason.
>
> Also, it's not your bug, but I'm not convinced that "slow_val = intcon(1)" is correct.  I think another thread could come in and clear the interrupted bit, and this call to the native function could return false.
Good catch! You are right, there's a very small window there.

>> If you prefer [1] let me know.
>>
>> Also, cleaned up the code around a little.
>
> Good cleanups.
>
> — John
>
>> Testing: failing test, manual (verified generated code), JPRT (in progress)
>>
>> Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> [1] http://cr.openjdk.java.net/~vlivanov/8003135/fast/webrev.00
>