RFR: 8210832: Remove sneaky locking in class Monitor
David Holmes
david.holmes at oracle.com
Mon Feb 4 04:42:34 UTC 2019
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.
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