RFR: 8210832: Remove sneaky locking in class Monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 4 19:05:31 UTC 2019
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)
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.
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 '*')
L38: "invalid owner: owner=" INTPTR_FORMAT ", should be="
INTPTR_FORMAT,
Another possible style of mesg:
"invalid owner: actual=" INTPTR_FORMAT ", expect="
INTPTR_FORMAT,
ThreadBlockinVMWithDeadlockCheck renames to
ThreadBlockInVMWithDeadlockCheck in
this file also.
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?
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