On Fri, 21 May 2021 09:41:10 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:
>
> Final fixes: last famous words
Thumbs up.
I just have some minor questions about a couple of the details.
Nothing blocking.
src/hotspot/share/prims/jvmtiRawMonitor.cpp line 405:
> 403:
> 404: guarantee(self == _owner, "invariant");
> 405: guarantee(save == _recursions, "invariant");
Since you just restored the `_recursions` field on L402, why do
you need this guarantee() 2 code lines later?
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 212:
> 210: _thread->frame_anchor()->make_walkable(_thread);
> 211: OrderAccess::storestore();
> 212: _thread->set_thread_state(_thread_in_native);
Please add a comment to the `storestore()` call:
// Keep thread_state change and make_walkable() separate.
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 251:
> 249: thread->frame_anchor()->make_walkable(thread);
> 250: OrderAccess::storestore();
> 251: thread->set_thread_state(_thread_blocked);
Please add a comment to the storestore() call:
// Keep thread_state change and make_walkable() separate.
src/hotspot/share/runtime/objectMonitor.cpp line 435:
> 433: ThreadBlockInVMPreprocess<ExitOnSuspend> tbivs(current, eos);
> 434: EnterI(current);
> 435: current->set_current_pending_monitor(NULL);
`current->set_current_pending_monitor(this)` is called above on L413
which is before the `for(;;)` loop so after this call to clear the pending
monitor, if we loop again, we will no longer have a pending monitor set.
That's a change in behavior from when the pending monitor was set to
NULL on old L435 below. I'm not sure why you move the code from old
L435 (outside the loop) to new L435 (inside the loop).
Update: I found this resolved comment below:
> And I set current->set_current_pending_monitor(om); to OM again
> in ExitOnSuspend if we exit the OM.
so that explains why we don't have a big change in behavior.
src/hotspot/share/runtime/objectMonitor.cpp line 451:
> 449: // We cleared the pending monitor info since we've just gotten past
> 450: // the enter-check-for-suspend dance and we now own the monitor free
> 451: // and clear, i.e., it is no longer pending.
This comment explains why the set-pending-to-NULL code was here
(outside the loop). You moved the code, but now the comment is
misplaced and the code no longer works the same.
Update: I found this resolved comment below:
And I set current->set_current_pending_monitor(om); to OM again
in ExitOnSuspend if we exit the OM.
so that explains why we don't have a big change in behavior.
However, I think you still need to do something about this comment.
There is still an observable change in behavior in that the current
pending monitor can cycle between set->NULL->set->NULL when
we run into a suspend request. Previously, it did not cycle.
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3875
