On Thu, 20 May 2021 11:42:06 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 seven additional commits since > the last revision: > > - Review fixes 2 > - Merge branch 'master' into 8265753 > - Review fixes > - Merge branch 'master' into 8265753 > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Removed manual transitions Updates mostly good but still a storestore issue to resolve. Thanks, David src/hotspot/share/prims/jvmtiRawMonitor.cpp line 379: > 377: ret = simple_wait(self, millis); > 378: > 379: // Now we need to re-enter the monitor. For JavaThread's My bad: no apostrophe in JavaThreads 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); Can't help but think the ppc/aarch64 folk have it right and that set_thread_state should always (?) have release semantics - and would then be renamed release_set_thread_state(). Also avoids a double barrier on ppc/aarch64. Followup RFE? But note that we need the storestore after all the make_walkable's that are followed by set_thread_state. ------------- Changes requested by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3875
