RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Tue Aug 27 14:34:39 UTC 2019

On 8/26/19 7:36 PM, Patricio Chilano wrote:
> 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.
>>

How about this slight variation:

261 // The interpreter and compiler assembly code tries to lock using 
the fast path of this algorithm.
        // Make sure to update that code if the following function is 
changed. The implementation
263 // is extremely sensitive to race condition. Be careful.

thanks,
Coleen
>>
>> 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
>>>
>>
>