On Tue, 11 May 2021 12:13:33 GMT, Robbin Ehn <[email protected]> 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 with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains two additional commits since
> the last revision:
>
> - Merge branch 'master' into 8265753
> - Removed manual transitions
Thumbs up on the over all logic. I only have minor nits and suggestions.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 242:
> 240: if (self->is_Java_thread()) {
> 241: JavaThread* jt = self->as_Java_thread();
> 242: guarantee(jt->thread_state() == _thread_in_native, "invariant");
Thanks for making this a guarantee()!
There is an existing assert() about this in:
ThreadInVMfromNative -> trans() -> transition_from_native()
so it doesn't have to be there for forever.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 320:
> 318: // JavaThreads will enter here with state _thread_in_native.
> 319: void JvmtiRawMonitor::raw_enter(Thread* self) {
> 320: // TODO Atomic::load on _owner field
Why is this still a TODO?
src/hotspot/share/prims/jvmtiRawMonitor.hpp line 42:
> 40: // Important note:
> 41: // Raw monitors can be used in callbacks which happen during safepoint by
> the VM
> 42: // thread (e.g. heapRootCallback). This means we may not
> tranisition/safepoint
nit typo: s/e.g./e.g.,/
nit typo: s/tranisition/transition/
src/hotspot/share/prims/jvmtiRawMonitor.hpp line 43:
> 41: // Raw monitors can be used in callbacks which happen during safepoint by
> the VM
> 42: // thread (e.g. heapRootCallback). This means we may not
> tranisition/safepoint
> 43: // poll in many cases, else the agent JavaThread can deadlock with VM
> thread,
nit typo: s/VM thread/the VM thread/
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 have been
> released.
nit typo: s/have been/has been/
src/hotspot/share/prims/jvmtiRawMonitor.hpp line 52:
> 50: // native for all operations. However we need to honor suspend request,
> not
> 51: // entering a monitor if suspended, and check for interrupts. Honoring
> suspened
> 52: // and reading interrupt flag must be done from VM state (a safepoint
> unsafe
nit typo: s/suspend request/a suspend request/
nit typo: s/Honoring suspened/Honoring a suspend request/
nit typo: s/interrupt flag/the interrupt flag/
(You'll probably want to reformat the changed lines a bit.)
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 206:
> 204: ~ThreadInVMfromNative() {
> 205: assert(!_thread->has_last_Java_frame() ||
> _thread->frame_anchor()->walkable(), "Unwalkable stack in vm->native
> transition");
> 206: _thread->set_thread_state(_thread_in_native);
You're losing the assertion that we're transitioning from
`_thread_in_vm` to `_thread_in_native` here. Although,
the constructor did verify that we were in `_thread_in_native`
when we transitioned to `_thread_in_vm` so at least the
first half is still verified.
Can you an assertion that we're in `_thread_in_vm` here?
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 230:
> 228: };
> 229:
> 230: template <typename PRE_PROC>
When you mentioned doing this with templates, I was having
nightmares, but this one is not bad at all...
src/hotspot/share/runtime/objectMonitor.cpp line 313:
> 311: if (current->is_suspended()) {
> 312: _om->_recursions = 0;
> 313: _om->_succ = NULL;
Please add a comment after this line:
// Don't need a full fence after clearing successor here because of the call to
exit().
src/hotspot/share/runtime/objectMonitor.cpp line 325:
> 323: OrderAccess::fence(); // always do a full fence when successor is
> cleared
> 324: }
> 325: _om_exit = true;
Hmmm... `_om_exit` flag is misnamed here since you're "only" clearing the
successor.
Update: Now I see that the ClearSuccOnSuspend class is subclassed from
the ExitOnSuspend, but I still find that flag a bit confusing.
src/hotspot/share/runtime/objectMonitor.hpp line 309:
> 307: protected:
> 308: ObjectMonitor* _om;
> 309: bool _om_exit;
Instead of `_om_exit` maybe this should be more generic?
Perhaps `_om_op_done` or something like that?
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3875
