On Thu, 20 May 2021 21:51:31 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> 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 > > 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 Fixed > 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. Ah, you mean the pre-exiting missing of a storestore in TBIVM? Fixed. But we have more of those. Note that on TSO it's not needed, since both are volatile and will not be re-ordered by compiler. Yes, the platform which actually do care about avoiding release for performance have it (because it's needed). So yes it should always be there, good RFE! ------------- PR: https://git.openjdk.java.net/jdk/pull/3875