RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Mon Feb 4 20:57:48 UTC 2019
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