RFR: 8210832: Remove sneaky locking in class Monitor
Robbin Ehn
robbin.ehn at oracle.com
Tue Feb 5 07:58:17 UTC 2019
On 2019-02-05 01:06, David Holmes wrote:
> v6 incremental looks good to me.
+1
/Robbin
>
> Thanks,
> David
> -----
>
> On 5/02/2019 6:57 am, Patricio Chilano wrote:
>> Hi Dan,
>>
>> Here is v06. It also contains David's suggested changes for assert_owner(),
>> and based on your correction about declaring pointers I fixed one in
>> ThreadBlockInVMWithDeadlockCheck declaration. Files mutex.cpp and mutex.hpp
>> have different styles of "pointer to" declarations so except the one you
>> mentioned I didn't try to fix them all since I don't know if changing all of
>> them to " * " is the right approach.
>>
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v06/inc/webrev
>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v06/webrev/
>>
>> Thanks,
>> Patricio
>>
>> On 2/4/19 2:47 PM, Daniel D. Daugherty wrote:
>>> On 2/4/19 2:40 PM, Patricio Chilano wrote:
>>>> 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.
>>>
>>> Extra period and extra comma... I'm a font/typography nut... :-)
>>>
>>>
>>>>
>>>>> 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.
>>>
>>> That's a good question. The general rule is to follow the
>>> predominant style in the file. However, I'm not sure what
>>> the HotSpot recommended style is (I used to know this...).
>>>
>>>
>>>>
>>>>> 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.
>>>
>>> I'm okay with what you have.
>>>
>>>
>>>>
>>>>> 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.
>>>
>>> I'm good with that comment.
>>>
>>> Dan
>>>
>>>
>>>>
>>>>
>>>> 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