RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Mon Feb 4 01:46:30 UTC 2019
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