On Wed, 12 May 2021 08:04:47 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: > > Fixes for Dan src/hotspot/share/prims/jvmtiRawMonitor.cpp line 395: > 393: ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos); > 394: simple_enter(jt); > 395: _recursions = save; I think you should restore `_recursions` only when finally leaving the loop because if you do it here and exit the monitor again because of a suspend then another thread could hit the `guarantee(_recursions == 0, "invariant")` at L349. This is a preexisting issue but since you're modifying this I reckon it makes sense to correct it. ------------- PR: https://git.openjdk.java.net/jdk/pull/3875