RFR(S): 7037756 Deadlock in compiler thread similiar to 6789220

[Tom Rodriguez]

I'm not against a hybrid of your original fix and the current one.  Use the owns_pending_list_lock check instead of the is_reference_handler_check and keep the rest.  The try/lock logic itself it fairly ugly though.  Maybe it's just the name LockerMutexLocker that seems wrong.  Maybe LockedMutexUnlocker or a variant constructor for MutexLocker like this:

  MutexLocker(Monitor * mutex, bool already_locked) {
    assert(mutex->rank() != Mutex::special,
      "Special ranked mutex should only use MutexLockerEx");
    _mutex = mutex;
    if (already_locked) {
      assert(mutex->owned_by_self(), "must already be locked");
    } else {
      _mutex->lock();
    }
  }

Then the try_lock piece just sets a flag that we pass into that and otherwise the locking proceeds as normal.

tom

On Apr 26, 2011, at 10:55 AM, John Cuthbertson wrote:

> Hi Tom, Rmki,
> 
> I think that the main loop might still get compiled (some of the time) even with this fix. Here's the code of the run method:
> 
>         public void run() {
>             for (;;) {
> 
>                 Reference r;
>                 synchronized (lock) {
>                     if (pending != null) {
>                         r = pending;
>                         Reference rn = r.next;
>                         pending = (rn == r) ? null : rn;
>                         r.next = r;
>                     } else {
>                         try {
>                             lock.wait();
>                         } catch (InterruptedException x) { }
>                         continue;
>                     }
>                 }
> 
>                 // Fast path for cleaners
>                 if (r instanceof Cleaner) {
>                     ((Cleaner)r).clean();
>                     continue;
>                 }
> 
>                 ReferenceQueue q = r.queue;
>                 if (q != ReferenceQueue.NULL) q.enqueue(r);
>             }
>         }
> 
> With Xcomp, the reference handler thread is not holding the lock when run() is called and so the compilation request would be successful. Normally though the compilation of run() would be an OSR compilation - it looks like there are 3 backward branches and only one of which is inside the the locked region. If we have a lot of references to enqueue then I would expect that the backward branch after the enqueue() call would trigger an OSR compile (I don't think the branch inside the locked region would be executed any more frequently).
> 
> The actual method I saw being compiled while holding the pending list lock was: java/lang/ref/Reference.access$202:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
> 
> Other methods invoked from the locked region are:
> 
> java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
> java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;
> java/lang/Object.wait:()V
> 
> Of these java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock; is also invoked outside of the locked region (it's actually at bci:0 - which is the target bci of the backward branches), and java/lang/Object.wait:()V is a native method.
> 
> 
> On 04/25/11 19:58, Tom Rodriguez wrote:
>> 
>> We could have a small side queue that we could enqueue these on and when a later safe request comes in we can request a non blocking compile of them.  It's kind of gross but it would work I think.  It could even just be a queue of length one instead of a full data structure.
>> 
>> tom
>> 
>>   
>> 
> I think having a special queue for these is a lot uglier than adapting the original fix (i.e. the one with the try_lock) to use ownership of the pending list lock rather than the thread id.
> 
> JohnC
>