RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

[coleen.phillimore at oracle.com]

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.

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.

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.

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
>