On Wed, 12 May 2021 08:04:47 GMT, Robbin Ehn <r...@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
