Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Wed, 26 May 2021 14:45:07 GMT, Daniel D. Daugherty wrote: > Thumbs up. > > Reviewed the incremental between v05 -> v06. Looks good. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 07:06:58 GMT, Robbin Ehn 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 10 additional commits since > the last revision: > > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Final fixes: last famous words > - 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 Marked as reviewed by pchilanomate (Committer). - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 07:06:58 GMT, Robbin Ehn 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 10 additional commits since > the last revision: > > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Final fixes: last famous words > - 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 Thumbs up. Reviewed the incremental between v05 -> v06. Looks good. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 22:59:55 GMT, David Holmes wrote: > Nothing further from me. > > Thanks, > David Thank you David! - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 07:06:58 GMT, Robbin Ehn 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 10 additional commits since > the last revision: > > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Final fixes: last famous words > - 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 Nothing further from me. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 13:04:46 GMT, Richard Reingruber wrote: > Still looks good to me. > > Cheer's, Richard. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
On Tue, 25 May 2021 07:06:58 GMT, Robbin Ehn 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 10 additional commits since > the last revision: > > - Fixes for Dan > - Merge branch 'master' into 8265753 > - Final fixes: last famous words > - 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 Still looks good to me. Cheer's, Richard. - PR: https://git.openjdk.java.net/jdk/pull/3875
Re: RFR: 8265753: Remove manual JavaThread transitions to blocked [v7]
> 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 10 additional commits since the last revision: - Fixes for Dan - Merge branch 'master' into 8265753 - Final fixes: last famous words - 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3875/files - new: https://git.openjdk.java.net/jdk/pull/3875/files/cc15431e..30d62e85 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3875&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3875&range=05-06 Stats: 9596 lines in 1190 files changed: 4469 ins; 2824 del; 2303 mod Patch: https://git.openjdk.java.net/jdk/pull/3875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3875/head:pull/3875 PR: https://git.openjdk.java.net/jdk/pull/3875
