RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Mon Feb 4 06:46:46 UTC 2019
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
those two cases? Although I don't agree we are losing information. The
information is the same, it's only presented different.
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