RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

[Patricio Chilano]

Thanks David!

Patricio
On 8/26/19 1:34 AM, David Holmes wrote:
> Hi Patricio,
>
> On 24/08/2019 8:17 am, 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.
>
> Okay - thanks for verifying.
>
>>> 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.
>
> Okay. The assert could have been done in the else part of the new if 
> statement as its redundant when you called set_obj directly. BUt its 
> okay where it is.
>
>>> ** 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.
>
> I'll ask the compiler folk what value these three flags have - maybe 
> we can cull them all. :) Separate RFE of course.
>
>>> ---
>>>
>>> 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.
>
> Ah I see. Okay that is fine.
>
>>
>> Here is v2:
>>
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8229844/v2/inc/webrev
>> Full: http://cr.openjdk.java.net/~pchilanomate/8229844/v2/webrev
>
> Changes look good.
>
> Thanks,
> David
> -----
>
>> Running tests again.
>>
>> Thanks for taking a look into this David!
>>
>>
>> Patricio
>>> Thanks,
>>> David
>>> -----
>>>
>>>>
>>>> Thanks!
>>>> Patricio
>>