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

[Vladimir Kozlov]

Looks good.

Vladimir

On 12/14/12 4:08 AM, Vladimir Ivanov wrote:
> John, Vladimir K.,
>
> Here's updated version [1]
>
> Best regards,
> Vladimir Ivanov
>
> [1] http://cr.openjdk.java.net/~vlivanov/8003135/simple/webrev.01/
>
> On 12/14/12 4:11 AM, Vladimir Kozlov wrote:
>> On 12/13/12 4:55 PM, Vladimir Ivanov wrote:
>>> Vladimir K.,
>>>
>>> Thanks for the review!
>>>
>>>  > You don't need second region node, use one.
>>> Can you elaborate on this a little?
>>> Do you suggest to remove slow_region node? What should I use to point to
>>> slow path (for example, in generate_slow_guard(bol_thr, slow_call))?
>>
>> Never mind this. We can't remove slow_region because there are 2 input
>> paths.
>>
>>>
>>>  > If you rearrange code to generate slow call first and the load on
>>> fast path last you may not need to preserve init_mem.
>>
>> It looks like "fast" approach is much more complicated than I thought.
>> OK, go with "simple" case but move insert_mem_bar down (below enum)
>> where we can see it.
>>
>> Your comments for enum values are incorrect. !TLS... should be switched
>> like next:
>>
>>       no_int_result_path   = 1, // t == Thread.current() &&
>> !TLS._osthread._interrupted
>>       no_clear_result_path = 2, // t == Thread.current() &&
>> TLS._osthread._interrupted && !clear_int
>>
>> Thanks,
>> Vladimir K.
>>
>>> When considering moving slow path construction up, I was concerned about
>>> the case where there's no slow path:
>>>    if (stopped()) {
>>>      // There is no slow path.
>>>      result_rgn->init_req(slow_result_path, top());
>>>      result_val->init_req(slow_result_path, top());
>>>    } else {
>>>
>>> Is it worth retaining?
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 12/14/12 2:05 AM, Vladimir Kozlov wrote:
>>>> John, the barrier is before enum {} in simple fix.
>>>>
>>>> But I also prefer fast code.
>>>>
>>>> Vladimir I.,
>>>>
>>>> You don't need second region node, use one.
>>>>
>>>> If you rearrange code to generate slow call first and the load on fast
>>>> path last you may not need to preserve init_mem.
>>>>
>>>> Thanks,
>>>> Vladimir K.
>>>>
>>>> On 12/13/12 2:31 PM, John Rose wrote:
>>>>> On Dec 13, 2012, at 3:02 PM, Vladimir Ivanov wrote:
>>>>>
>>>>>> http://cr.openjdk.java.net/~vlivanov/8003135/simple/webrev.00
>>>>>> 31 lines changed: 16 ins; 6 del; 9 mod
>>>>>>
>>>>>> In the IR produced by Thread.isInterrupted(Z)Z intrinsic it's
>>>>>> possible to hoist the load of _interrupted flag out of the loop. The
>>>>>> fix is to add a barrier to forbid such optimization.
>>>>>>
>>>>>> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>>> 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
>>>>>