On Wed, 19 May 2021 07:26:14 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 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 five additional commits since
> the last revision:
>
> - Review fixes
> - Merge branch 'master' into 8265753
> - Fixes for Dan
> - Merge branch 'master' into 8265753
> - Removed manual transitions
Hi Robbin,
Overall this looks good to me, but there is one issue that needs fixing
(partially pre-existing but now also affecting ObjectMonitor::enter).
Other minor comments below.
Thanks,
David
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 236:
> 234:
> 235: template <typename PRE_PROC>
> 236: class ThreadBlockInVMPreprocess : public ThreadStateTransition {
Can we add a comment before this template definition please:
// Perform a transition to _thread_blocked and take a call-back to be executed
before
// SafepointMechanism::process_if_requested when returning to the VM. This
allows us
// to perform an "undo" action if we might block processing a
safepoint/handshake operation
// (such as thread suspension).
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 245:
> 243: // Once we are blocked vm expects stack to be walkable
> 244: thread->frame_anchor()->make_walkable(thread);
> 245: thread->set_thread_state(_thread_blocked);
This is a pre-existing issue. Everywhere we call make_walkable and then call
plain set_thread_state (other than on PPC/Aarch64 which do a release_store) we
are at risk of the thread_state update being reordered with stores related to
making the stack walkable. Potentially allowing the VMThread (or other thread)
to walk a stack that is not yet walkable! The original ObjectMonitor::enter
code was aware of this:
`current->frame_anchor()->make_walkable(current);`
`// Thread must be walkable before it is blocked.`
`// Read in reverse order.`
`OrderAccess::storestore();`
`for (;;) {`
` current->set_thread_state(_thread_blocked);`
src/hotspot/share/runtime/objectMonitor.cpp line 448:
> 446: // Completed the tranisition.
> 447: SafepointMechanism::process_if_requested(current);
> 448: current->set_thread_state(_thread_in_vm);
The comment block above this code is no longer accurate as there is no longer
an opportunity to go to a safepoint at the end of the block. I'm not sure what
a thread dump would show with the new code.
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3875
