RFR: 8210832: Remove sneaky locking in class Monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Feb 1 16:10:09 UTC 2019
> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/inc/
src/hotspot/share/logging/logTag.hpp
No comments.
src/hotspot/share/runtime/interfaceSupport.inline.hpp
L295: // - When transitioning in (constructor), it checks for
safepoints without having to block, i.e calls
Perhaps:
// - When transitioning in (constructor), it checks for
safepoints without blocking, i.e., calls
L298: // the monitor that is in the process of being acquired
(only setting the owner is missing).
Perhaps:
// the monitor that is only partially acquired.
src/hotspot/share/runtime/mutex.cpp
L59: log_trace(vmmonitor)("JavaThread " INTPTR_FORMAT " on %d
attempt trying to acquire native monitor %s", p2i(self), retry_cnt, _name);
nit - s/native monitor/vmmonitor/ - for consistency.
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)) {
L84: if (global_poll()) {
I'm having trouble with this construct:
- if uses_thread_local_poll() is false, then we check global_poll()
and call SafepointSynchronize::block() if global_poll() == true.
That makes sense since we're not using local polling.
- if uses_thread_local_poll() is true, then we check
if local_poll_armed(thread) == true and then we check
global_poll()
and call SafepointSynchronize::block() if global_poll() == true.
Huh?
So we only call SafepointSynchronize::block() if global_poll()
== true
no matter what. It seems like the checks for local polling
don't really
do anything here.
What am I missing?
Dan
On 2/1/19 12:05 AM, 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