RFR: 8210832: Remove sneaky locking in class Monitor
David Holmes
david.holmes at oracle.com
Fri Feb 1 05:34:44 UTC 2019
Hi Patricio,
Comments on inc v4:
src/hotspot/share/runtime/safepointMechanism.inline.hpp
+ if(!uses_thread_local_poll() || local_poll_armed(thread)) {
Need space after if.
I trust Robbin's judgement that this is the right way to express this,
but if ordering is so critical it should be documented.
---
src/hotspot/share/runtime/mutex.hpp/.cpp
assert_owned_by_self doesn't need to be a member function, it can just
be a file static in the .cpp file. No need to clutter the API.
Using a version that takes the owner thread is a good idea for checking
the NULL case too.
---
src/hotspot/share/runtime/interfaceSupport.inline.hpp
I like the comment block - thanks! A couple of minor things:
+ // are trying to acquire. This will be used to access and release the
monitor in case needed to avoid
s/in case/if/
+ // the monitor that is in the process of being acquired (only
setting the owner is missing).
I think the part in parentheses can be dropped - it reads like you
should be setting the owner but have chosen not to for some reason.
+ // The logic of this class could have been integrated into a more
general ThreadBlockInVM. By using a
+ // separate class though, we avoid executing branches that would
otherwise be needed to identify each
+ // case.
Not necessary IMHO but up to you whether to keep or drop.
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. :)
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