RFR: 8210832: Remove sneaky locking in class Monitor

[Patricio Chilano]

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  : )

> 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.

> 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?

> 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"?

> ---
>
> 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.

> 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.

>  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.


I could keep setting the owner as _owner = Thread::current_or_null() in 
jvm_raw_lock(), at least it wouldn't hurt.

> 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?


Thanks for looking into this! Waiting on your comments to send v04.

Thanks,
Patricio
> ---
>
> Thanks,
> David
> -----
>