RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Aug 27 17:05:28 UTC 2019 

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.

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