RFR: 8210832: Remove sneaky locking in class Monitor
David Holmes
david.holmes at oracle.com
Mon Feb 4 07:36:04 UTC 2019
On 4/02/2019 4:46 pm, Patricio Chilano wrote:
> Hi David,
>
> On 2/3/19 11:42 PM, David Holmes wrote:
>> Hi Patricio,
>>
>> I don't want to drag this out but we're losing some detail in the
>> assertion messages here:
>>
>> + static inline void assert_owner(Thread *owner, Thread *thread) {
>> + assert(owner == thread,
>> + "invalid owner: owner=" INTPTR_FORMAT ", should be="
>> INTPTR_FORMAT,
>> + p2i(owner), p2i(thread));
>> + }
>>
>> First does inline mean anything on a file static function?
>>
>> Second if this were actually a member function (yes I know I should
>> have realized this on previous feedback - my bad, sorry) then you
>> don't need to pass in the owner.
> No problem, I also liked it more being a member function actually. Is it
> okay then if I switch it back as it was in v04 and change the msg for
v04 has asserts plus assert_owned_by_self, I'd rather see one function
assert_owned_by(Thread* expected) but with that function giving clearer
information about the actual error.
> those two cases? Although I don't agree we are losing information. The
> information is the same, it's only presented different.
The information needs decoding if you only have two hex values to
compare, which is not as clear as the original assert messages, and
that's what I'd like to restore - but within the context of a single
helper function.
Thanks,
David
>
> Thanks,
> Patricio
>> Finally, I'd like to see clearer messages for the common cases of
>> unowned or owned-by-current-thread e.g. something like
>>
>> #ifdef DEBUG
>> const char* msg = "invalid owner";
>> if (thread == NULL) {
>> msg = "should be un-owned";
>> }
>> else if (thread == Thread::current()) {
>> msg = "should be owned by current thread";
>> }
>> assert(owner == thread, "%s: owner=" INTPTR_FORMAT ", should be="
>> INTPTR_FORMAT,
>> msg, p2i(owner), p2i(thread));
>> #endif
>>
>> Otherwise no further comments.
>>
>> Thanks,
>> David
>> -----
>>
>> On 4/02/2019 11:46 am, 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/
>>>
>>> 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