RFR: 8210832: Remove sneaky locking in class Monitor

[David Holmes]

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!

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.

Also I'm unclear what the "Lock" in ThreadLockBlockInVM actually refers 
to. I find the name quite jarring to read.

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

---

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.

---

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

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?

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.

  OSThreadWaitState osts(self->osthread(), false /* not Object.wait() */);

This needs to be returned to its original place as per Dan's comments.

     } else {
       Monitor::lock(self);
     }

You don't need Monitor:: here

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

Monitor::~Monitor() {
   assert(_owner == NULL, "should be NULL: owner=" INTPTR_FORMAT, 
p2i(_owner));
}

Will this automatically result in the PlatformMonitor destructor being 
called?

---

Thanks,
David
-----