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

[David Holmes]

On Wed, 19 May 2021 07:26:14 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 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