RFR: 8225351(13): assert failed: Revoke failed, unhandled biased lock state

[Robbin Ehn]

Hi Patricio,

On 2019-06-18 22:49, Patricio Chilano wrote:
> Hi Robbin,
> 
> Change looks good to me!

Thanks!

Below is an issue, thanks for noticing!
There are several of options here, still considering which one I like...

This fix will have to address that also.
I'll respond to RFR mail with whatever I do.

Thanks, Robbin

> 
> One question though. In deoptimization.cpp there is method 
> fetch_unroll_info_helper() which calls create_vframeArray() which will 
> eventually call vframeArrayElement::fill_in(). That method tries to get the 
> monitors from the stack and has the following assertion:
> 
> assert(monitor->owner() == NULL || (!monitor->owner()->is_unlocked() && 
> !monitor->owner()->has_bias_pattern()), "object must be null or locked, and 
> unbiased");
> 
> So it is expecting that the locked monitors are not biased anymore. Do you know 
> exactly when fetch_unroll_info_helper() would be called during this 
> deoptimization phase? There is a comment "fetch_unroll_info() is called at the 
> beginning of the deoptimization handler" but no sure what that means exactly 
> since it is expecting monitors to be revoked already.
> My question is whether we could hit that assert later on if we avoid trying to 
> revoke these biased objects that actually belong to somebody else. One scenario 
> could be as follows:
> 
> 1) JT1 calls BiasedLocking::revoke_and_rebias() for object X, finds X is biased 
> toward JT2 and requests a revocation using a bulk rebias operation. The 
> attempt_rebias flag is true, so this call comes from interpreter/c1/c2 where JT1 
> was trying to lock this object.
> 2) VMThread executes a deoptimization operation (present in the VM queue before 
> the bulk rebias operation was added). VMThread executes the handshake on behalf 
> of JT1, who is blocked, finds monitor associated with object X in the stack but 
> skips revocation since JT1 is not the owner. JT2 executes the handshake too
> but does not revoke bias of X because it is not currently using that lock (it's 
> not in its stack).
> 3) VMThread executes the bulk rebias operation for objects of class X that JT1 
> requested. Object  X is not currently in the stack of JT2 and so the bias for 
> object X expires and is not valid anymore. Since 
> bulk_revoke_or_rebias_at_safepoint() was called with a value of 
> attempt_rebias_of_object=true the object will be rebiased towards JT1.
> 4) JT1 unblocks, and tries to execute fetch_unroll_info_helper() (?)
> 
> But not sure if that last step is possible.
> 
> 
> Thanks Robbin!
> 
> 
> Patricio
> 
> On 6/14/19 4:03 AM, Robbin Ehn wrote:
>> Hi all, please review.
>>
>> When looking at a JavaThreads locks we retrieve them per frame via it's monitors
>> list. How this list actually populated differs from frame type. If a 
>> JavaThread tries to enter a new monitor a basic lock is directly/indirectly 
>> via e.g. scope info added to the virtual monitor list. If this lock is biased 
>> towards another
>> JavaThread we try to revoke that bias with a safepoint. In this case a deopt
>> handshake is already in queue. The handshake is thus executed before the revoke
>> safepoint.
>> The handshake goes over the monitors in compiled frames, find this lock and we
>> hit the assert. The assert make sure we actual can revoke the lock. A basic lock
>> on stack should always, if biased, be biased to current thread, with the
>> exception:
>> We may have a stack lock biased against another thread until
>> ObjectSynchronizer::fast_enter returns.
>>
>> To handle this exception we can safely ignore biased lock towards other threads
>> in the deopt handshake. Since such locks will always be revoked before we
>> deopt/unpack stack.
>>
>> Code:
>> http://cr.openjdk.java.net/~rehn/8225351/v1/webrev/index.html
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8225351
>>
>> Passes t1-7
>> The assert code tested with local code changes to HandshakeAlot handshake.
>> We then see this state where last lock can be biased towards another thread 
>> and the thread is trying to execute revoke safepoint.
>>
>> Thanks, Robbin
>