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

[Aleksey Shipilev]

BTW, the minimized test case is available as part of
java-concurrency-torture [1], which handles the stuck-on-failure running
mode. ...and it is Pavel's bidding to integrate those tests.

-Aleksey.

[1]
https://github.com/shipilev/java-concurrency-torture/blob/master/src/main/java/org/openjdk/concurrent/torture/tests/interrupt/CurrentThreadIsInterruptedMethodTest.java

On 12/14/2012 06:22 PM, Vladimir Ivanov wrote:
> Pavel,
> 
> I considered adding it as a regression, but there are some problems with
> the test case [*] and I don't see an easy way to fix them.
> 
> Otherwise, it'll produce intermittent failures in our automated test
> execution.
> 
> Best regards,
> Vladimir Ivanov
> 
> [*] - failure mode due to timeout, instead of explicit check
>     - thread coordination is based on timing
> 
> On 12/14/12 4:07 PM, Pavel Punegov wrote:
>> 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
>>