RFR: 8210832: Remove sneaky locking in class Monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 4 19:47:21 UTC 2019
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