Re: RFR: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS [v3]
On Mon, 4 Dec 2023 23:47:12 GMT, David Holmes wrote: >> Please review this simple clarification to the JVM TI spec regarding use of >> `JAVA_TOOL_OPTIONS` in regards to module options and their format. >> >> I do not believe this clarification needs a CSR request. >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > reviewer feedback Thanks, this is very clear now. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16896#pullrequestreview-1764102034
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
On Tue, 5 Dec 2023 00:23:45 GMT, Serguei Spitsyn wrote:
> The fix is for a regression caused by:
> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI
> ClassLoad event slows down vthread creation by factor 10
>
> The fix of 8308614 just triggered a known issue:
> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events
> are not always posted with -Xcomp option
>
> The fix is just a work around with the extra checks with the
> `JvmtiExport::should_post_field_access()` and
> `JvmtiExport::should_post_field_modification()`.
>
> Testing:
> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore
> with this fix
> - In progress: Test with tiers 1-6
src/hotspot/share/prims/jvmtiThreadState.cpp line 562:
> 560: if (JvmtiThreadState::seen_interp_only_mode() ||
> 561: JvmtiExport::should_post_field_access() ||
> 562: JvmtiExport::should_post_field_modification()){
The comment needs updating to explain the extra checks.
Can't say I see the connection with
[8316283](https://bugs.openjdk.org/browse/JDK-8316283) as no `-Xcomp` is
involved in the current failures AFAICS.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1414867564
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On Tue, 5 Dec 2023 01:20:28 GMT, Alex Menkov wrote: > VM_HeapDumper caches stack traces for platform/carrier and mounted virtual > threads only. > For unmounted virtual threads ThreadDumper objects are created on the stack > (see `VM_HeapDumper::dump_vthread`), so I don't see problems with memory > usage even huge number of unmounted vthreads. > I think an option to exclude stack traces from heap dump is a separate task. I was actually referring to the footprint of the hprof file, not the in process memory usage while producing it. My concern with not doing the option to exclude stack traces now is that it could result in some unusable or unmanageably large heap dumps, or tools simply being overwhelmed by the number of threads. For example, I just looked at the VisualVM threads view, and it just produces a scrollable list of all threads. What happens if there are suddenly 10's of thousands if not millions of threads? If we are lucky is doesn't choke on them and the platform threads are first in the list, but this is the type of thing I'd like to see testing of before pushing this change. - PR Comment: https://git.openjdk.org/jdk/pull/16665#issuecomment-1839855843
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On Mon, 4 Dec 2023 19:14:59 GMT, Chris Plummer wrote: > > My understanding that information about references is one of the most > > important things for dump analysis (and that's what the issue about). So we > > cannot avoid stack unwinding for unmounted virtual threads. > > As for heapdump file size, each stack trace adds 21 + 53 * frame_number > > bytes for 64bit system (uncompressed data) > > So for 10 frames it adds ~550 bytes, for 20 frames ~1.1KB > > I'm not sure if stack traces are important for analysis, maybe we it makes > > sense to add an option to not include them in heap dump (for both platform > > and virtual threads). > > My concern was with the memory usage of the stack traces. Yes I agree that > including all referenced objects in the dump is important. An option just to > leave out the stack traces seems like a good idea. VM_HeapDumper caches stack traces for platform/carrier and mounted virtual threads only. For unmounted virtual threads ThreadDumper objects are created on the stack (see `VM_HeapDumper::dump_vthread`), so I don't see problems with memory usage even huge number of unmounted vthreads. I think an option to exclude stack traces from heap dump is a separate task. > > > Hprof spec says nothing about 1:1 relation between threads and stack > > traces, so theoretically several HPROF_GC_ROOT_THREAD_OBJ subrecords may > > refer to the same stack trace, but search for identical stack traces may be > > expensive. > > I was thinking initially that you couldn't do this because each stack has > it's own unique set of locals that are referenced, and the locals were part > of the stack trace, but they are not. There is instead a > HPROF_GC_ROOT_JAVA_FRAME record for each local reference. It does include the > ThreadID, and we could probably get away with multiple Thread records > referring to the same stack trace. I think this possible improvement is out of scope for this PR - PR Comment: https://git.openjdk.org/jdk/pull/16665#issuecomment-1839838068
Integrated: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently
On Fri, 24 Nov 2023 06:06:16 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix the issue > noted in https://bugs.openjdk.org/browse/JDK-8320687? > > As noted in the issue, the > `sun.jvmstat.monitor.MonitoredHost.getMonitoredHost()` uses a shared instance > of `java.util.ServiceLoader` to load `MonitoredHostService` services. The > `ServiceLoader` class javadoc explicitly notes that it isn't thread safe. The > issue at hand is caused to due using an instance of `ServiceLoader` > concurrently by multiple threads. > > The fix proposes to guard the usage of the shared `ServiceLoader` instance > through the `monitoredHosts` object monitor. We already use that monitor when > dealing with the internal cache which is populated after loading the relevant > `MonitoredHostService`(s). > > A new jtreg test has been introduced which always reproduces the issue > without the source changes and passes with this fix. > > tier1, tier2, tier3 and svc_tools tests have been run with this change and > all passed. This pull request has now been integrated. Changeset: 81484d8c Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/81484d8c0520cf55ec58fc7b4c81880e69537674 Stats: 103 lines in 2 files changed: 93 ins; 7 del; 3 mod 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently Reviewed-by: alanb, kevinw - PR: https://git.openjdk.org/jdk/pull/16805
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 13:00:33 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8320687? >> >> As noted in the issue, the >> `sun.jvmstat.monitor.MonitoredHost.getMonitoredHost()` uses a shared >> instance of `java.util.ServiceLoader` to load `MonitoredHostService` >> services. The `ServiceLoader` class javadoc explicitly notes that it isn't >> thread safe. The issue at hand is caused to due using an instance of >> `ServiceLoader` concurrently by multiple threads. >> >> The fix proposes to guard the usage of the shared `ServiceLoader` instance >> through the `monitoredHosts` object monitor. We already use that monitor >> when dealing with the internal cache which is populated after loading the >> relevant `MonitoredHostService`(s). >> >> A new jtreg test has been introduced which always reproduces the issue >> without the source changes and passes with this fix. >> >> tier1, tier2, tier3 and svc_tools tests have been run with this change and >> all passed. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion - don't share ServiceLoader Hello Chris, I misunderstood the 2 Reviewer rule for serviceablitiy area then. I thought it was 2 Reviewers (capital R). I'll go ahead and integrate this shortly. Thank you all for the help and reviews. - PR Comment: https://git.openjdk.org/jdk/pull/16805#issuecomment-1839816547
Re: RFR: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS [v3]
On Mon, 4 Dec 2023 23:47:12 GMT, David Holmes wrote: >> Please review this simple clarification to the JVM TI spec regarding use of >> `JAVA_TOOL_OPTIONS` in regards to module options and their format. >> >> I do not believe this clarification needs a CSR request. >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > reviewer feedback Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16896#pullrequestreview-1763703748
RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
The fix is for a regression caused by: [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 The fix of 8308614 just triggered a known issue: [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events are not always posted with -Xcomp option The fix is just a work around with the extra checks of the `JvmtiExport::should_post_field_access()` and `JvmtiExport::should_post_field_modification()`. Testing: - In progress: Test with tiers 1-6 - Commit messages: - 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected Changes: https://git.openjdk.org/jdk/pull/16961/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16961=00 Issue: https://bugs.openjdk.org/browse/JDK-8321219 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961 PR: https://git.openjdk.org/jdk/pull/16961
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v55]
On Sat, 2 Dec 2023 07:37:26 GMT, Jonathan Joo wrote: >> 8315149: Add hsperf counters for CPU time of internal GC threads > > Jonathan Joo has updated the pull request incrementally with two additional > commits since the last revision: > > - Only create CPUTimeCounters if supported > - Ensure TTTC is destructed before publishing Marked as reviewed by manc (Committer). - PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1763690561
Re: RFR: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS [v3]
> Please review this simple clarification to the JVM TI spec regarding use of > `JAVA_TOOL_OPTIONS` in regards to module options and their format. > > I do not believe this clarification needs a CSR request. > > Thanks. David Holmes has updated the pull request incrementally with one additional commit since the last revision: reviewer feedback - Changes: - all: https://git.openjdk.org/jdk/pull/16896/files - new: https://git.openjdk.org/jdk/pull/16896/files/5417a109..7cb6f439 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16896=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16896=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16896.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16896/head:pull/16896 PR: https://git.openjdk.org/jdk/pull/16896
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 18:16:52 GMT, Daniel D. Daugherty wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > dholmes CR - adjust a comment. We would need to examine all uses of JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() to see if they can occur in the context of an attaching thread prior to allocation of the thread object. I suspect not because we allocate immediately after creating the JavaThread. The issue we hit here was events triggered by the execution of the Java code for the Thread constructor. It may be that we are just lucky that the same Java code doesn't lead to the VMObjectAlloc event though. ??? - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839755493
Re: RFR: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS [v2]
On Mon, 4 Dec 2023 12:35:42 GMT, Alan Bateman wrote: >> Thanks for looking at this @AlanBateman and @sspitsyn . I was trying to >> refer to the JNI text in a casual way rather than duplicating the actual >> content from there. This was more a strong hint/suggestion to "go read the >> JNI spec for details". >> >> Happy to change 'form' to 'format'. > > I agree with not duplicating the entire sentence but what would you think > about replacing the text in the parenthesis with "Note the required = > between option and value". Only asking because "(not "option value")" isn't > as clear that we want the reader to see that you can't use a space here. Okay. I thought it very clear to say "use format X (not format Y)", but sure I can change it. - PR Review Comment: https://git.openjdk.org/jdk/pull/16896#discussion_r1414637979
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v48]
On Mon, 4 Dec 2023 13:07:15 GMT, Stefan Johansson wrote: >> In the interest of the RDP1 deadline, should we leave improving the sync >> issues with gc_total to a separate RFE? (Especially given that a "correct" >> design may take some time to come up with, and that gc_total being slightly >> out of sync is not a major issue.) > > Me and Albert discussed this again and we are ok with handling the `gc_total` > sync issue as a follow up. Please create the RFE for that. If that would > include needing a `PerfAtomicCounter`, that would a be its own RFE as well. > For me I think a lock would be a good enough solution. > > From our point of view having the counters out of sync for a long period of > time (think a long concurrent mark cycle without any young collections > updating the total) is not good since it shows that the counters are not > incremented in sync. It would also be nice to avoid the two-step updating of > the total time, so please try to find time to work on this. Thanks, opened https://bugs.openjdk.org/browse/JDK-8321304. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1414634977
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only Just for the record Coleen's review is marked as "Review applies to [81e31dae](https://git.openjdk.org/jdk/pull/16662/files/81e31daeef4c68352368a90e3ab453ba9b33650c)" - which is not the final version. The skara tooling does not currently support our rules but it remains as always that non-trivial Hotspot changes require two reviewers. Thanks for the additional explanations Coleen. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1839721631
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 20:52:46 GMT, Daniel D. Daugherty wrote: > In the interest of reducing the noise in the Mach5 CI, I'm going ahead with > integrating this fix without waiting for a reply to my comment above. If > there are remaining issues, then we'll deal with them in a follow-up bug/RFE. Thumbs up, thanks! - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839594297
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 20:52:46 GMT, Daniel D. Daugherty wrote: >>> @jianglizhou - Thanks for doing the testing. Can you also do a review? We >>> need two reviewers for this change. >> >> Complete test run finished. Checking the results, looks like there's no >> issue related to JVMTIThreadState change. >> >> @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && >> thread->is_attaching_via_jni())` looks ok. >> >> I just rechecked all usages of setup_jvmti_thread_state(). Currently it's >> used in three cases: >> - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() >> - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() >> - JvmtiSampledObjectAllocEventCollector::start() >> >> JDK-8319935 ran into issue with >> JvmtiSampledObjectAllocEventCollector::start() call path. We changed >> JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to >> avoid sampling if there is no thread obj allocated for the attaching thread. >> We also changed JvmtiThreadState::state_for_while_locked to handle the >> attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a >> case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() >> might also see an attaching thread without the thread obj allocated? >> >> JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path >> probably is not affected by this case. > > @jianglizhou and @sspitsyn - Thanks for the reviews. > > In the interest of reducing the noise in the Mach5 CI, I'm going ahead with > integrating this fix without waiting for a reply to my comment above. If there > are remaining issues, then we'll deal with them in a follow-up bug/RFE. > > @dcubed-ojdk and @dholmes-ora, is there a case > > JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > > might also see an attaching thread without the thread obj allocated? > > JvmtiVMObjectAllocEventCollector is the code path that we ran into with the > internal stress test, but in the case that tripped, we had a thread obj > allocated. If we ran the same code path without the thread obj allocated, > then we would return `nullptr` which was the goal of your original fix. > > I don't know if that specific test case is actually reached by any of our > tests, but if such a case occurred, it did not result in the guarantee() > firing or any other failure mode that I saw. The `guarantee(state != nullptr) > failed: exiting thread called setup_jvmti_thread_state` has not been seen in > Mach5 Tier[1-8] testing with this fix in place and I didn't see any other > test failures that could be tracked back to problems with this code. > > A JvmtiVMObjectAllocEventCollector object is created in 34 places in jvm.cpp > alone and I haven't checked all of those. I only checked out the one use in > JVM_GetStackAccessControlContext. Thanks. I was wondering if we would need further work to handle `JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector()` with something similar to the JvmtiSampledObjectAllocEventCollector change, if it's possible to trigger the `guarantee(state != nullptr)` for attaching thread with no allocated thread obj. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839626256
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit
On Mon, 4 Dec 2023 16:08:32 GMT, Alan Bateman wrote: > When a virtual thread continues after Thread.yield it currently consumes > thread's parking permit. This is an oversight, the parking permit should only > be consumed when continuing after park. > > The changes are straight-forward. The internal "RUNNABLE" state is replaced > with UNPARKED and YIELDED state, effectively encoding the previous state. So > for the most part, it's just replacing the usages of RUNNABLE. The additional > states require refactoring tryGetStackTrace, this is the method that is used > for Thread::getStackTrace when the virtual thread is unmounted. It is also > changed to not not swallow the REE if the reesubmit fails (tryStackTrace has > to resubmit as the task for the thread may run, or the thread unparked, while > "suspended" and sampling its stack trace). The changes are a subset of larger > changes in the loom repo, future PRs will propose to bring in other changes > to get main line up to date. > > For testing the existing ThreadAPI has new test cases. > > Testing: test1-5. This includes the JVMTI tests as it maps the thread states > to JVMTI thread states. src/hotspot/share/classfile/javaClasses.cpp line 1998: > 1996: case UNPARKED: > 1997: case YIELDING : > 1998: case YIELDED: Is this new state `YIELDED` really needed? It looks like `UNPARKED` is also appeared where `YIELDED` is used. - PR Review Comment: https://git.openjdk.org/jdk/pull/16953#discussion_r1414561935
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 1 Dec 2023 07:59:49 GMT, Jaikiran Pai wrote: > I'm looking for one more Reviewer, please. You have 2 reviewers already. - PR Comment: https://git.openjdk.org/jdk/pull/16805#issuecomment-1839539742
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit
On Mon, 4 Dec 2023 16:08:32 GMT, Alan Bateman wrote: > When a virtual thread continues after Thread.yield it currently consumes > thread's parking permit. This is an oversight, the parking permit should only > be consumed when continuing after park. > > The changes are straight-forward. The internal "RUNNABLE" state is replaced > with UNPARKED and YIELDED state, effectively encoding the previous state. So > for the most part, it's just replacing the usages of RUNNABLE. The additional > states require refactoring tryGetStackTrace, this is the method that is used > for Thread::getStackTrace when the virtual thread is unmounted. It is also > changed to not not swallow the REE if the reesubmit fails (tryStackTrace has > to resubmit as the task for the thread may run, or the thread unparked, while > "suspended" and sampling its stack trace). The changes are a subset of larger > changes in the loom repo, future PRs will propose to bring in other changes > to get main line up to date. > > For testing the existing ThreadAPI has new test cases. > > Testing: test1-5. This includes the JVMTI tests as it maps the thread states > to JVMTI thread states. src/java.base/share/classes/java/lang/VirtualThread.java line 116: > 114: private static final int NEW = 0; > 115: private static final int STARTED = 1; > 116: private static final int RUNNING = 2; // runnable-unmounted Just want to make sure this change is intentional. Before the comment was: `// runnable-mounted`. - PR Review Comment: https://git.openjdk.org/jdk/pull/16953#discussion_r1414528008
Integrated: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Sat, 2 Dec 2023 17:15:57 GMT, Daniel D. Daugherty wrote: > In the fix for the following bug: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > JvmtiThreadState::state_for_while_locked() was changed to > return nullptr for attaching JNI threads regardless of whether > that JNI thread/JavaThread had a java.lang.Thread object. > > We should only filter out a JavaThread that's attaching via JNI > if it has no java.lang.Thread object. > > This fix has been tested with Mach5 Tier[1-7] and there are no related test > failures. > Mach5 Tier8 is in process. > > I'm going to need @jianglizhou to rerun her testing for: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > since the test(s) for that fix are not yet integrated in the jdk/jdk repo. This pull request has now been integrated. Changeset: 30b5d427 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/30b5d427350d03ec8b9eb39fbf06fbd1b1f66cd8 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 Reviewed-by: dholmes, jiangli, sspitsyn - PR: https://git.openjdk.org/jdk/pull/16934
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 19:01:05 GMT, Jiangli Zhou wrote: >> @dcubed-ojdk Thanks for the notification. I just ran one of our affected >> test 100 times with JDK-8312174 change rolled back and with both following >> applied: >> >> - https://git.openjdk.org/jdk/pull/16642 >> - https://github.com/openjdk/jdk/pull/16934 >> >> All 100 runs passed without failure. I'm going to run all tests tonight, >> will report back later tomorrow if I see any issue. > >> @jianglizhou - Thanks for doing the testing. Can you also do a review? We >> need two reviewers for this change. > > Complete test run finished. Checking the results, looks like there's no issue > related to JVMTIThreadState change. > > @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && > thread->is_attaching_via_jni())` looks ok. > > I just rechecked all usages of setup_jvmti_thread_state(). Currently it's > used in three cases: > - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() > - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > - JvmtiSampledObjectAllocEventCollector::start() > > JDK-8319935 ran into issue with > JvmtiSampledObjectAllocEventCollector::start() call path. We changed > JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to > avoid sampling if there is no thread obj allocated for the attaching thread. > We also changed JvmtiThreadState::state_for_while_locked to handle the > attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a > case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > might also see an attaching thread without the thread obj allocated? > > JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path > probably is not affected by this case. @jianglizhou and @sspitsyn - Thanks for the reviews. In the interest of reducing the noise in the Mach5 CI, I'm going ahead with integrating this fix without waiting for a reply to my comment above. If there are remaining issues, then we'll deal with them in a follow-up bug/RFE. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839457963
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 18:13:50 GMT, Daniel D. Daugherty wrote: >> Initially I thought this was not the right fix as we should not be exposing >> an attaching thread that may still have a partially constructed `threadObj`. >> But this issue shows that we must expose such a thread because the >> constructor of the `Thread` object can trigger these events on the current >> thread so it must have a valid JVMTI state! >> >> Thanks. > > @dholmes-ora - Thanks for the review. > @dcubed-ojdk and @dholmes-ora, is there a case > JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > might also see an attaching thread without the thread obj allocated? JvmtiVMObjectAllocEventCollector is the code path that we ran into with the internal stress test, but in the case that tripped, we had a thread obj allocated. If we ran the same code path without the thread obj allocated, then we would return `nullptr` which was the goal of your original fix. I don't know if that specific test case is actually reached by any of our tests, but if such a case occurred, it did not result in the guarantee() firing or any other failure mode that I saw. The `guarantee(state != nullptr) failed: exiting thread called setup_jvmti_thread_state` has not been seen in Mach5 Tier[1-8] testing with this fix in place and I didn't see any other test failures that could be tracked back to problems with this code. A JvmtiVMObjectAllocEventCollector object is created in 34 places in jvm.cpp alone and I haven't checked all of those. I only checked out the one use in JVM_GetStackAccessControlContext. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839456136
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 18:16:52 GMT, Daniel D. Daugherty wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > dholmes CR - adjust a comment. Looks good. Thank you for fixing this! - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16934#pullrequestreview-1763277330
RFR: 8321270: Virtual Thread.yield consumes parking permit
When a virtual thread continues after Thread.yield it currently consumes thread's parking permit. This is an oversight, the parking permit should only be consumed when continuing after park. The changes are straight-forward. The internal "RUNNABLE" state is replaced with UNPARKED and YIELDED state, effectively encoding the previous state. So for the most part, it's just replacing the usages of RUNNABLE. The additional states require refactoring tryGetStackTrace, this is the method that is used for Thread::getStackTrace when the virtual thread is unmounted. It is also changed to not not swallow the REE if the reesubmit fails (tryStackTrace has to resubmit as the task for the thread may run, or the thread unparked, while "suspended" and sampling its stack trace). The changes are a subset of larger changes in the loom repo, future PRs will propose to bring in other changes to get main line up to date. For testing the existing ThreadAPI has new test cases. Testing: test1-5. This includes the JVMTI tests as it maps the thread states to JVMTI thread states. - Commit messages: - Leave onPinned to another PR - Initial commit Changes: https://git.openjdk.org/jdk/pull/16953/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16953=00 Issue: https://bugs.openjdk.org/browse/JDK-8321270 Stats: 150 lines in 4 files changed: 79 ins; 19 del; 52 mod Patch: https://git.openjdk.org/jdk/pull/16953.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16953/head:pull/16953 PR: https://git.openjdk.org/jdk/pull/16953
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On Sat, 2 Dec 2023 01:48:28 GMT, Alex Menkov wrote: > My understanding that information about references is one of the most > important things for dump analysis (and that's what the issue about). So we > cannot avoid stack unwinding for unmounted virtual threads. > As for heapdump file size, each stack trace adds 21 + 53 * frame_number bytes > for 64bit system (uncompressed data) > So for 10 frames it adds ~550 bytes, for 20 frames ~1.1KB > I'm not sure if stack traces are important for analysis, maybe we it makes > sense to add an option to not include them in heap dump (for both platform > and virtual threads). My concern was with the memory usage of the stack traces. Yes I agree that including all referenced objects in the dump is important. An option just to leave out the stack traces seems like a good idea. > Hprof spec says nothing about 1:1 relation between threads and stack traces, > so theoretically several HPROF_GC_ROOT_THREAD_OBJ subrecords may refer to the > same stack trace, but search for identical stack traces may be expensive. I was thinking initially that you couldn't do this because each stack has it's own unique set of locals that are referenced, and the locals were part of the stack trace, but they are not. There is instead a HPROF_GC_ROOT_JAVA_FRAME record for each local reference. It does include the ThreadID, and we could probably get away with multiple Thread records referring to the same stack trace. but now I'm not seeing any indications that hprof stacks reference the locals, which seems odd. - PR Comment: https://git.openjdk.org/jdk/pull/16665#issuecomment-1839303561
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 18:16:52 GMT, Daniel D. Daugherty wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > dholmes CR - adjust a comment. Marked as reviewed by jiangli (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16934#pullrequestreview-1763177973
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 05:16:59 GMT, Jiangli Zhou wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > @dcubed-ojdk Thanks for the notification. I just ran one of our affected test > 100 times with JDK-8312174 change rolled back and with both following applied: > > - https://git.openjdk.org/jdk/pull/16642 > - https://github.com/openjdk/jdk/pull/16934 > > All 100 runs passed without failure. I'm going to run all tests tonight, will > report back later tomorrow if I see any issue. > @jianglizhou - Thanks for doing the testing. Can you also do a review? We > need two reviewers for this change. Complete test run finished. Checking the results, looks like there's no issue related to JVMTIThreadState change. @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && thread->is_attaching_via_jni())` looks ok. I just rechecked all usages of setup_jvmti_thread_state(). Currently it's used in three cases: - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() - JvmtiSampledObjectAllocEventCollector::start() JDK-8319935 ran into issue with JvmtiSampledObjectAllocEventCollector::start() call path. We changed JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to avoid sampling if there is no thread obj allocated for the attaching thread. We also changed JvmtiThreadState::state_for_while_locked to handle the attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() might also see an attaching thread without the thread obj allocated? JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path probably is not affected by this case. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839282331
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On 12/4/23 5:20 AM, Alan Bateman wrote: On 04/12/2023 12:41, David Holmes wrote: On 1/12/2023 2:08 pm, Alex Menkov wrote: On Thu, 30 Nov 2023 21:11:08 GMT, Chris Plummer wrote: I wasn't thinking in terms of the scheduler somehow no longer references the virtual thread, but instead the program no longer referencing the scheduler (and also not referencing the virtual thread). AFAIU unfinished unmounted virtual threads are referenced from other objects (they are parked on), so they can't be unreachable even is the application is not referencing them and the scheduler. There is (or was - there may be a property that affects this: trackAllThreads?) a scenario where a VT might park on a synchronization object which is not referenced from any other thread. The VT can never be unparked, and the sync object and the VT are reachable only from either other and so both can be GC'd. That's right, the door is not closed to introducing ephemeral threads in the future. Right now, virtual threads created directly with the Thread API remaining strongly reachable once started until they terminate. Virtual threads created in other containers (e.g. a thread-per-task ExecutorService) are kept reachable by the container. -Alan So does this mean if the application is no longer referencing the ExecutorService, then we can have unreachable virtual threads that have not completed? This is really the point I've been getting at. Chris
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 06:31:43 GMT, David Holmes wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> dholmes CR - adjust a comment. > > Initially I thought this was not the right fix as we should not be exposing > an attaching thread that may still have a partially constructed `threadObj`. > But this issue shows that we must expose such a thread because the > constructor of the `Thread` object can trigger these events on the current > thread so it must have a valid JVMTI state! > > Thanks. @dholmes-ora - Thanks for the review. > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 90: > >> 88: // Don't add a JvmtiThreadState to a thread that is exiting or is >> attaching. >> 89: // When a thread is attaching, it may not have a Java level thread >> object >> 90: // created yet. > > The comment needs adjusting now - suggestion: > > // Don't add a JvmtiThreadState to a thread that is exiting, or is attaching > // and does not yet have a Java level thread object allocated. Thanks for the suggested change. I agree and I've applied it. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839205636 PR Review Comment: https://git.openjdk.org/jdk/pull/16934#discussion_r1414306493
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 05:16:59 GMT, Jiangli Zhou wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > @dcubed-ojdk Thanks for the notification. I just ran one of our affected test > 100 times with JDK-8312174 change rolled back and with both following applied: > > - https://git.openjdk.org/jdk/pull/16642 > - https://github.com/openjdk/jdk/pull/16934 > > All 100 runs passed without failure. I'm going to run all tests tonight, will > report back later tomorrow if I see any issue. @jianglizhou - Thanks for doing the testing. Can you also do a review? We need two reviewers for this change. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839205099
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
> In the fix for the following bug: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > JvmtiThreadState::state_for_while_locked() was changed to > return nullptr for attaching JNI threads regardless of whether > that JNI thread/JavaThread had a java.lang.Thread object. > > We should only filter out a JavaThread that's attaching via JNI > if it has no java.lang.Thread object. > > This fix has been tested with Mach5 Tier[1-7] and there are no related test > failures. > Mach5 Tier8 is in process. > > I'm going to need @jianglizhou to rerun her testing for: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > since the test(s) for that fix are not yet integrated in the jdk/jdk repo. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: dholmes CR - adjust a comment. - Changes: - all: https://git.openjdk.org/jdk/pull/16934/files - new: https://git.openjdk.org/jdk/pull/16934/files/5c9eb2ec..0de1484f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16934=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16934=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16934.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16934/head:pull/16934 PR: https://git.openjdk.org/jdk/pull/16934
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v55]
On Sat, 2 Dec 2023 07:37:26 GMT, Jonathan Joo wrote: >> 8315149: Add hsperf counters for CPU time of internal GC threads > > Jonathan Joo has updated the pull request incrementally with two additional > commits since the last revision: > > - Only create CPUTimeCounters if supported > - Ensure TTTC is destructed before publishing Marked as reviewed by sjohanss (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1762364095
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On 04/12/2023 12:41, David Holmes wrote: On 1/12/2023 2:08 pm, Alex Menkov wrote: On Thu, 30 Nov 2023 21:11:08 GMT, Chris Plummer wrote: I wasn't thinking in terms of the scheduler somehow no longer references the virtual thread, but instead the program no longer referencing the scheduler (and also not referencing the virtual thread). AFAIU unfinished unmounted virtual threads are referenced from other objects (they are parked on), so they can't be unreachable even is the application is not referencing them and the scheduler. There is (or was - there may be a property that affects this: trackAllThreads?) a scenario where a VT might park on a synchronization object which is not referenced from any other thread. The VT can never be unparked, and the sync object and the VT are reachable only from either other and so both can be GC'd. That's right, the door is not closed to introducing ephemeral threads in the future. Right now, virtual threads created directly with the Thread API remaining strongly reachable once started until they terminate. Virtual threads created in other containers (e.g. a thread-per-task ExecutorService) are kept reachable by the container. -Alan
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v48]
On Fri, 1 Dec 2023 22:37:58 GMT, Jonathan Joo wrote: >> I think the ideal approach to simplify this is to support Atomic operation >> on a `PerfCounter`. >> We could either introduce a `PerfAtomicCounter`/`PerfAtomicLongCounter` >> class, or perform `Atomic::add()` on the `PerfData::_valuep` pointer. >> There's already `PerfData::get_address()`, so we might be able to do: >> >> >> Atomic::add((volatile jlong >> *)(instance->get_counter(CPUTimeGroups::CPUTimeType::gc_total)->get_address()), >> net_cpu_time); >> >> >> However, a new class `PerfAtomicCounter` is likely cleaner. E.g., we may >> also want to make `PerfAtomicCounter::sample()` use a CAS. It is probably >> better to introduce `PerfAtomicCounter` in a separate RFE later. >> >> Would the `Atomic::add()` with `PerfData::get_address()` approach be OK for >> now, or would we rather introduce a lock, or leave the `gc_total` mechanism >> as-is and address the out-of-sync-ness in a follow-up RFE? >> >> IMO the out-of-sync-ness problem is minor, because users are likely to >> either look at a single `gc_total` counter, or look at each individual GC >> CPU counter and disregard `gc_total`. > > In the interest of the RDP1 deadline, should we leave improving the sync > issues with gc_total to a separate RFE? (Especially given that a "correct" > design may take some time to come up with, and that gc_total being slightly > out of sync is not a major issue.) Me and Albert discussed this again and we are ok with handling the `gc_total` sync issue as a follow up. Please create the RFE for that. If that would include needing a `PerfAtomicCounter`, that would a be its own RFE as well. For me I think a lock would be a good enough solution. >From our point of view having the counters out of sync for a long period of >time (think a long concurrent mark cycle without any young collections >updating the total) is not good since it shows that the counters are not >incremented in sync. It would also be nice to avoid the two-step updating of >the total time, so please try to find time to work on this. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1413848440
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On 1/12/2023 2:08 pm, Alex Menkov wrote: On Thu, 30 Nov 2023 21:11:08 GMT, Chris Plummer wrote: I wasn't thinking in terms of the scheduler somehow no longer references the virtual thread, but instead the program no longer referencing the scheduler (and also not referencing the virtual thread). AFAIU unfinished unmounted virtual threads are referenced from other objects (they are parked on), so they can't be unreachable even is the application is not referencing them and the scheduler. There is (or was - there may be a property that affects this: trackAllThreads?) a scenario where a VT might park on a synchronization object which is not referenced from any other thread. The VT can never be unparked, and the sync object and the VT are reachable only from either other and so both can be GC'd. David - - PR Comment: https://git.openjdk.org/jdk/pull/16665#issuecomment-1835422117
Re: RFR: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS [v2]
On Mon, 4 Dec 2023 04:43:46 GMT, David Holmes wrote: >> src/hotspot/share/prims/jvmti.xml line 746: >> >>> 744: JNI_CreateJavaVM (in the JNI Invocation API) will >>> prepend these options to the options supplied >>> 745: in its JavaVMInitArgs argument. Note that module >>> related options must be expressed in their >>> 746: "option=value" form (not "option value") for >>> JNI_CreateJavaVM to process them correctly. >> >> This looks okay. I'm just comparing it to the text that we put into the JNI >> spec: >> >> "The module related options ... as option strings using their "option=value" >> format instead of their "option value" format. (Note the required = between >> "option" and "value".)" >> >> It uses "format" instead of "form" and also, the bit I think works well, is >> to point out "required =" to force the reader to re-read the previous >> sentence and see what the difference is in the formats. > > Thanks for looking at this @AlanBateman and @sspitsyn . I was trying to > refer to the JNI text in a casual way rather than duplicating the actual > content from there. This was more a strong hint/suggestion to "go read the > JNI spec for details". > > Happy to change 'form' to 'format'. I agree with not duplicating the entire sentence but what would you think about replacing the text in the parenthesis with "Note the required = between option and value". Only asking because "(not "option value")" isn't as clear that we want the reader to see that you can't use a space here. - PR Review Comment: https://git.openjdk.org/jdk/pull/16896#discussion_r1413811389
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Mon, 4 Dec 2023 11:11:27 GMT, Thomas Stuefe wrote: > Okay so why does this happen and is it a reasonable thing to be happening? On > the surface it sounds wrong to deallocate anything associated with a live > classloader. If we didn't deallocate these old methods, there would be a memory leak when using class redefinition. It would be a lot simpler if we didn't have to do this though. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838543829
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v2]
> On AIX, repeated calls to dlopen referring to the same shared library may > result in different, unique dl handles to be returned from libc. In that it > differs from typical libc implementations that cache dl handles. > > This causes problems in the JVM with code that assumes equality of handles. > One such problem is in the JVMTI agent handler. That problem was fixed with a > local fix to said handler > ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this > fix causes follow-up problems since it assumes that the file name passed to > `os::dll_load()` is the file that has been opened. It prevents efficient, > os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality. > See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it > is a hack that causes other, more uglier hacks to follow (see discussion of > https://github.com/openjdk/jdk/pull/16604). > > We propose a different, cleaner way of handling this: > > - Handle this entirely inside the AIX versions of os::dll_load and > os::dll_unload. > - Cache dl handles; repeated opening of a library should return the cached > handle. > - Increase handle-local ref counter on open, Decrease it on close > - Make sure calls to os::dll_load are matched to os::dll_unload (See > [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)). > > This way we mimic dl handle equality as it is implemented on other platforms, > and this works for all callers of os::dll_load. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: improve handling of nonexisting files - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/e756f496..0f6716db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16920=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16920=00-01 Stats: 16 lines in 1 file changed: 4 ins; 5 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16920.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16920/head:pull/16920 PR: https://git.openjdk.org/jdk/pull/16920
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only We deallocate the old Method* if nothing is referring to them (ie they're not running or being referenced for some other reason). Look at MetadataOnStackMark. The jmethodIDs to an obsolete method were a dangling pointer and we want to just null them out. The old Method* are attached to the scratch_version of the InstanceKlass so we essentially remove that and walk down to the Methods when none of them are found. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838538568
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Mon, 4 Dec 2023 00:41:23 GMT, David Holmes wrote: > From the blog: > > > Yes! The methods are being deallocated for a class loader that is still > > alive. > > Okay so why does this happen and is it a reasonable thing to be happening? On > the surface it sounds wrong to deallocate anything associated with a live > classloader. This sounds odd to me to. I know that we deallocate the old *byte code* of re-transformed classes; but `Method*` ? - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838413238
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Fri, 1 Dec 2023 05:15:15 GMT, Thomas Stuefe wrote: >> Jaroslav Bachorik has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Restrict cleanup to obsolete methods only > > I won't be able to review this this week, too snowed in atm. I can take a > look next week. We can always just revert the change if needed. > > Thinking about Skara, I think as long as we have this confusing mixture of > rules (hotspot wants 2 reviewers that are Reviewer/Committer, but some jdk > libs only want one, but then you need two for desktop I think otherwise Phil > gets angry) - we should hard-code the 2-reviewer rule into skara as default > since it affects the lion's share of all changes. @tstuefe I got confused by the Skara tooling. I had a vague memory of some discussions going on about relaxing the requirement of 2 reviewers for some parts to the code base and I thought I was in a good shape seeing the Skara checkbox.  As for not having a review for the final version - I am not that restless. I specifically dismissed the previous review to avoid incidentally integrating based on a review of a version that was not actual. Then I asked @coleenp to re-do the review on the final bits (https://github.com/openjdk/jdk/pull/16662#issuecomment-1827432032) @dholmes-ora >Okay so why does this happen and is it a reasonable thing to be happening? On >the surface it sounds wrong to deallocate anything associated with a live >classloader. This is happening for previous versions of retransformed methods. As long as those methods are still on stack they are kept alive. But once they are not executing they are free to be destroyed. And this is where the problem was happening - the previous versions of methods were being destroyed but the associated jmethodIDs were not updated not to point to what became an invalid memory block. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838221097
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Sat, 2 Dec 2023 01:22:33 GMT, Jonathan Joo wrote: >> I still think that a total counter is useful and I'd appreciate if you can >> keep it. To second what @caoman said before, it is GC agnostic, easy to use >> even for non GC experts and future proof with regards to implementation >> changes in the GCs. Please keep it. > > Put the closure in a scope, I think that should address the concern. Yes, scoping it will work. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1413591437
