RFR: 8210832: Remove sneaky locking in class Monitor

[Robbin Ehn]

Hi David,

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


The entire file is just a load of duplicated methods, e.g.
transition_and_fence/transition/transition_from_native with slight variations.
Not considering the ENTRY macros which we have zillion of, looking very much the 
same :)
IMHO it much easier to read code without all those branches.
I suspect it done this way to easier get it inlined. Already today 
transition_from_native is almost never inlined due to being to large (compared 
to the methods using it).

For the future my suggestion is to make native and block identical regarding
transition, that would help. But still suspend/async exception should only be 
delivered in certain transitions. Using templates for ThreadStateTransition so 
we can generate/eliminate code at compile time would be better regarding the 
duplication.

Essential just generate it from a 'table', where we just list state to state and 
what checks apply.

/Robbin

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