RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()

Tue Aug 27 16:56:39 UTC 2019

Sorry for being late to this review party! I've been a bit tied with
Async Monitor Deflation and gatekeeping lately...

 > Full: http://cr.openjdk.java.net/~pchilanomate/8229844/v2/webrev

src/hotspot/share/runtime/synchronizer.hpp
     Just keeping my head on straight here:
       - delete fast_enter() and fast_exit().
       - rename slow_enter() -> enter() slow_exit() -> exit().

src/hotspot/share/runtime/synchronizer.cpp
     Code motion makes this one difficult to review, but I don't see a
     way to make this easier:

     - appropriate parts of fast_enter() refactored to the beginning
       of new enter()
     - rest of new enter() matches old slow_enter()
     - fast_exit() just renamed to exit() (as I would have expected)

src/hotspot/share/runtime/biasedLocking.hpp
     L193:   static void revoke_at_safepoint(Handle obj);
         This function no longer has a comment that says who/what
         should be using it.

     So the changes here make it look like rebiasing as a concept is
     gone. I just reread the bug report... so you're not just removing
     the attempt_rebias parameter, you are removing the concept of
     rebiasing completely. No wait... bulk rebiasing still appears to
     be here... maybe you're just removing singleton rebiasing...
     Just trying to understand what you're doing here...

src/hotspot/share/runtime/biasedLocking.cpp
     With the removal of various return codes and status values, it's
     looking like the assumption is that bulk operations always work
     and don't need a return code or a status value anymore.

     I was originally wondering if we need a new comment somewhere,
     but it occurs to me that anyone reading the Biased Locking
     after this fix won't know that there used to be return codes
     and status values. Maybe no new comment is needed.

src/hotspot/share/interpreter/interpreterRuntime.cpp
     No comments.

src/hotspot/share/prims/jvmtiEnvBase.cpp
     No comments.

src/hotspot/share/runtime/deoptimization.cpp
     No comments.

src/hotspot/share/runtime/sharedRuntime.cpp
     No comments.

src/hotspot/share/c1/c1_Runtime1.cpp
     The old logic in "Runtime1::monitorenter()" is a mess to try
     and unwind to determine equivalence. I think you have it correct!

src/hotspot/share/jvmci/jvmciRuntime.cpp
     I find it interesting that this code doesn't have anything to
     make sure that this condition holds:

       assert(obj == lock->obj(), "must match");

     I wonder if there is code elsewhere in JVM/CI that makes sure
     that the BasicLock refers to the object... Obviously not your
     bug/issue... :-)

test/hotspot/gtest/oops/test_markOop.cpp
     I don't understand the test changes... I think David commented
     on those so I'll read the review thread now...

     Update: I've read Patricio's answer and now I understand. The
         test code after the change is making sure that the printing
         code does not report "is_biased" with a NULL locker. Got it.

Okay so I think I understand what you're taking out and that looks
good. What is not clear to me is what we are giving up here. I reread
the bug report again... Here's what I think:

     The singleton rebias code is used from a single place and that
     place will only succeed in a couple of very rare cases. Removing
     the singleton rebias code simplifies the code base by reducing
     the number of special case conditions.

Thumbs up!

I see that a V3 is out so I'll review the incremental there.

Dan

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
>