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 > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [hotspot-runtime-dev](mailto:hotspot-runtime-...@mail.openjdk.java.net):_ > > On 20/05/2021 5:10 pm, Robbin Ehn wrote: > > > On Wed, 19 May 2021 14:25:41 GMT, Richard Reingruber <rrich at openjdk.org> > > wrote: > > > Just one more, rather unimportant comment... > > > Either way: LGTM! > > > Thanks, Richard. > > > > > > Thanks! > > > src/hotspot/share/prims/jvmtiRawMonitor.cpp line 382: > > > > 380: > > > > 381: _recursions = 0; > > > > 382: ret = simple_wait(self, millis); > > > > > > > > > IMHO the guarantee at L379 is redundant with the newly added identical > > > guarantee in `JvmtiRawMonitor::simple_wait()` at L240. > > > In case you agree to remove the guarantee, I don't see why the following > > > statements cannot be pulled out of the if-statement. > > > ``` > > > _recursions = 0; > > > ret = simple_wait(self, millis); > > > _recursions = save; > > > ``` > > > > > > Yes, it can be re-written as something below, but is this more readable? > > I'd say yes with two minor modifications: > > > _recursions = 0; > > ret = simple_wait(self, millis); > > + // Now we need to re-enter the monitor. For JavaThread's > + // we need to manage suspend requests. > > > if (self->is_Java_thread()) { // JavaThread re-enter > > JavaThread* jt = self->as_Java_thread(); > > { > > I think this extra block scope can also go. > > Cheers, > David > ----- Fixed, thanks, Robbin ------------- PR: https://git.openjdk.java.net/jdk/pull/3875
