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

[Pavel Punegov]

Yes, these tests will be integrated to our testing as new component soon.
So, we'll have the test case for this issue in regular testing.

Thanks,
Pavel

On 17:37:33, Aleksey Shipilev wrote:
> 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/ja
> va/org/openjdk/concurrent/torture/tests/interrupt/CurrentThreadIsInterrupted
> MethodTest.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