RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()
patricio.chilano.mateo at oracle.com
Fri Aug 23 22:17:46 UTC 2019
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.
> 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
// 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.
> 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:
Running tests again.
Thanks for taking a look into this David!
More information about the hotspot-runtime-dev