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

[Pavel Punegov]

Vladimir,

it would be great if you add code from [1] as a regression test into 
repository

[1] http://cs.oswego.edu/pipermail/concurrency-interest/2012-
November/010184.html

Best regards, 
Pavel

On Friday 14 December 2012 15:08:22 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20121214/06fda16a/attachment-0001.html