RFR: 8210832: Remove sneaky locking in class Monitor
David Holmes
david.holmes at oracle.com
Tue Feb 5 00:06:29 UTC 2019
v6 incremental looks good to me.
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