Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v13]
> This is the implementation of JEP 425: Virtual Threads (Preview). > > We will refresh this PR periodically to pick up changes and fixes from the > loom repo. > > Most of the new mechanisms in the HotSpot VM are disabled by default and > require running with `--enable-preview` to enable. > > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for > zero and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. > > There are changes in many areas. To reduce notifications/mails, the labels > have been trimmed down for now to hotspot, serviceability and core-libs. We > can add additional labels, if required, as the PR progresses. > > The changes include a refresh of java.util.concurrent and ForkJoinPool from > Doug Lea's CVS. These changes will probably be proposed and integrated in > advance of this PR. > > The changes include some non-exposed and low-level infrastructure to support > the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to > make life a bit easier and avoid having to separate VM changes and juggle > branches at this time. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - Refresh 6ace49bf42e5504c69fa11c564e8e865c0a95fb3 - Merge with 9425ab2b43b649bd591706bfc820e9c8795a6fdf - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5 - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec - Merge with jdk-19+20 - ... and 11 more: https://git.openjdk.java.net/jdk/compare/9425ab2b...6d968671 - Changes: https://git.openjdk.java.net/jdk/pull/8166/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8166&range=12 Stats: 99456 lines in 1130 files changed: 91188 ins; 3598 del; 4670 mod Patch: https://git.openjdk.java.net/jdk/pull/8166.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166 PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 13:42:12 GMT, Jaroslav Bachorik wrote: >> An option would be to place it after >> https://github.com/openjdk/jdk/blob/ce15582a7570b529a4c9b3d500f60fa0a2dc772d/src/hotspot/share/runtime/thread.hpp#L901 >> but it would make the code less coherent. > > Ok, moved it a bit around - now the bool field is after an int field which > should make things slightly better, I guess. At the start of JavaThread we have: private: bool _on_thread_list;// Is set when this JavaThread is added to the Threads list OopHandle _threadObj; // The Java level thread object so adding it next to the existing bool seems good. The accessors don't have to be defined at the same place. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v3]
On Thu, 5 May 2022 12:40:02 GMT, Jaroslav Bachorik wrote: >> src/hotspot/share/runtime/thread.hpp line 649: >> >>> 647: // support AGCT >>> 648: private: >>> 649: bool _in_agct; >> >> This should actually be in JavaThread as AGCT only operates on JavaThreads. > > I will have to do check/cast in `CodeCache::find_blob()` as that may get > called from any thread, not just Java threads. > I would assume that having this flag defined at Thread level is a lesser of > the evils - or am I wrong here? We have been actively moving JavaThread fields out of Thread. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8279358: vmTestbase/nsk/jvmti/scenarios/jni_interception/JI03/ji03t003/TestDescription.java fails with usage tracker [v2]
> The test counts calls of intercepted JNI functions, but doesn't completely > filter out calls from other threads. > Function isThreadExpected is used only for ExceptionOccurred function and the > function checks only some known JFR/Graal threads. > > The change: > - updates the test to count only the calls on the test thread; > - adds verbose output. Alex Menkov 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 two additional commits since the last revision: - Merge branch 'openjdk:master' into jni_intercept_test - Fix the test - Changes: - all: https://git.openjdk.java.net/jdk/pull/8475/files - new: https://git.openjdk.java.net/jdk/pull/8475/files/eb4626bb..0356c759 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8475&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8475&range=00-01 Stats: 22613 lines in 616 files changed: 14253 ins; 4094 del; 4266 mod Patch: https://git.openjdk.java.net/jdk/pull/8475.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8475/head:pull/8475 PR: https://git.openjdk.java.net/jdk/pull/8475
Integrated: 8284027: vmTestbase/nsk/jvmti/GetAllThreads/allthr001/ is failing
On Mon, 2 May 2022 23:20:52 GMT, Alex Menkov wrote: > The test counts all "system" threads before the execution and expects that > this number remains the same during test execution. > This makes the test fragile - JVM may start internal threads, some threads > may end. > > The fix updates the test: > - the test checks only test threads, and verify that the live threads are > reported by GetAllThreads and terminated threads are not reported; > - dropped "system" thread counting stuff; > - added proper deallocation of GetThreadInfo results. This pull request has now been integrated. Changeset: 1bba6407 Author:Alex Menkov URL: https://git.openjdk.java.net/jdk/commit/1bba64070e03ae3e047dc70dca75caeb49813908 Stats: 114 lines in 3 files changed: 48 ins; 42 del; 24 mod 8284027: vmTestbase/nsk/jvmti/GetAllThreads/allthr001/ is failing Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/8512
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
On Thu, 5 May 2022 17:43:58 GMT, Aleksey Shipilev wrote: > I am sorry to be a buzzkill here, but this integration would break lots of > platforms even when Loom functionality is not enabled/used. For example, > running `java -version` on RISC-V runs into many issues: > `TemplateInterpreterGenerator::generate_Continuation_doYield_entry` runs into > `Unimplemented()`, `UnsafeCopyMemory` asserts about UCM table size, > `NativeDeoptInstruction::is_deopt` would run into `Unimplemented()` while > being called from signal handler. > > This effectively blocks development on those platforms, and it seems to be > too much breakage for a preview feature. Are we really breaking these > platforms? Do we have plans in place with maintainers of those platforms to > fix all this in JDK 19 timeframe? I'd suggest holding off the integration > until such a plan and agreement is in place. We mailed porters-dev in Sep 2021 to give a heads up that this feature would require porting work so it shouldn't be a surprise. We have been open to including other ports with the initial integration but it was never a goal to have all the ports done in advance of that. Most of the new code in the VM is only used when running with --enable-preview. You'll see several places that test Continuations::enabled(). It should be possible to get these port running without -enable-preview without much effort. The feature has to be implemented to pass all the tests of course. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
On Thu, 5 May 2022 09:35:42 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 19 commits: > > - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f > - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5 > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - ... and 9 more: > https://git.openjdk.java.net/jdk/compare/1bb4de2e...86bea8ec I am sorry to be a buzzkill here, but this integration would break lots of platforms even when Loom functionality is not enabled/used. For example, running `java -version` on RISC-V runs into many issues: `TemplateInterpreterGenerator::generate_Continuation_doYield_entry` runs into `Unimplemented()`, `UnsafeCopyMemory` asserts about UCM table size, `NativeDeoptInstruction::is_deopt` would run into `Unimplemented()` while being called from signal handler. This effectively blocks development on those platforms, and it seems to be too much breakage for a preview feature. Are we really breaking these platforms? Do we have plans in place with maintainers of those platforms to fix all this in JDK 19 timeframe? I'd suggest holding off the integration until such a plan and agreement is in place. - Changes requested by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
On Thu, 5 May 2022 09:35:42 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 19 commits: > > - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f > - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5 > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - ... and 9 more: > https://git.openjdk.java.net/jdk/compare/1bb4de2e...86bea8ec Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v4]
On Thu, 5 May 2022 13:02:58 GMT, Jaroslav Bachorik wrote: >> A gist of the fix is to allow relaxed special handling of code blob lookup >> when done for ASGCT. >> >> Currently, a guarantee will fail when we happen to hit a zombie method which >> is still on stack. While this would indicate a serious error for the normal >> execution flow, in case of ASGCT being in progress when the executing thread >> can be expected at any possible method this is something which may happen >> and we really should not take the profiled JVM down due to it. >> >> >> Unfortunately, I am not able to create a simple reproducer for the crash >> other that testing in our production where the crash is happening >> sporadically. >> However, thanks to @parttimenerd and his [ASGCT stress >> test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be >> reproduced quite reliably. >> >> >> >> _Note: This is a followup PR for #8061_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > AGCT -> ASGCT > I would like to disagree: The abbreviation "ASGCT" is used in multiple places > in the JVM, > especially in `forte.cpp` (where "AGCT" cannot be found). That was a very old mistake on my part. It should have been `AGCT` instead of `ASGCT` and I never got around to fixing that. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
On Thu, 5 May 2022 09:35:42 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 19 commits: > > - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f > - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5 > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - ... and 9 more: > https://git.openjdk.java.net/jdk/compare/1bb4de2e...86bea8ec Studied the java.io package edits, the new JFR events and the new jcmd dump_to_file functionality. Looks good! - Marked as reviewed by coffeys (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 13:21:39 GMT, Johannes Bechberger wrote: >> I had tried identifying any gaps I might use but found none. Not saying they >> are none but it is rather difficult to spot anything with all the Thread >> related attributes spread across many lines, interspersed with the method >> declarations :( >> Any trick or tool I might use to find the right place for this attribute? > > An option would be to place it after > https://github.com/openjdk/jdk/blob/ce15582a7570b529a4c9b3d500f60fa0a2dc772d/src/hotspot/share/runtime/thread.hpp#L901 > but it would make the code less coherent. Ok, moved it a bit around - now the bool field is after an int field which should make things slightly better, I guess. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 12:55:31 GMT, Jaroslav Bachorik wrote: >> Just to clarify - you mean finding a gap due to padding and putting the >> field there? >> The rest of the fields are usually clustered around the supported >> functionality so before I insert this flag somewhere in the middle of >> unrelated fields I want to be sure this is what we want. > > I had tried identifying any gaps I might use but found none. Not saying they > are none but it is rather difficult to spot anything with all the Thread > related attributes spread across many lines, interspersed with the method > declarations :( > Any trick or tool I might use to find the right place for this attribute? An option would be to place it after https://github.com/openjdk/jdk/blob/ce15582a7570b529a4c9b3d500f60fa0a2dc772d/src/hotspot/share/runtime/thread.hpp#L901 but it would make the code less coherent. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 12:20:41 GMT, Jaroslav Bachorik wrote: >> src/hotspot/share/runtime/thread.hpp line 649: >> >>> 647: // support ASGCT >>> 648: private: >>> 649: bool _in_asgct; >> >> The position of this field may be significant. See if there are gaps in the >> Thread structure which this bool might be able to fill. > > Just to clarify - you mean finding a gap due to padding and putting the field > there? > The rest of the fields are usually clustered around the supported > functionality so before I insert this flag somewhere in the middle of > unrelated fields I want to be sure this is what we want. I had tried identifying any gaps I might use but found none. Not saying they are none but it is rather difficult to spot anything with all the Thread related attributes spread across many lines, interspersed with the method declarations :( Any trick or tool I might use to find the right place for this attribute? - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v4]
> A gist of the fix is to allow relaxed special handling of code blob lookup > when done for ASGCT. > > Currently, a guarantee will fail when we happen to hit a zombie method which > is still on stack. While this would indicate a serious error for the normal > execution flow, in case of ASGCT being in progress when the executing thread > can be expected at any possible method this is something which may happen and > we really should not take the profiled JVM down due to it. > > > Unfortunately, I am not able to create a simple reproducer for the crash > other that testing in our production where the crash is happening > sporadically. > However, thanks to @parttimenerd and his [ASGCT stress > test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be > reproduced quite reliably. > > > > _Note: This is a followup PR for #8061_ Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision: AGCT -> ASGCT - Changes: - all: https://git.openjdk.java.net/jdk/pull/8549/files - new: https://git.openjdk.java.net/jdk/pull/8549/files/71e5a919..caf43e39 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=02-03 Stats: 15 lines in 3 files changed: 4 ins; 8 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8549.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8549/head:pull/8549 PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v3]
On Thu, 5 May 2022 12:13:49 GMT, David Holmes wrote: >> Jaroslav Bachorik has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Naming and comments cleanup > > src/hotspot/share/runtime/thread.hpp line 649: > >> 647: // support AGCT >> 648: private: >> 649: bool _in_agct; > > This should actually be in JavaThread as AGCT only operates on JavaThreads. I will have to do check/cast in `CodeCache::find_blob()` as that may get called from any thread, not just Java threads. I would assume that having this flag defined at Thread level is a lesser of the evils - or am I wrong here? - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 12:00:49 GMT, David Holmes wrote: >> Jaroslav Bachorik has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Make sure the code blob result check is correct > > src/hotspot/share/runtime/thread.hpp line 649: > >> 647: // support ASGCT >> 648: private: >> 649: bool _in_asgct; > > The position of this field may be significant. See if there are gaps in the > Thread structure which this bool might be able to fill. Just to clarify - you mean finding a gap due to padding and putting the field there? The rest of the fields are usually clustered around the supported functionality so before I insert this flag somewhere in the middle of unrelated fields I want to be sure this is what we want. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 12:08:54 GMT, Johannes Bechberger wrote: >> I would like to disagree: The abbreviation "ASGCT" is used in multiple >> places in the JVM, especially in `forte.cpp` (where "AGCT" cannot be found). > > I found "AGCT" only in a method named > "Java_MyPackage_ASGCTBaseTest_checkAsyncGetCallTraceCall". My apologies. I've always referred to it by the obvious abbreviation. But if the existing code uses ASGCT then lets be consistent. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v3]
On Thu, 5 May 2022 12:13:18 GMT, Jaroslav Bachorik wrote: >> A gist of the fix is to allow relaxed special handling of code blob lookup >> when done for ASGCT. >> >> Currently, a guarantee will fail when we happen to hit a zombie method which >> is still on stack. While this would indicate a serious error for the normal >> execution flow, in case of ASGCT being in progress when the executing thread >> can be expected at any possible method this is something which may happen >> and we really should not take the profiled JVM down due to it. >> >> >> Unfortunately, I am not able to create a simple reproducer for the crash >> other that testing in our production where the crash is happening >> sporadically. >> However, thanks to @parttimenerd and his [ASGCT stress >> test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be >> reproduced quite reliably. >> >> >> >> _Note: This is a followup PR for #8061_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Naming and comments cleanup Changes requested by dholmes (Reviewer). src/hotspot/share/runtime/thread.hpp line 649: > 647: // support AGCT > 648: private: > 649: bool _in_agct; This should actually be in JavaThread as AGCT only operates on JavaThreads. - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 12:07:10 GMT, Johannes Bechberger wrote: >> src/hotspot/share/runtime/thread.hpp line 647: >> >>> 645: #endif // __APPLE__ && AARCH64 >>> 646: >>> 647: // support ASGCT >> >> Nit: the abbreviation for AsyncGetCallTrace is AGCT not ASGCT > > I would like to disagree: The abbreviation "ASGCT" is used in multiple places > in the JVM, especially in `forte.cpp` (where "AGCT" cannot be found). I found "AGCT" only in a method named "Java_MyPackage_ASGCTBaseTest_checkAsyncGetCallTraceCall". - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 11:59:16 GMT, David Holmes wrote: >> Jaroslav Bachorik has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Make sure the code blob result check is correct > > src/hotspot/share/runtime/thread.hpp line 647: > >> 645: #endif // __APPLE__ && AARCH64 >> 646: >> 647: // support ASGCT > > Nit: the abbreviation for AsyncGetCallTrace is AGCT not ASGCT I would like to disagree: The abbreviation "ASGCT" is used in multiple places in the JVM, especially in `forte.cpp` (where "AGCT" cannot be found). - PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v3]
> A gist of the fix is to allow relaxed special handling of code blob lookup > when done for ASGCT. > > Currently, a guarantee will fail when we happen to hit a zombie method which > is still on stack. While this would indicate a serious error for the normal > execution flow, in case of ASGCT being in progress when the executing thread > can be expected at any possible method this is something which may happen and > we really should not take the profiled JVM down due to it. > > > Unfortunately, I am not able to create a simple reproducer for the crash > other that testing in our production where the crash is happening > sporadically. > However, thanks to @parttimenerd and his [ASGCT stress > test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be > reproduced quite reliably. > > > > _Note: This is a followup PR for #8061_ Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision: Naming and comments cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/8549/files - new: https://git.openjdk.java.net/jdk/pull/8549/files/ce15582a..71e5a919 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=01-02 Stats: 8 lines in 3 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/8549.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8549/head:pull/8549 PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
On Thu, 5 May 2022 11:51:47 GMT, Jaroslav Bachorik
wrote:
>> A gist of the fix is to allow relaxed special handling of code blob lookup
>> when done for ASGCT.
>>
>> Currently, a guarantee will fail when we happen to hit a zombie method which
>> is still on stack. While this would indicate a serious error for the normal
>> execution flow, in case of ASGCT being in progress when the executing thread
>> can be expected at any possible method this is something which may happen
>> and we really should not take the profiled JVM down due to it.
>>
>>
>> Unfortunately, I am not able to create a simple reproducer for the crash
>> other that testing in our production where the crash is happening
>> sporadically.
>> However, thanks to @parttimenerd and his [ASGCT stress
>> test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be
>> reproduced quite reliably.
>>
>>
>>
>> _Note: This is a followup PR for #8061_
>
> Jaroslav Bachorik has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Make sure the code blob result check is correct
Changes requested by dholmes (Reviewer).
src/hotspot/share/code/codeCache.cpp line 657:
> 655: if (current_thread != NULL && current_thread->in_asgct()) {
> 656: // when in ASGCT handler things might get rough and not all
> guarantees are held
> 657: // if the resolved blob is already a zombie return NULL instead of
> crashing on guarantee
Suggestion:
// If called from ACGT the usual invariants may not apply so if we find
// a zombie method just return NULL.
src/hotspot/share/runtime/thread.hpp line 647:
> 645: #endif // __APPLE__ && AARCH64
> 646:
> 647: // support ASGCT
Nit: the abbreviation for AsyncGetCallTrace is AGCT not ASGCT
src/hotspot/share/runtime/thread.hpp line 649:
> 647: // support ASGCT
> 648: private:
> 649: bool _in_asgct;
The position of this field may be significant. See if there are gaps in the
Thread structure which this bool might be able to fill.
-
PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee [v2]
> A gist of the fix is to allow relaxed special handling of code blob lookup > when done for ASGCT. > > Currently, a guarantee will fail when we happen to hit a zombie method which > is still on stack. While this would indicate a serious error for the normal > execution flow, in case of ASGCT being in progress when the executing thread > can be expected at any possible method this is something which may happen and > we really should not take the profiled JVM down due to it. > > > Unfortunately, I am not able to create a simple reproducer for the crash > other that testing in our production where the crash is happening > sporadically. > However, thanks to @parttimenerd and his [ASGCT stress > test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be > reproduced quite reliably. > > > > _Note: This is a followup PR for #8061_ Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision: Make sure the code blob result check is correct - Changes: - all: https://git.openjdk.java.net/jdk/pull/8549/files - new: https://git.openjdk.java.net/jdk/pull/8549/files/2f009671..ce15582a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8549.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8549/head:pull/8549 PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee
On Thu, 5 May 2022 11:28:14 GMT, Jaroslav Bachorik wrote: > A gist of the fix is to allow relaxed special handling of code blob lookup > when done for ASGCT. > > Currently, a guarantee will fail when we happen to hit a zombie method which > is still on stack. While this would indicate a serious error for the normal > execution flow, in case of ASGCT being in progress when the executing thread > can be expected at any possible method this is something which may happen and > we really should not take the profiled JVM down due to it. > > > Unfortunately, I am not able to create a simple reproducer for the crash > other that testing in our production where the crash is happening > sporadically. > However, thanks to @parttimenerd and his [ASGCT stress > test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be > reproduced quite reliably. > > > > _Note: This is a followup PR for #8061_ src/hotspot/share/code/codeCache.cpp line 658: > 656: // when in ASGCT handler things might get rough and not all > guarantees are held > 657: // if the resolved blob is already a zombie return NULL instead of > crashing on guarantee > 658: return result != NULL || result->is_zombie() ? NULL : result; Could you add parentheses around the condition and possibly use `&&` instead of `||`? - PR: https://git.openjdk.java.net/jdk/pull/8549
RFR: 8283849: AsyncGetCallTrace may crash JVM on guarantee
A gist of the fix is to allow relaxed special handling of code blob lookup when done for ASGCT. Currently, a guarantee will fail when we happen to hit a zombie method which is still on stack. While this would indicate a serious error for the normal execution flow, in case of ASGCT being in progress when the executing thread can be expected at any possible method this is something which may happen and we really should not take the profiled JVM down due to it. Unfortunately, I am not able to create a simple reproducer for the crash other that testing in our production where the crash is happening sporadically. However, thanks to @parttimenerd and his [ASGCT stress test](https://github.com/parttimenerd/asgct2-tester.git) the problem can be reproduced quite reliably. _Note: This is a followup PR for #8061_ - Commit messages: - 8283849: AsyncGetCallTrace may crash JVM on guarantee Changes: https://git.openjdk.java.net/jdk/pull/8549/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8549&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283849 Stats: 18 lines in 4 files changed: 18 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8549.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8549/head:pull/8549 PR: https://git.openjdk.java.net/jdk/pull/8549
Re: RFR: 8285794: AsyncGetCallTrace might acquire a lock via JavaThread::thread_from_jni_environment [v5]
On Thu, 5 May 2022 10:29:07 GMT, Johannes Bechberger wrote: >> Calling JavaThread::thread_from_jni_environment for a terminated thread in >> AsyncGetCallTrace might cause the acquisition of a lock, making >> AsyncGetCallTrace non-signal-safe. >> >> AsyncGetCallTrace can only be called for the current threads (there are >> asserts for that), therefore using JavaThread::current directly and checking >> the termination status is semantically equivalent. > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Break long line Marked as reviewed by jbachorik (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8446
Re: RFR: 8285794: AsyncGetCallTrace might acquire a lock via JavaThread::thread_from_jni_environment [v5]
> Calling JavaThread::thread_from_jni_environment for a terminated thread in > AsyncGetCallTrace might cause the acquisition of a lock, making > AsyncGetCallTrace non-signal-safe. > > AsyncGetCallTrace can only be called for the current threads (there are > asserts for that), therefore using JavaThread::current directly and checking > the termination status is semantically equivalent. Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Break long line - Changes: - all: https://git.openjdk.java.net/jdk/pull/8446/files - new: https://git.openjdk.java.net/jdk/pull/8446/files/8d4b60eb..48f45e95 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8446&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8446&range=03-04 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8446.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8446/head:pull/8446 PR: https://git.openjdk.java.net/jdk/pull/8446
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v11]
On Wed, 4 May 2022 16:02:36 GMT, Aleksey Shipilev wrote: > So, does this PR pass current GHA checks? I see they are not enabled for this > PR. It would be unfortunate for this large integration to break builds/tests > for smaller PRs that would follow it. I've enabled it on my fork and we'll see if it kicks in. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285794: AsyncGetCallTrace might acquire a lock via JavaThread::thread_from_jni_environment [v4]
On Tue, 3 May 2022 08:02:57 GMT, Johannes Bechberger
wrote:
>> Calling JavaThread::thread_from_jni_environment for a terminated thread in
>> AsyncGetCallTrace might cause the acquisition of a lock, making
>> AsyncGetCallTrace non-signal-safe.
>>
>> AsyncGetCallTrace can only be called for the current threads (there are
>> asserts for that), therefore using JavaThread::current directly and checking
>> the termination status is semantically equivalent.
>
> Johannes Bechberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Merge cast into condition
LGTM.
src/hotspot/share/prims/forte.cpp line 571:
> 569: JavaThread* thread;
> 570:
> 571: if (trace->env_id == NULL || raw_thread == NULL ||
> !raw_thread->is_Java_thread() || (thread =
> JavaThread::cast(raw_thread))->is_exiting()) {
Line is a bit long! (There are other long lines in this file, too, so, I can
live with it.)
-
Marked as reviewed by mdoerr (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8446
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
> This is the implementation of JEP 425: Virtual Threads (Preview). > > We will refresh this PR periodically to pick up changes and fixes from the > loom repo. > > Most of the new mechanisms in the HotSpot VM are disabled by default and > require running with `--enable-preview` to enable. > > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for > zero and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. > > There are changes in many areas. To reduce notifications/mails, the labels > have been trimmed down for now to hotspot, serviceability and core-libs. We > can add additional labels, if required, as the PR progresses. > > The changes include a refresh of java.util.concurrent and ForkJoinPool from > Doug Lea's CVS. These changes will probably be proposed and integrated in > advance of this PR. > > The changes include some non-exposed and low-level infrastructure to support > the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to > make life a bit easier and avoid having to separate VM changes and juggle > branches at this time. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5 - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec - Merge with jdk-19+20 - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 - ... and 9 more: https://git.openjdk.java.net/jdk/compare/1bb4de2e...86bea8ec - Changes: https://git.openjdk.java.net/jdk/pull/8166/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8166&range=11 Stats: 99452 lines in 1130 files changed: 91184 ins; 3598 del; 4670 mod Patch: https://git.openjdk.java.net/jdk/pull/8166.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166 PR: https://git.openjdk.java.net/jdk/pull/8166
