RFR 8229844: Remove attempt_rebias parameter from revoke_and_rebias()
Patricio Chilano
patricio.chilano.mateo at oracle.com
Tue Aug 27 18:59:40 UTC 2019
Hi Dan,
On 8/27/19 1:56 PM, Daniel D. Daugherty wrote:
> 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.
There are 3 callers of that method, two in synchronizer.cpp and one in
jvmtiEnvBase.cpp. All of them check first if we are at a safepoint, and
call that method if true otherwise they call the regular
BiasedLocking::revoke(). Since JavaThreads have to be stopped, this code
is either for the VMThread or for NonJavaThreads. We have an assertion
in single_revoke_at_safepoint() that the thread calling should be the
VMThread though. But not sure why VMThread would call
ObjectSynchronizer::enter() for instance.
> 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.
Right, I'm only removing the attempt to rebias in runtime code since we
already tried to do that and failed in interpreter/compiler. Once we
fall into runtime code rebiasing would work during a bulk rebiasing
operation or after a handshake failed, but those are not really "fast"
cases nor common. We are making the code more complex for no real gain.
> Thumbs up!
>
> I see that a V3 is out so I'll review the incremental there.
I'll reply that one with the incremental review.
Thanks for reviewing this Dan!
Patricio
> 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
>>
>
More information about the hotspot-runtime-dev
mailing list