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

[Vladimir Ivanov]

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
>