RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Patricio Chilano patricio.chilano.mateo at oracle.com
Tue Aug 27 19:38:13 UTC 2019 

Great, thanks Dan!

Patricio

On 8/27/19 4:36 PM, Daniel D. Daugherty wrote:
> On 8/27/19 3:02 PM, Patricio Chilano wrote:
>>
>>
>> On 8/27/19 2:05 PM, Daniel D. Daugherty wrote:
>>> On 8/27/19 12:04 PM, Patricio Chilano wrote:
>>>> 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/
>>>
>>> src/hotspot/cpu/aarch64/aarch64.ad
>>>     No comments.
>>>
>>> src/hotspot/cpu/sparc/macroAssembler_sparc.cpp
>>>     No comments.
>>>
>>> src/hotspot/cpu/x86/macroAssembler_x86.cpp
>>>     No comments.
>>>
>>> src/hotspot/share/runtime/synchronizer.cpp
>>>     Nice update!
>>>
>>> src/hotspot/share/runtime/synchronizer.hpp
>>>     L68:   // This is full version of monitor enter and exit.
>>>         Perhaps:
>>>            // This is the "slow path" version of monitor enter and 
>>> exit.
>> Fixed! Do you need to see a v4?
>
> Ooops... Forgot to say that I don't need to see a new webrev
> (for either of my reviews).
>
> Dan
>
>>
>> Thanks!
>>
>> Patricio
>>> Thumbs up!
>>>
>>> Dan
>>>
>>>
>>>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the hotspot-runtime-dev mailing list