RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Tue Aug 27 16:04:24 UTC 2019

Hi Coleen,

On 8/27/19 11:34 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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.
Ok, I changed it. I removed the same comment from synchronizer.hpp since 
there is no point in having it twice.

I also fixed other comments that had mentions to slow_enter() or 
fast_enter() that didn't include in my original webrev.

Here is v3:

Inc: http://cr.openjdk.java.net/~pchilanomate/8229844/v3/inc/webrev/
Full: http://cr.openjdk.java.net/~pchilanomate/8229844/v3/webrev/

Thanks,
Patricio
> 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
>>>>
>>>
>>
>