Request for reviews (S): 7196242: vm/mlvm/indy/stress/java/loopsAndThreads crashed

[Christian Thalinger]

On Sep 10, 2012, at 4:10 PM, Vitaly Davidovich <vitalyd at gmail.com> wrote:

> // Writers must store all other values before f1. 266 // Readers must test f1 first for non-null before reading other fields. 267 // Competing writers must acquire exclusive access on the first 268 // write, to flags, using a compare/exchange. 269 // A losing writer to flags must spin until the winner writes f1, 270 // so that when he returns, he can use the linked cache entry.
> 
> OK maybe I'm just not seeing the spin loop or the cas in the webrev for cpCache.cpp ...

Ahh, you were talking about the comment in the code.  Sorry, I misunderstood.  I updated the comment.

-- Chris

> 
> I had assumed the lock impl would be fast when uncontended (one word CAS or similar) but I take your point.
> 
> Thanks
> 
> Sent from my phone
> 
> On Sep 10, 2012 7:02 PM, "Christian Thalinger" <christian.thalinger at oracle.com> wrote:
> 
> On Sep 10, 2012, at 3:53 PM, Vitaly Davidovich <vitalyd at gmail.com> wrote:
> 
> > Hi Chris,
> >
> > Perhaps the comment above where you now take out a lock should be updated since it's still talking about the CAS race?
> 
> No.  These are two different problems.
> 
> >
> > Also, curious why the CAS with immediate yield() was used before - seems like that doesn't actually buy much since a losing thread may then immediately lose CPU time and have to wait for scheduling to put it back on (I.e. a lock seems more efficient anyway here :)).
> 
> But that's only in a race.  When there is only one thread the current approach might be faster.  It depends on how fast the lock implementation is.
> 
> -- Chris
> 
> >
> > Vitaly
> >
> > Sent from my phone
> >
> > On Sep 10, 2012 5:30 PM, "Christian Thalinger" <christian.thalinger at oracle.com> wrote:
> > http://cr.openjdk.java.net/~twisti/7196242
> > 29 lines changed: 13 ins; 10 del; 6 mod; 2239 unchg
> >
> > 7196242: vm/mlvm/indy/stress/java/loopsAndThreads crashed
> > Reviewed-by:
> >
> > The current code in ConstantPoolCacheEntry::set_method_handle_common uses
> > a CAS to find out who's the winning thread in the race about linking an
> > invokehandle/invokedynamic call site.  The winning thread does the linking
> > while the other threads are waiting in a loop until the winning thread
> > finishes.  The waiting threads enter the Patching_lock and call os::yield
> > to give the winning thread more CPU time.
> >
> > Unfortunately the implementation of os::yield on Solaris uses the
> > Threads_lock and we hit this assert:
> >
> > #  fatal error: acquiring lock Threads_lock/15 out of order with lock Patching_lock/1 -- possible deadlock
> >
> > Even worse, the CAS might fail spuriously and we don't have a winning
> > thread because we don't loop around the CAS.  This may lead to hangs.
> >
> > src/share/vm/interpreter/interpreterRuntime.cpp
> > src/share/vm/oops/cpCache.cpp
> > src/share/vm/oops/cpCache.hpp
> >
>