RFR: 8265753: Remove manual JavaThread transitions to blocked [v3]

[David Holmes]

On Wed, 12 May 2021 08:04:47 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Please consider this change which removes the manual transitions to blocked.
>> This adds a preprocess template/functor which is executed in the destructor of 'ThreadBlockInVM' if we are going to do any processing.
>> This gives us a way to backout of the object/raw monitor before suspend or other processing, such as a safepoint.
>> 
>> The object monitor code could be straight forward changed to use this instead of manual transitions.
>> 
>> Raw monitors on the other hand are a bit more complicated due to 'implicit' rules (consequences of the specs).
>> Added a comment in hpp trying to explain it; we cannot simply transition with a raw monitor held.
>> This caused the change in the destructor ~ThreadInVMfromNative() (this specific change have also been tested in unrelated exploration of transition), now this RAII does the same as we do when going to native from Java, just setting the state.
>> Since we are going from an unsafe state, in VM, to a safe state, in native, we do not need to check the poll.
>> That made it possible to careful use ThreadInVMfromNative in raw monitors.
>> 
>> I also remove the early CAS in raw_enter.
>> We lock a lock to do a CAS, in the uncontended case means CAS on lock then CAS raw monitor.
>> Now we instead do a transitions, in the uncontended case means fence, CAS raw monitor, fence.
>> (multiple fence (CAS is also a fence) very close to each other have little additional performance impact on contemporary hardware)
>> 
>> Passes t1-t7 and manual stressing relevant test groups.
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixes for Dan

Hi Robbin,

Sorry for the delay in getting through this.

Overall approach looks good. I have a few queries below and some requested naming changes to make things clearer.

Thanks,
David

src/hotspot/share/prims/jvmtiRawMonitor.hpp line 48:

> 46: // The rules are:
> 47: // - We must never safepoint poll if raw monitor is owned.
> 48: // - We may safepoint poll before it is owned and after it has been released.

I'm not sure exactly what this is trying to say because user code can acquire a RawMonitor, then call into Java while holding the RawMonitor. That external use of RawMonitors should never cause any deadlock with the VMThread of course.

src/hotspot/share/prims/jvmtiRawMonitor.hpp line 96:

> 94:    protected:
> 95:     JvmtiRawMonitor* _rm;
> 96:     bool _rm_exit;

If this signifies the monitor exited then please name it `_rm_exited`.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 207:

> 205:     assert(_thread->thread_state() == _thread_in_vm, "coming from wrong thread state");
> 206:     assert(!_thread->has_last_Java_frame() || _thread->frame_anchor()->walkable(), "Unwalkable stack in vm->native transition");
> 207:     _thread->set_thread_state(_thread_in_native);

After doing a heavyweight transition for many many years I find it somewhat disconcerting to suddenly be told it is not necessary. If we are _thread_in_vm and so unsafe, then no handshake/safepoint can have been processed, so there is nothing to check. Makes sense. But how long has that been true? Could we have simplified this years ago or it is a result of other changes?

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 277:

> 275: class ThreadBlockInVM {
> 276:   InFlightMutexRelease _ifmr;
> 277:   ThreadBlockInVMPreprocess<InFlightMutexRelease> _tbivmpp;

Why delegate to the TBIVMPP instead of extending it?

src/hotspot/share/runtime/objectMonitor.cpp line 435:

> 433:         EnterI(current);
> 434:       }
> 435:       if (!eos.om_op_done()) {

I find this API too generic. I'd much rather see:

if (!eos.exited()) {
  assert ...
  break;
}

src/hotspot/share/runtime/objectMonitor.hpp line 309:

> 307:    protected:
> 308:     ObjectMonitor* _om;
> 309:     bool _om_op_done;

Please rename to _exited - we know what the "op" is so no need to use generic terminology.

src/hotspot/share/runtime/objectMonitor.hpp line 313:

> 311:     ExitOnSuspend(ObjectMonitor* om) : _om(om), _om_op_done(false) {}
> 312:     void operator()(JavaThread* current);
> 313:     bool om_op_done() { return _om_op_done; }

please rename to exited()

src/hotspot/share/runtime/objectMonitor.hpp line 315:

> 313:     bool om_op_done() { return _om_op_done; }
> 314:   };
> 315:   class ClearSuccOnSuspend : public ExitOnSuspend {

I don't see why there is any relationship between these two. You don't clear-succ and exit.

-------------

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3875