Removing intrinsic of Thread.isInterrupted()

[Yumin Qi]

When I am writing email I saw your email.

to answer Vladimir's question, I come up with a scenario:

When Thread is in process of interrupt call:

   OSThread* osthread = thread->osthread();
   osthread->set_interrupted(true);
   // More than one thread can get here with the same value of osthread,
   // resulting in multiple notifications.  We do, however, want the store
   // to interrupted() to be visible to other threads before we post
   // the interrupt event.
   OrderAccess::release();
   SetEvent(osthread->interrupt_event());


   Before SetEvent, bit is set.

   Now, with intrinsification, call

    Thread.currentThread().isInterrupted() will return return 'true' due 
to clear_int is 'false'.

     The following call
     Thread.isInterrupted() will  go to slow path, since the Event is 
not set, we got a 'false' with new fix.

     Karen's suggestion is get ride of second fastpath (t == 
Thread.current() && !clear_int) so it will be

    return  (t == Thread.current() && !TLS._osthread._interrupted) ? 
fast : slow

Thanks
Yumin
Thanks
Yumin



On 2/25/2014 11:56 AM, Karen Kinnear wrote:
> Vladimir,
>
> I updated the bug to reflect the code review email thread between Yumin, myself
> and David Holmes.
>
> To the best of my understanding there is a potential timing hole here, with the clearInterrupted
> case (study the webrev for the fix), i.e. in the isInterrupted(false) case.
>
> What I proposed was that we could keep the intrinsic quick check for isInterrupted bit, but not
> the logic for the isInterrupted(false) - unless you want to change it to add the Windows
> WaitForSingleObject call - which I assume removes the benefit of the intrinsic.
>
> Does that make sense to you?
> thanks,
> Karen
>
> On Feb 25, 2014, at 2:14 PM, Vladimir Kozlov wrote:
>
>> Yumin,
>>
>> On 2/24/14 5:46 PM, Yumin Qi wrote:
>>> Hi, Compiler team
>>>
>>>    I worked on this bug: 6498581:ThreadInterruptTest3 produces wrong
>>> output on Windows.  This is a problem thread sleep wakes up spuriously
>>> due to a race condition between os::interrupt and os::is_interruped.
>>> Detail please see bug comments.
>>>
>>>   https://bugs.openjdk.java.net/browse/JDK-6498581
>>>
>>>    The fix is (without removing intrinsic, but intend to remove it) :
>>>    http://cr.openjdk.java.net/~minqi/6498581/webrev00/
>>>
>>>    One problem is that Thread.isInterrupted() is intrinsic and there is
>>> chance that code like
>>>
>>>    boolean a = Thread.currentThread().isInterrupted();
>>>    boolean b = Thread.interrupted();
>>>
>>>    Will get different value. (fast/slow path)
>> How you come to this conclusion? You my be mistaken. We intrinsify native boolean isInterrupted(boolean ClearInterrupted) and not java method: isInterrupted().
>>
>> Also you are comparing different code which is nothing to do with intrinsic:
>>
>> isInterrupted() passes ClearInterrupted=false:
>>
>>     public boolean isInterrupted() {
>>         return isInterrupted(false);
>>     }
>>
>> when interrupted() passes 'true':
>>
>>     public static boolean interrupted() {
>>         return currentThread().isInterrupted(true);
>>     }
>>
>> Both method calls native isInterrupted(bool) which is intrinsified. So both calls intrinsic. There should be no difference.
>>
>>  From performance point of view, as Aleksey pointed, there is huge difference. We can't remove intrinsic.
>>
>> Thanks,
>> Vladimir
>>
>>>    I tried to remove the intrinsic code and done a test using following
>>> code. The result showed there is no difference by removing the intrinsic
>>> of Thread.isInterrupted().
>>>
>>> // test performance of removing Thread.isInterrupted() inlining
>>> public class TestThreadInterrupted {
>>>      public static void main(String... args) {
>>>          Thread t = new Thread () {
>>>              public void run() {
>>>                  boolean isInt = false;
>>>                  while (!isInt) {
>>>                      try {
>>>                          Thread.sleep(30);
>>>                      } catch (InterruptedException ie) {
>>>                          isInt = true;
>>>                      }
>>>                  }
>>>              }
>>>          };
>>>
>>>          t.start();
>>>          // run
>>>          long start, finish, isum = 0L, osum = 0L;
>>>          int  NUM = 20000;
>>>          for (int j = 0; j < 100; j++) {
>>>          isum = 0L;
>>>          for (int i = 0; i < NUM; i++) {
>>>              start = System.currentTimeMillis();
>>>              t.isInterrupted();
>>>              finish = System.currentTimeMillis();
>>>              isum += (finish - start);
>>>          }
>>>
>>>          System.out.println("Total cost of " + NUM + " calls is " + isum
>>> + " ms");
>>>          osum += isum;
>>>          }
>>>          System.out.println("Average " + osum/100 + " ms");
>>>          t.interrupt();
>>>          try {
>>>              t.join();
>>>          } catch (InterruptedException e) {}
>>>      }
>>> }
>>>
>>> And found there is no difference on Solaris-x64/sparcv9, Windows(32/64),
>>> linux(32/64) before and after the removing of intrinsic
>>> Thread.isInterrupted().
>>>
>>> Should I remove the intrinsic?
>>>
>>> Data (no major difference  for both with/without intrinsic):
>>>
>>> 1)windows :
>>> ....
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 1 ms
>>> Total cost of 20000 calls is 1 ms
>>> Total cost of 20000 calls is 1 ms
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 1 ms
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 0 ms
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 2 ms
>>> Average 1 ms
>>>
>>> 2) Solaris-x64
>>> ....
>>> Total cost of 20000 calls is 3 ms
>>> Total cost of 20000 calls is 1 ms
>>> Total cost of 20000 calls is 4 ms
>>> Total cost of 20000 calls is 6 ms
>>> Total cost of 20000 calls is 6 ms
>>> Total cost of 20000 calls is 5 ms
>>> Total cost of 20000 calls is 7 ms
>>> Total cost of 20000 calls is 5 ms
>>> Total cost of 20000 calls is 5 ms
>>> Total cost of 20000 calls is 1 ms
>>> Total cost of 20000 calls is 3 ms
>>> Total cost of 20000 calls is 2 ms
>>> Total cost of 20000 calls is 3 ms
>>> Total cost of 20000 calls is 3 ms
>>> Total cost of 20000 calls is 5 ms
>>> Total cost of 20000 calls is 4 ms
>>> Total cost of 20000 calls is 4 ms
>>> Total cost of 20000 calls is 7 ms
>>> Average 4 ms
>>>
>>> 3) Linux:
>>>
>>> ....
>>> Total cost of 20000 calls is 30 ms
>>> Total cost of 20000 calls is 29 ms
>>> Total cost of 20000 calls is 26 ms
>>> Total cost of 20000 calls is 26 ms
>>> Total cost of 20000 calls is 26 ms
>>> Total cost of 20000 calls is 24 ms
>>> Total cost of 20000 calls is 29 ms
>>> Total cost of 20000 calls is 25 ms
>>> Total cost of 20000 calls is 20 ms
>>> Average 24 ms
>>>
>>>
>>> Thanks
>>> Yumin