RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Aug 27 19:36:12 UTC 2019 

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