Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]
On Mon, 18 Mar 2024 13:14:41 GMT, Richard Reingruber wrote: >> This pr changes `JfrJvmtiAgent::retransform_classes()` and >> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm. >> >> Testing: >> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java >> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync >> >> More tests are pending. > > Richard Reingruber 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 three additional > commits since the last revision: > > - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite > - Set WXWrite at more locations > - Switch to WXWrite before entering the vm I still feel that this should be fixed inside the `ThreadInVMfromNative` transition - the number of callsites that need it just reinforces that for me. Granted we then need to look at where we would now have redundant calls. That said we have had a lot of people looking at the overall WX state management logic in the past week or so due to https://bugs.openjdk.org/browse/JDK-8327860. The workaround there requires us to be in EXEC mode and there are a number of folk who are questioning why "we" chose WRITE mode as the default with a switch to EXEC, instead of EXEC as the default with a switch to WRITE. But whichever way that goes I think the VM state transitions are the places to enforce that choice. - PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1945158370
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v13]
On Mon, 18 Mar 2024 23:45:29 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: rename is_interrupted() to get_and_clear_interrupted(); update > comment Thanks for the updates. I think that is all from me. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18093#pullrequestreview-1945144021
Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v2]
> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were > introduced in the JDK 22 to unify/simplify the JVM TI functions supporting > implementation of the virtual threads. This enhancement is to refactor JVM TI > functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on the > base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes. > > Testing: > - Ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: correct one comment - Changes: - all: https://git.openjdk.org/jdk/pull/18332/files - new: https://git.openjdk.org/jdk/pull/18332/files/a31c4f71..78b7c894 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18332=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18332=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18332.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18332/head:pull/18332 PR: https://git.openjdk.org/jdk/pull/18332
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v12]
On Mon, 18 Mar 2024 11:10:47 GMT, Serguei Spitsyn wrote: >> Maybe `is_interrupted_with_reset`? (though that would seem like an alias >> for `is_interrupted(true)`. > > Thanks, Alan and David. Will consider your suggestions. Renamed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1529479113
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v13]
> Please, review this fix correcting the JVMTI `RawMonitorWait()` > implementation. > The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to update > the interrupt status of the interrupted waiting thread. The issue is that > when it calls `jt->is_interrupted(true)` to fetch and clear the `interrupt > status` of the virtual thread, this information is not propagated to the > `java.lang.Thread` instance. > In the `VirtualThread` class implementation the `interrupt status` for > virtual thread is stored for both virtual and carrier threads. It is because > the implementation of object monitors for virtual threads is based on pinning > virtual threads, and so, always operates on carrier thread. The fix is to > clear the interrupt status for both virtual and carrier `java.lang.Thread` > instances. > > Testing: > - tested with new test > `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is > passed with the fix and failed without it > - ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: rename is_interrupted() to get_and_clear_interrupted(); update comment - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/5e0db950..f45f355c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18093=12 - incr: https://webrevs.openjdk.org/?repo=jdk=18093=11-12 Stats: 9 lines in 3 files changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18093.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18093/head:pull/18093 PR: https://git.openjdk.org/jdk/pull/18093
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests
On Mon, 18 Mar 2024 16:47:24 GMT, Bill Huang wrote: > This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. test/jdk/java/nio/channels/unixdomain/Bind.java line 191: > 189: server.bind(null); > 190: UnixDomainSocketAddress usa = > (UnixDomainSocketAddress)server.getLocalAddress(); > 191: usa.getPath().toFile().deleteOnExit(); The test already deletes the file, I think you just want a try-finally here, same comment on a few other tests. - PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1529408542
RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests
This task addresses an essential aspect of our testing infrastructure: the proper handling and cleanup of temporary files and socket files created during test execution. The motivation behind these changes is to prevent the accumulation of unnecessary files in the default temporary directory, which can affect the system's storage and potentially influence subsequent test runs. Our review identified that several tests create temporary files or socket files without ensuring their removal post-execution. - Direct calls to java.io.File.createTempFile and java.nio.file.Files.createTempFile without adequate cleanup. - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not explicitly removing socket files post-use. - Commit messages: - Clean up temporary files after tests complete. Changes: https://git.openjdk.org/jdk/pull/18352/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18352=00 Issue: https://bugs.openjdk.org/browse/JDK-8327474 Stats: 42 lines in 11 files changed: 20 ins; 1 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
RFR: 8328341: Remove deprecated per-thread compiler stats in sun.management
Please review this cleanup PR which removes per-thread compiler stats from `sun.management` This removes: * The deprecated interface method `HotspotCompilationMBean.getCompilerThreadStats()` along with the implementation method in `HotspotCompilation` * The class returned by these methods, `CompilerThreadStat` * The nested class `HotspotCompilation.CompilerThreadInfo` which now falls out of use * The field `HotspotCompilation.threads`, along with its initialization in `initCompilerCounters` Testing and verification: As this is purely a removal and cleanup PR of unused code, no updates are made on the testing side. I have verified that the string `getCompilerThreadStats` is not found in OpenJDK after this PR. - Commit messages: - Remove HotspotCompilationMBean.getCompilerThreadStats() with associated implementation code Changes: https://git.openjdk.org/jdk/pull/18344/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18344=00 Issue: https://bugs.openjdk.org/browse/JDK-8328341 Stats: 149 lines in 3 files changed: 0 ins; 147 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18344.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18344/head:pull/18344 PR: https://git.openjdk.org/jdk/pull/18344
RE: [External] : Re: RFD: Can we remove per-thread compiler stats now?
Would that be okay with you? Absolutely!
Re: [External] : Re: RFD: Can we remove per-thread compiler stats now?
On Mon, Mar 18, 2024 at 2:31 PM Kevin Walls wrote: > Right, the existing Deprecated tag had me thinking this was a Java SE > interface that had begun the deprecation process. > > But it's not a published API. > Great, it seems we have consensus that we can go ahead and remove these deprecated methods which return no useful data. Maybe we can go further: HotspotCompilationMBean and HotspotInternalMBean > look unused. They are not exposed in the standard PlatformMBeanServer. > > It seems we can! But since (a) this is my first serviceability contribution and (b) the motivations for removing other unused beans may differ and also be more controversial, I'd prefer to warm up with a PR narrowly focused on the deprecated per-thread stats. We can then follow up with a broader discussion of removal of unused Hotspot internal beans. Would that be okay with you? While looking for who/what uses these, I also found a recent mailing list > thread: > > "Question on why sun.management MBeans are not exported?" > > https://www.mail-archive.com/core-libs-dev@openjdk.org/msg19878.html > Thanks, that provides some very useful context. Cheers, Eirik.
RE: [External] : Re: RFD: Can we remove per-thread compiler stats now?
Right, the existing Deprecated tag had me thinking this was a Java SE interface that had begun the deprecation process. But it's not a published API. Maybe we can go further: HotspotCompilationMBean and HotspotInternalMBean look unused. They are not exposed in the standard PlatformMBeanServer. While looking for who/what uses these, I also found a recent mailing list thread: "Question on why sun.management MBeans are not exported?" https://www.mail-archive.com/core-libs-dev@openjdk.org/msg19878.html ..where the idea of removal also came up. Thanks Kevin From: Eirik Bjørsnøs Sent: 17 March 2024 18:43 To: Kevin Walls Cc: serviceability-dev@openjdk.org Subject: [External] : Re: RFD: Can we remove per-thread compiler stats now? On Fri, Mar 15, 2024 at 7:54 PM Kevin Walls mailto:kevin.wa...@oracle.com>> wrote: https://openjdk.org/jeps/277 "An API element should not be removed from the Java SE specification unless it has been delivered with an annotation of @Deprecated(forRemoval=true) in a previous version of Java SE." So deprecations need to be "upgraded" to forRemoval, this will take a few updates to get them removed. Hmm.. But if sun.management.HotspotCompilationMBean is an internal class, it's not defined in the Java SE Specification, right? My understanding when working on similar core-libs cleanups is that internal APIs can be removed directly without the need for a forRemoval step. HotspotCompilationMBean: @Deprecated public java.util.List getCompilerThreadStats(); .. need to consider whether a method is removed or just "degraded" (not return any information, or throw). I think this is already "degraded" in the sense that it does not return useful information (underlying counters are long removed). My intention here is to remove crufty leftover code. I think the best outcome is if we can remove the code so maintainers no longer need to see or reason about it. That said, I see sun.management.CompilationImpl[java.lang:type=Compilation] ..rather than HotspotCompilation. ManagementFactoryHelper.java: getCompilationMXBean() specifically creates a CompilationImpl, users don’t normally see the HotspotCompilation mbean. This and the rest goes into code and concepts I'm not very familiar with (yet!), so I'm not sure I understand the implications well. But do you think perhaps the whole HotspotCompilationMBean with implementation could be removed? I'm not sure I see any consumers within OpenJDK of this interface? The role of HotspotCompilationMBean needs more investigation, but basically yes to the idea. 8-) Good! It seems there is potential for some level of cleanup here, so I'll go ahead and make a PR where we can continue the discussion. Thanks, Eirik.
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]
> This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` > to switch to `WXWrite` before transitioning to the vm. > > Testing: > make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > > More tests are pending. Richard Reingruber 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 three additional commits since the last revision: - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite - Set WXWrite at more locations - Switch to WXWrite before entering the vm - Changes: - all: https://git.openjdk.org/jdk/pull/18238/files - new: https://git.openjdk.org/jdk/pull/18238/files/9075f4be..1b4d7606 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18238=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18238=01-02 Stats: 293771 lines in 434 files changed: 6197 ins; 6221 del; 281353 mod Patch: https://git.openjdk.org/jdk/pull/18238.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238 PR: https://git.openjdk.org/jdk/pull/18238
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v12]
On Mon, 18 Mar 2024 02:21:40 GMT, David Holmes wrote:
>> src/hotspot/share/runtime/javaThread.cpp line 596:
>>
>>> 594: // Checks and clears the interrupt status for platform or virtual
>>> thread.
>>> 595: // Used by the JVMTI RawMonitorWait only.
>>> 596: bool JavaThread::is_interrupted() {
>>
>> Reading code that uses JavaThread::is_interrupted() won't be clear that it
>> resets the interrupt status. Can this be something that
>> get_and_clear_interrupted or something that makes it clearer?
>
> Maybe `is_interrupted_with_reset`? (though that would seem like an alias for
> `is_interrupted(true)`.
Thanks, Alan and David. Will consider your suggestions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1528336441
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v12]
On Mon, 18 Mar 2024 02:23:44 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: made current changes limitedto just RawMonitorWait > > src/hotspot/share/runtime/javaThread.cpp line 595: > >> 593: >> 594: // Checks and clears the interrupt status for platform or virtual >> thread. >> 595: // Used by the JVMTI RawMonitorWait only. > > Or more strongly: > > // This is only for use by JVMTI RawMonitorWait. It emulates the actions of > the Java code in Object::wait > // which are not present in RawMonitorWait. Good suggestion, thanks. Will make an update. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1528337516
Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake
On Mon, 18 Mar 2024 04:43:02 GMT, David Holmes wrote: >> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were >> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting >> implementation of the virtual threads. This enhancement is to refactor JVM >> TI functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on >> the base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes. >> >> Testing: >> - Ran mach5 tiers 1-6 > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 2004: > >> 2002: >> 2003: hs_cl->set_target_jt(target_jt); // can be needed in the virtual >> thread case >> 2004: hs_cl->set_is_virtual(is_virtual); // needed when suspend is >> required for non-current target thread > > Is this comment correct? Seems like it was just copied. Good catch. It was a copy-paste of the wrong comment. Will fix it. - PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1528332705
RFR: 8328177: Move LDFLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
Similar to [JDK-8328157](https://bugs.openjdk.org/browse/JDK-8328157), we want to move the setting of LDFLAGS to LDFLAGS_JDK[LIB/EXE into SetupJdkLibrary and SetupJdkExecutable. - Commit messages: - 8328177: Move LDFLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk Changes: https://git.openjdk.org/jdk/pull/18343/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18343=00 Issue: https://bugs.openjdk.org/browse/JDK-8328177 Stats: 155 lines in 32 files changed: 29 ins; 99 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/18343.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18343/head:pull/18343 PR: https://git.openjdk.org/jdk/pull/18343
Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v2]
On Fri, 15 Mar 2024 13:58:15 GMT, Tobias Holenstein wrote: > > As I wrote in JBS, shouldn't this be handled by `ThreadInVMfromNative`? > > I agree. This is something I am investigating at the moment. Ideally, > AssertWXAtThreadSync would also be true by default. I've added a bunch more locations we've seen when testing with AssertWXAtThreadSync. @tobiasholenstein would you think that this PR is actually not needed because you are going to push a better way of handling the WXMode in the near future? How should be handle the issues in released versions (jdk 21, 17, ...)? Will it be possible to backport your work? - PR Comment: https://git.openjdk.org/jdk/pull/18238#issuecomment-2003496165
Re: RFR: 8327990: [macosx-aarch64] JFR enters VM without WXWrite [v2]
> This pr changes `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` > to switch to `WXWrite` before transitioning to the vm. > > Testing: > make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java > TEST_VM_OPTS=-XX:+AssertWXAtThreadSync > > More tests are pending. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Set WXWrite at more locations - Changes: - all: https://git.openjdk.org/jdk/pull/18238/files - new: https://git.openjdk.org/jdk/pull/18238/files/b34ad5f3..9075f4be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18238=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18238=00-01 Stats: 6 lines in 4 files changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18238.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238 PR: https://git.openjdk.org/jdk/pull/18238
