RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()
Patricio Chilano
patricio.chilano.mateo at oracle.com
Tue Aug 27 19:02:57 UTC 2019
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?
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