RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

[Patricio Chilano]

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