RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Mon Aug 26 23:36:26 UTC 2019

Hi Coleen,

On 8/26/19 5:21 PM, coleen.phillimore at oracle.com wrote:
>
> http://cr.openjdk.java.net/~pchilanomate/8229844/v2/webrev/src/hotspot/share/runtime/synchronizer.hpp.udiff.html 
>
>
> + // The interpreter and compiler use assembly copies of these routines.
>
> Nit, please remove the extra space after 'use'.  But see below.
Fixed.

> http://cr.openjdk.java.net/~pchilanomate/8229844/v2/webrev/src/hotspot/share/runtime/synchronizer.cpp.frames.html 
>
>
> 260 // Monitor Enter/Exit
> 261 // The interpreter and compiler use some assembly copies of this 
> code. Make sure
> 262 // update those code if the following function is changed. The 
> implementation
> 263 // is extremely sensitive to race condition. Be careful.
>
>
> The interpreter and compiler assembly code aren't really copies of 
> this code.  From what I understand, the interpreter and compiler 
> assembly code attempt to lock the object using the bias to avoid a CAS 
> or a simple CAS if !UseBiasedLocking.  If it fails because the lock is 
> biased or locked by another thread, this runtime code is the slow path 
> that revokes the bias and/or inflates the monitor.
Yes, we try a couple of things before falling into this runtime code. If 
the JavaThread doesn't already own the lock, we try to bias it if 
possible, otherwise we try with stack locks (MacroAssembler::fast_lock() 
even attempts to CAS the _owner field in case of an inflated monitor).

> I don't think there should be a warning about updating the code in 
> both places because it should be obvious, and not because it's a copy. 
> Correct me if I'm wrong though.
For anyone changing this code I think it will probably be obvious that 
you might need to also update interpreter/compiler code. I guess we 
could remove the comment or maybe change it to be something like "Make 
sure interpreter and compiler assembly code remains in sync if this 
function is changed." What do you think?

Thanks for taking a look into this Coleen!

Patricio
> The change looks great to me!
> Coleen
>
>
> On 8/23/19 6:17 PM, Patricio Chilano wrote:
>> Hi David,
>>
>> On 8/22/19 11:18 PM, David Holmes wrote:
>>> Hi Patricio,
>>>
>>> On 23/08/2019 5:27 am, Patricio Chilano wrote:
>>>> Hi all,
>>>>
>>>> Please have a look at the following patch.
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8229844
>>>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8229844/v01/webrev/
>>>>
>>>> The attempt_rebias parameter is only used by 
>>>> ObjectSynchronizer::fast_enter() after we failed to acquire the 
>>>> lock in interpreter/compiler code. But even in that case the 
>>>> rebiasing will only work for the bulk rebiasing case, i.e. after a 
>>>> safepoint occurs, so not only this is not the common case but also 
>>>> there is nothing really fast about it. We can remove it without any 
>>>> real performance penalty and simplify the code. Also this allows to 
>>>> merge the fast_enter() and slow_enter() into a common enter() and 
>>>> remove biased locking knowledge in other parts of the code. Tested 
>>>> with tiers1-6 on Linux, Windows, OSX and Solaris.
>>>
>>> I really like the simplification and removing the biased locking 
>>> knowledge from external sites!
>>>
>>> I have one concern. We have this comment:
>>>
>>> // The interpreter and compiler use  assembly copies of these routines.
>>> // Please keep them synchronized.
>>>
>>> and you've made changes to these routines but not to anything in the 
>>> interpreter or compiler. So were they already out of sync or ??
>> I haven't found any "exact copy" of fast_enter() and slow_enter() in 
>> other places. I think this might be referring to code in 
>> *MacroAssembler::lock_object(...) or MacroAssembler::fast_lock(...) 
>> which tries to acquire the lock using the different techniques in 
>> order (biased locking, stack locks, full object monitors) similar to 
>> what we do in fast_enter()/slow_enter(). I would think that comment 
>> is there for cases where the overall synchronization logic changes, 
>> in which case we would have to update those interpreter/compiler 
>> versions.
>>
>>> src/hotspot/share/c1/c1_Runtime1.cpp
>>>
>>> 708   assert(obj == lock->obj(), "must match");
>>>
>>> It isn't at all obvious to me that this assert, which was previously 
>>> only applied to !UseBiasedLocking&&UseFastLocking is now always 
>>> valid. In particular I'd find it suspect is UseFastLocking** is 
>>> disabled.
>> Yes, I missed that one. I found that whether the _obj field was set 
>> or not actually only depends on UseFastLocking. If UseFastLocking is 
>> set then C1_MacroAssembler::lock_object() will be executed and that 
>> will set the _obj field for that BasicObjectLock.
>>
>> It goes the other way too, so when UseFastLocking is false the _obj 
>> field is not set. That's why I had to also bring back the 
>> lock->set_obj(obj); line when not using UseFastLocking otherwise I 
>> was hitting the assert in monitorexit "assert(oopDesc::is_oop(obj), 
>> "must be NULL or an object")". With the current code, running tests 
>> with -XX:-UseFastLocking works because that automatically disables 
>> flag UseBiasedLocking (arguments.cpp L4024-L4042) and forces the 
>> branch that has the lock->set_obj(obj) statement to be executed.
>>
>>> ** UseFastLocking must surely be a candidate for removal! :)
>> When working on the issue above I stumbled upon the following comment 
>> in arguments.cpp:
>>
>> // Turn off biased locking for locking debug mode flags,
>> // which are subtly different from each other but neither works with
>> // biased locking
>>
>> So seems this flag was meant to be used for debugging along with 
>> UseHeavyMonitors and JVMCIUseFastLocking. It might be useful to 
>> bypass compiler code when debugging but not sure how much it is used.
>>
>>> ---
>>>
>>> test/hotspot/gtest/oops/test_markOop.cpp
>>>
>>> Not sure the change here really makes sense. Previously the test was 
>>> testing the actions of fast_enter but now its just checking its own 
>>> previous setup. ??
>> That test was actually meant to exercise method markWord::print_on() 
>> and check the output for each possible state of the markword. The 
>> call to fast_enter() with the previous change of the epoch was just a 
>> hack to bias the lock.
>>
>> Here is v2:
>>
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8229844/v2/inc/webrev
>> Full: http://cr.openjdk.java.net/~pchilanomate/8229844/v2/webrev
>>
>> Running tests again.
>>
>> Thanks for taking a look into this David!
>>
>>
>> Patricio
>>> Thanks,
>>> David
>>> -----
>>>
>>>>
>>>> Thanks!
>>>> Patricio
>>
>