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

Daniel D.Daugherty dcubed at openjdk.java.net
Tue May 11 19:12:01 UTC 2021


On Tue, 11 May 2021 12:13:33 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 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


More information about the hotspot-runtime-dev mailing list