RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Mon Feb 4 19:40:50 UTC 2019
Hi Dan,
On 2/4/19 2:05 PM, Daniel D. Daugherty wrote:
> On 2/3/19 8:46 PM, Patricio Chilano wrote:
>> Hi all,
>>
>> Here is v05:
>>
>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v05/webrev/
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v05/inc/
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
> L295: // - When transitioning in (constructor), it checks for
> safepoints without blocking, i.e calls
> nit - s/i.e /i.e., /
> (You missed the change from my v04 review)
Done! I did made the change s/having to block/blocking from your review
but didn't see the extra comma you added after i.e.
> L299: class ThreadBlockinVMWithDeadlockCheck : public
> ThreadStateTransition {
> L312: ThreadBlockinVMWithDeadlockCheck(JavaThread *thread,
> Monitor** in_flight_monitor_adr)
> L326: ~ThreadBlockinVMWithDeadlockCheck() {
> s/ThreadBlockinVMWithDeadlockCheck/ThreadBlockInVMWithDeadlockCheck/
> Lower case 'i' -> 'I' to match the CamelCastStyleInOtherHelpers.
Done.
> src/hotspot/share/runtime/mutex.cpp
> L36: static inline void assert_owner(Thread *owner, Thread *thread) {
> nit - the predominant style in this file is ' * ' (space on
> each side of '*')
Ok, is that the style that we are supposed to follow in general ?
Otherwise I can change all the ones that do not match the preferred style.
> L38: "invalid owner: owner=" INTPTR_FORMAT ", should be="
> INTPTR_FORMAT,
> Another possible style of mesg:
> "invalid owner: actual=" INTPTR_FORMAT ", expect="
> INTPTR_FORMAT,
I like more the first style, but if you really prefer the second one I
can change it.
> ThreadBlockinVMWithDeadlockCheck renames to
> ThreadBlockInVMWithDeadlockCheck in
> this file also.
Done.
> src/hotspot/share/runtime/mutex.hpp
> No comments.
>
> src/hotspot/share/runtime/safepointMechanism.inline.hpp
> L83: if (!uses_thread_local_poll() || local_poll_armed(thread)) {
> Did you decide this line does not need a comment at all?
How about adding the following comment based on your previous email:
// If using thread local polls, we should not check the
// global_poll() and callback via block() if the VMThread
// has not yet armed the local poll. Otherwise, when used in
// combination with should_block(), the latter could miss
// detecting the same safepoint that this method would detect
// if only checking global polls.
Patricio
> Dan
>
>
>>
>> Tested tiers 1-6.
>>
>> Thanks,
>> Patricio
>>
>> On 2/1/19 7:58 PM, Daniel D. Daugherty wrote:
>>> On 2/1/19 7:49 PM, David Holmes wrote:
>>>> <trimming>
>>>>
>>>> On 2/02/2019 7:48 am, Patricio Chilano wrote:
>>>>> On 2/1/19 4:28 PM, coleen.phillimore at oracle.com wrote:
>>>>>>> In regards to the name ThreadLockBlockInVM ... I don't have a
>>>>>>> good suggestion.
>>>>>>> ThreadBlockInVMWithSafepointCheckingButOnlyBlockOnTheWayOut is
>>>>>>> rather unwieldy. ;-) But the "Lock" part really doesn't mean
>>>>>>> anything. So your suggestion of
>>>>>>> ThreadBlockinVMWithDeadlockPrevention seems a big improvement to
>>>>>>> me. :)
>>>>>>
>>>>>> How about ThreadBlockInVMForLock ? This answers the question
>>>>>> "why" this class, vs. "what" this class does. Since the latter
>>>>>> can change.
>>>>> I like that name. Is that name okay with you David?
>>>>
>>>> Sorry no. "Lock" still adds zero information about when/why you
>>>> would use this. It's used within Monitor::lock and Monitor::wait so
>>>> is not specific to "locking" even in that class. This is about
>>>> blocking in the VM and we need to know what this version does
>>>> differently to the existing TBIVM. There are lots of different
>>>> types of "locking" in the VM and we don't use this with them.
>>>>
>>>> Dan can be a tie-breaker on this bikeshed. Grab your brush Dan! ;-)
>>>
>>> ThreadBlockInVMBecauseSneakyLockingIsEvil?
>>>
>>> ThreadBlockinVMWithDeadlockPrevention is okay.
>>>
>>> ThreadBlockinVMWithDeadlockCheck
>>> ThreadBlockinVMWithDeadlockChk
>>> ThreadBlockinVMWithDeadlockCk (David won't like this one :-) )
>>>
>>> Just pick a name and run with it... :-)
>>>
>>> Dan
>>>
>>>
>>>>
>>>> David
>>>> -----
>>>>
>>>>
>>>>> Thanks,
>>>>> Patricio
>>>>>> thanks,
>>>>>> Coleen
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>> On 1/02/2019 3:05 pm, Patricio Chilano wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> On 1/31/19 12:54 AM, David Holmes wrote:
>>>>>>>>> On 31/01/2019 7:37 am, Patricio Chilano wrote:
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>> On 1/30/19 2:29 AM, David Holmes wrote:
>>>>>>>>>>> Hi Patricio,
>>>>>>>>>>>
>>>>>>>>>>> <trimming>
>>>>>>>>>>>
>>>>>>>>>>> First, thanks for all the many weeks of work you've put into
>>>>>>>>>>> this, pulling together a number of ideas from different
>>>>>>>>>>> people to make it all work!
>>>>>>>>>> Thanks! Credit to you for the PlatformMonitor implementation
>>>>>>>>>> : )
>>>>>>>>>
>>>>>>>>> :) Nothing innovative there.
>>>>>>>>>
>>>>>>>>>>> I've only got a few minor comments/suggestions.
>>>>>>>>>>>
>>>>>>>>>>> On 30/01/2019 10:24 am, Patricio Chilano wrote:
>>>>>>>>>>>> Full:
>>>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/webrev/
>>>>>>>>>>>> Inc:
>>>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/inc/webrev/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>>>>>>>>>
>>>>>>>>>>> I'm very unclear how ThreadLockBlockInVM differs from
>>>>>>>>>>> ThreadBlockInVM. You've duplicated a lot of complex code
>>>>>>>>>>> which is masking the actual difference between the two
>>>>>>>>>>> wrappers to me. It seems to me that an extra arg to
>>>>>>>>>>> transition_and_fence should allow you to handle the new
>>>>>>>>>>> behaviour without having to duplicate so much of this code.
>>>>>>>>>>> In any case the semantics of ThreadLockBlockInVM needs to be
>>>>>>>>>>> described.
>>>>>>>>>> I could do it with one extra argument, but I would need to
>>>>>>>>>> add two extra branches in transition_and_fence(), one to
>>>>>>>>>> decide if I'm in the Monitor case to avoid calling
>>>>>>>>>> SafepointMechanism::block_if_requested() directly and another
>>>>>>>>>> one to actually decide if I'm transitioning in or out, since
>>>>>>>>>> the actions to perform are different. I think it is easier to
>>>>>>>>>> read without adding new conditionals, and also we will save
>>>>>>>>>> those extra branches, but if you think it's better this way I
>>>>>>>>>> can change it.
>>>>>>>>>
>>>>>>>>> I would like something that tells me more clearly how this new
>>>>>>>>> transition helper differs from the existing TBIVM. Sharing the
>>>>>>>>> code between them and using different args would be one way.
>>>>>>>>> Documenting the difference in comments would be another. Your
>>>>>>>>> choice.
>>>>>>>> Ok, I added a description on top of TLBIVM.
>>>>>>>>
>>>>>>>>>>> Also I'm unclear what the "Lock" in ThreadLockBlockInVM
>>>>>>>>>>> actually refers to. I find the name quite jarring to read.
>>>>>>>>>> What about changing it to ThreadBlockinMonitor?
>>>>>>>>>
>>>>>>>>> That's not quite conveying the semantics. The problem is that
>>>>>>>>> the semantics we are changing compared to TBIVM are not
>>>>>>>>> evident in the TBIVM name. If TBIVM was actually
>>>>>>>>> ThreadBlockInVMWithSafepointBlocking, then this new transition
>>>>>>>>> would obviously be ThreadBlockInVMWithoutSafepointBlocking -
>>>>>>>>> but perhaps that lengthy, but clear name would be okay anyway?
>>>>>>>> Not convinced on that name since we are blocking at safepoints
>>>>>>>> in the destructor. Based on the comments I added how about
>>>>>>>> ThreadBlockinVMWithDeadlockPrevention ? or
>>>>>>>> ThreadBlockinVMWithPreemption? (as in eliminate one of the
>>>>>>>> conditions for deadlocks).
>>>>>>>>
>>>>>>>>>>> On the subject of naming, do_preempt and
>>>>>>>>>>> preempt_by_safepoint don't really convey to me what happens
>>>>>>>>>>> - what is being "preempted" here? I would suggest a more
>>>>>>>>>>> direct Monitor::release_for_safepoint
>>>>>>>>>> Changed.
>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Logging: why "nativemonitor"? The logging in mutex.cpp
>>>>>>>>>>> doesn't relate to a "native" monitor?? Actually I'm not even
>>>>>>>>>>> sure if we need bother at all with the one logging statement
>>>>>>>>>>> that is present.
>>>>>>>>>> I added it to eventually track unbounded try locks. Not sure
>>>>>>>>>> I follow you with the name, isn't that how we name this
>>>>>>>>>> monitors? I tried to differentiate them from Java monitors.
>>>>>>>>>> What about just "monitor"?
>>>>>>>>>
>>>>>>>>> How about vmmonitor ?
>>>>>>>> Ok, changed.
>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>>>>>>>
>>>>>>>>>>> void Monitor::lock_without_safepoint_check(Thread * self) {
>>>>>>>>>>> // Ensure that the Monitor does not require or allow
>>>>>>>>>>> safepoint checks.
>>>>>>>>>>>
>>>>>>>>>>> The comment there should only say "not require".
>>>>>>>>>> Done.
>>>>>>>>>>
>>>>>>>>>>> void Monitor::preempt_by_safepoint() {
>>>>>>>>>>> _lock.unlock();
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> Apart from renaming this as suggested above, aren't there
>>>>>>>>>>> any suitable assertions we should have here?
>>>>>>>>>>> safepoint-in-progress or handshake-in-progress? _owner ==
>>>>>>>>>>> Thread::current?
>>>>>>>>>> Ok, I added an assertion that owner should be NULL. Asserting
>>>>>>>>>> safepoint-in-progress does not really work because _state
>>>>>>>>>> could change to _not_synchronized right after you checked for
>>>>>>>>>> it in TLBIVM.
>>>>>>>>>
>>>>>>>>> Okay.
>>>>>>>>>
>>>>>>>>>>> Nit:
>>>>>>>>>>>
>>>>>>>>>>> assert(_owner == Thread::current(), "should be equal:
>>>>>>>>>>> owner=" INTPTR_FORMAT
>>>>>>>>>>> ", self=" INTPTR_FORMAT, p2i(_owner),
>>>>>>>>>>> p2i(Thread::current()));
>>>>>>>>>>>
>>>>>>>>>>> with Dan's enhanced assertions there's an indentation issue.
>>>>>>>>>>> The second line should indent to the first comma, but that
>>>>>>>>>>> will make the second line extend way past 80 columns.
>>>>>>>>>>>
>>>>>>>>>>> Also you could factor that assertion for
>>>>>>>>>>> _owner==Thread::current() into its own function or macro to
>>>>>>>>>>> avoid the repetition.
>>>>>>>>>> Corrected indentation based on Dan's reply to align with _owner.
>>>>>>>>>
>>>>>>>>> I though it should indent to the comma because it is a
>>>>>>>>> continuation of the same argument being passed to the assert
>>>>>>>>> "function". But I'm okay with Dan's suggestion.
>>>>>>>>>
>>>>>>>>> Factoring it into its own little function or macro would still
>>>>>>>>> be good to avoid the repetition.
>>>>>>>> Ok, added new function assert_owned_by_self(). I could change
>>>>>>>> it to assert_owner(Thread*) and use it for the NULL case too
>>>>>>>> unifying the printed messages to something like "invalid owner:
>>>>>>>> owner=" INTPTR_FORMAT ", should be:" INTPTR_FORMAT. What do you
>>>>>>>> think?
>>>>>>>>
>>>>>>>>>>> OSThreadWaitState osts(self->osthread(), false /* not
>>>>>>>>>>> Object.wait() */);
>>>>>>>>>>>
>>>>>>>>>>> This needs to be returned to its original place as per Dan's
>>>>>>>>>>> comments.
>>>>>>>>>> Done.
>>>>>>>>>>
>>>>>>>>>>> } else {
>>>>>>>>>>> Monitor::lock(self);
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> You don't need Monitor:: here
>>>>>>>>>> Removed.
>>>>>>>>>>
>>>>>>>>>>> // Temporary JVM_RawMonitor* support. A raw monitor can just
>>>>>>>>>>> be a PlatformMonitor now.
>>>>>>>>>>>
>>>>>>>>>>> This needs to be resolved before committing. Some of the
>>>>>>>>>>> existing commentary on what raw monitors are needs to be
>>>>>>>>>>> retained. Not clear if we need to set the _owner field or
>>>>>>>>>>> can just skip it.
>>>>>>>>>> Is it okay if I keep the following comments?
>>>>>>>>>>
>>>>>>>>>> // Yet another degenerate version of Monitor::lock() or
>>>>>>>>>> lock_without_safepoint_check()
>>>>>>>>>> // jvm_raw_lock() and _unlock() can be called by non-Java
>>>>>>>>>> threads via JVM_RawMonitorEnter.
>>>>>>>>>> //
>>>>>>>>>> // There's no expectation that JVM_RawMonitors will
>>>>>>>>>> interoperate properly with the native
>>>>>>>>>> // Mutex-Monitor constructs. We happen to implement
>>>>>>>>>> JVM_RawMonitors in terms of
>>>>>>>>>> // native Mutex-Monitors simply as a matter of convenience.
>>>>>>>>>
>>>>>>>>> Yep that's perfect. And as a future RFE we can replace them
>>>>>>>>> with direct use of PlatformMonitor (or PlatformMutex).
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I could keep setting the owner as _owner =
>>>>>>>>>> Thread::current_or_null() in jvm_raw_lock(), at least it
>>>>>>>>>> wouldn't hurt.
>>>>>>>>>
>>>>>>>>> It's useful for checking usage errors, but we won't have that
>>>>>>>>> if we replace with PlatformMonitor, so may as well drop it now
>>>>>>>>> IMO.
>>>>>>>> Ok, I added asserts that _owner should be NULL after acquiring
>>>>>>>> it and before releasing it though.
>>>>>>>>
>>>>>>>>>>> Monitor::~Monitor() {
>>>>>>>>>>> assert(_owner == NULL, "should be NULL: owner="
>>>>>>>>>>> INTPTR_FORMAT, p2i(_owner));
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> Will this automatically result in the PlatformMonitor
>>>>>>>>>>> destructor being called?
>>>>>>>>>> Yes, should I add a comment to make it clear that
>>>>>>>>>> ~PlatformMonitor() will be executed?
>>>>>>>>>
>>>>>>>>> No need - assume other people have a better understanding of
>>>>>>>>> C++ than I do :)
>>>>>>>>
>>>>>>>> Below is version v04, which also contains a correction pointed
>>>>>>>> out off-list by Robbin to do a local poll first in
>>>>>>>> SafepointMechanism::callback_if_safepoint() if we are using
>>>>>>>> local polls. Since the thread local poll is armed after
>>>>>>>> changing _state to _synchronizing, if we only do a global poll
>>>>>>>> in the TLBIVM constructor we could detect there is a safepoint
>>>>>>>> in progress and callback but when coming back in the destructor
>>>>>>>> SafepointMechanism::should_block() could miss detecting the
>>>>>>>> safepoint in progress since that method checks first for local
>>>>>>>> polls.
>>>>>>>>
>>>>>>>>
>>>>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/
>>>>>>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/inc/
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Patricio
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks for looking into this! Waiting on your comments to
>>>>>>>>>> send v04.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Patricio
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list