RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Mon Feb 4 21:45:53 UTC 2019
On 2/4/19 4:25 PM, Daniel D. Daugherty wrote:
> On 2/4/19 3:57 PM, 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
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
> No comments.
>
> src/hotspot/share/runtime/mutex.cpp
> No comments.
>
> src/hotspot/share/runtime/mutex.hpp
> No comments.
>
> src/hotspot/share/runtime/safepointMechanism.inline.hpp
> No comments.
>
> Thumbs up!
Thanks Dan!
Patricio
> Dan
>
>
>> 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