Re: RFR: 8330427: Obsolete -XX:+PreserveAllAnnotations [v2]
On Thu, 25 Jul 2024 01:53:13 GMT, Alex Menkov wrote: >> Obsolete PreserveAllAnnotations flag which was deprecated in JDK 23. >> >> Testing: tier1,tier2,tier3,tier4,hs-tier5-svc > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > remove test Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20315#pullrequestreview-2198112296
Re: RFR: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be [v5]
On Wed, 24 Jul 2024 21:50:01 GMT, Serguei Spitsyn wrote:
>> The test
>> `serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java` is
>> failing with the assert in the `thaw_internal()` function. The assert is not
>> fully correct as it does not account for an unexpected scenario.
>>
>> Thanks to Patricio for reproducing this failure and identifying the root
>> cause:
>>> The problem is that we can unmount a virtual thread, then mount it again,
>>> thaw a few frames, execute code that acquires a JNI monitor, and then call
>>> thaw again without releasing that monitor. In this test this will happen if
>>> the vthread is unmounted in System.out.println("Thread doing JNI call: "
>>> ...) because of contention with the main thread doing
>>> System.out.println("Main waiting for event.").
>> The issue can be reproduced by adding Thread.yield() before
>> jniMonitorEnterAndLetObjectDie().
>>
>> The fix corrects the assert to account for the `thread->jni_monitor_count()`.
>> Question: Is the same scenario possible for non-JNI monitors as well?
>> Also, the fix includes the test tweak described above which makes this
>> failure always reproducible.
>>
>> Testing:
>> - Ran the test `GetOwnedMonitorInfoTest.java` locally
>> - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> restored recently fixed comment
Marked as reviewed by dholmes (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/20294#pullrequestreview-2198085110
Re: RFR: 8330427: Obsolete -XX:+PreserveAllAnnotations
On Wed, 24 Jul 2024 18:01:15 GMT, Alex Menkov wrote: > Obsolete PreserveAllAnnotations flag which was deprecated in JDK 23. > > Testing: tier1,tier2,tier3,tier4,hs-tier5-svc Great cleanup - good to see all that complexity go! I think the test can be removed completely - see below. Thanks test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 32: > 30: * @run shell MakeJAR.sh retransformAgent > 31: * @run main/othervm -javaagent:retransformAgent.jar > -Xlog:redefine+class=trace RetransformRecordAnnotation > 32: * @run main/othervm -javaagent:retransformAgent.jar > -XX:+PreserveAllAnnotations -Xlog:redefine+class=trace > RetransformRecordAnnotation This test is described as: * @summary test that records with invisible annotation can be retransformed ``` which suggests to me the test can actually be deleted as it serves no purpose now there are no invisible annotations - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20315#pullrequestreview-2198083244 PR Review Comment: https://git.openjdk.org/jdk/pull/20315#discussion_r1690664957
Re: RFR: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be [v2]
On Wed, 24 Jul 2024 18:57:18 GMT, Serguei Spitsyn wrote: >> test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java >> line 89: >> >>> 87:+ Thread.currentThread().getName()); >>> 88: >>> 89: Thread.yield(); >> >> Please add a comment explaining why the yield is present > > Thanks, David. Added a comment. Please, let me know if new comment is not > good enough. I do not want to explain the whole scenario there. The comment is sufficient - thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1690435047
Re: RFR: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be [v3]
On Wed, 24 Jul 2024 18:59:44 GMT, Serguei Spitsyn wrote:
>> The test
>> `serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java` is
>> failing with the assert in the `thaw_internal()` function. The assert is not
>> fully correct as it does not account for an unexpected scenario.
>>
>> Thanks to Patricio for reproducing this failure and identifying the root
>> cause:
>>> The problem is that we can unmount a virtual thread, then mount it again,
>>> thaw a few frames, execute code that acquires a JNI monitor, and then call
>>> thaw again without releasing that monitor. In this test this will happen if
>>> the vthread is unmounted in System.out.println("Thread doing JNI call: "
>>> ...) because of contention with the main thread doing
>>> System.out.println("Main waiting for event.").
>> The issue can be reproduced by adding Thread.yield() before
>> jniMonitorEnterAndLetObjectDie().
>>
>> The fix corrects the assert to account for the `thread->jni_monitor_count()`.
>> Question: Is the same scenario possible for non-JNI monitors as well?
>> Also, the fix includes the test tweak described above which makes this
>> failure always reproducible.
>>
>> Testing:
>> - Ran the test `GetOwnedMonitorInfoTest.java` locally
>> - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> added a comment explaining why extra yield is needed
test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
line 56:
> 54:
> 55: private static void jniMonitorEnterAndLetObjectDie() {
> 56: // The monitor iterator used by GetOwnedMonitorInfo to
The original was correct "used to assert"
test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
line 90:
> 88:
> 89: // Extra unmount helps to reproduce 8334085.
> 90: // Two sub-sequential thaws are needed in that sceanrio.
s/sceanrio/scenario
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1690432483
PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1690433117
Re: RFR: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be [v2]
On Tue, 23 Jul 2024 07:25:44 GMT, Serguei Spitsyn wrote:
>> The test
>> `serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java` is
>> failing with the assert in the `thaw_internal()` function. The assert is not
>> fully correct as it does not account for an unexpected scenario.
>>
>> Thanks to Patricio for reproducing this failure and identifying the root
>> cause:
>>> The problem is that we can unmount a virtual thread, then mount it again,
>>> thaw a few frames, execute code that acquires a JNI monitor, and then call
>>> thaw again without releasing that monitor. In this test this will happen if
>>> the vthread is unmounted in System.out.println("Thread doing JNI call: "
>>> ...) because of contention with the main thread doing
>>> System.out.println("Main waiting for event.").
>> The issue can be reproduced by adding Thread.yield() before
>> jniMonitorEnterAndLetObjectDie().
>>
>> The fix corrects the assert to account for the `thread->jni_monitor_count()`.
>> Question: Is the same scenario possible for non-JNI monitors as well?
>> Also, the fix includes the test tweak described above which makes this
>> failure always reproducible.
>>
>> Testing:
>> - Ran the test `GetOwnedMonitorInfoTest.java` locally
>> - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> comment tweak
Looks okay but one request.
Thanks
test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java
line 89:
> 87:+ Thread.currentThread().getName());
> 88:
> 89: Thread.yield();
Please add a comment explaining why the yield is present
-
PR Review: https://git.openjdk.org/jdk/pull/20294#pullrequestreview-2196517572
PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1689636214
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]
On Fri, 19 Jul 2024 14:07:12 GMT, Sonia Zaldana Calles
wrote:
>> Hi all,
>>
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492)
>> enabling jcmd diagnostic commands that issue an output file to accept the
>> `%p` pattern in the file name and substitute it for the PID.
>>
>> This PR addresses the following diagnostic commands:
>> - [x] Compiler.perfmap
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>>
>> Note that some jcmd diagnostic commands already enable this functionality
>> (`JFR.configure, JFR.dump, JFR.start and JFR.stop`).
>>
>> I propose opening a separate issue to track updating the man page similarly
>> to how it’s done for the JFR diagnostic commands. For example,
>>
>>
>> filename (Optional) Name of the file to which the flight recording
>> data is
>>written when the recording is stopped. If no filename is
>> given, a
>>filename is generated from the PID and the current date
>> and is
>>placed in the directory where the process was started. The
>>filename may also be a directory in which case, the
>> filename is
>>generated from the PID and the current date in the
>> specified
>>directory. (STRING, no default value)
>>
>>Note: If a filename is given, '%p' in the filename will be
>>replaced by the PID, and '%t' will be replaced by the
>> time in
>>'_MM_dd_HH_mm_ss' format.
>>
>>
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265),
>> sources for the jcmd manpage remain in Oracle internal repos so this PR
>> can’t address that.
>>
>> Testing:
>>
>> - [x] Added test case passes.
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames.
>>
>> Looking forward to your comments and addressing any diagnostic commands I
>> might have missed (if any).
>>
>> Cheers,
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Missing copyright header update
src/hotspot/share/services/diagnosticArgument.cpp line 361:
> 359: void DCmdArgument::parse_value(const char *str, size_t len,
> 360:TRAPS) {
> 361: if (str == NULL) {
s/NULL/nullptr
src/hotspot/share/services/diagnosticArgument.cpp line 372:
> 370:
> 371: template <> void DCmdArgument::init_value(TRAPS) {
> 372: if (has_default() && _default_string != NULL) {
s/NULL/nullptr
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685862082
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685862613
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]
On Sat, 20 Jul 2024 07:50:46 GMT, Thomas Stuefe wrote: >> Yes. In this file, other commands use `fatal` only where reading the >> hard-coded default values - in the various `init_...` functions. Hard-coded >> values should be valid, obviously, otherwise the JVM developer messed up. >> Other values are passed in by the end user via jcmd and should not crash the >> JVM. > > I see the prevalent way to deal with runtime parse errors is to throw a java > exception. That exception later is caught in the command processing loop at > the entrance of the attach listener thread. > > So, @SoniaZaldana, I would do this here too - when in Rome... > > But is this not unnecessarily complex? It requires the AttachListener to be a > java thread when in fact it does need no java - we just misuse java exception > handling as a way to pass error information up the stack, with the simple > ultimate goal of writing error information into the outputStream to be sent > to the caller. We might just as well pass the outputStream* to the parse_xxx > functions as third argument, and write directly and return some error code. > This would make the attach listener thread a lot less dependent on Java and > more robust - at least for jcmds that don't need Java (which jcmds need > java?). > > After all, the attach listener is supposed to be super robust and always work > even if the JVM misbehaves. @dholmes-ora @lmesnik what do you guys think, > should we change that? (obviously in a different RFE) If the attach listener thread doesn't actually need to be a Java thread then you could look into changing that. Not sure it would really buy us that much in terms of added robustness though. - PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685861533
Re: [jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
On Fri, 19 Jul 2024 08:26:51 GMT, Alan Bateman wrote: >> Before RC we need to update the man pages in the repo from their Markdown >> sources. All pages at a minimum have 23-ea replaced with 23, and publication >> year set to 2024 if needed. >> >> This also picks up the unpublished changes to java.1 from: >> >> - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage >> for obsoletion of RAMFraction flags >> - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage >> for obsoletion of ScavengeBeforeFullGC >> >> And a typo crept in to java.1 from: >> - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the >> Memory-Access Methods in sun.misc.Unsafe for Removal >> >> This also picks up the unpublished change to keytool.1 from: >> >> - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in >> Supported Named Extensions - BasicContraints >> >> This also picks up the unpublished change to javadoc.1 from: >> >> - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should >> default @since for a nested class to that of its enclosing class >> >> and some formatting changes (unclear why - perhaps pandoc version) from the >> combined changeset for: >> >> - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP >> 467: Markdown Documentation Comments >> - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update >> Elements for '///' documentation comments >> >> The javac.1 file has its copyright year reverted to match what is in the >> source file (which should have been updated but wasn't). >> >> Thanks. > > Thanks for the tireless effort to update the man pages at the end of each > release. Thanks for the reviews @AlanBateman and @irisclark ! - PR Comment: https://git.openjdk.org/jdk/pull/20248#issuecomment-2241805932
[jdk23] Integrated: 8325280: Update troff manpages in JDK 23 before RC
On Fri, 19 Jul 2024 05:47:15 GMT, David Holmes wrote: > Before RC we need to update the man pages in the repo from their Markdown > sources. All pages at a minimum have 23-ea replaced with 23, and publication > year set to 2024 if needed. > > This also picks up the unpublished changes to java.1 from: > > - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage > for obsoletion of RAMFraction flags > - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage > for obsoletion of ScavengeBeforeFullGC > > And a typo crept in to java.1 from: > - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the > Memory-Access Methods in sun.misc.Unsafe for Removal > > This also picks up the unpublished change to keytool.1 from: > > - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in > Supported Named Extensions - BasicContraints > > This also picks up the unpublished change to javadoc.1 from: > > - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should > default @since for a nested class to that of its enclosing class > > and some formatting changes (unclear why - perhaps pandoc version) from the > combined changeset for: > > - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP > 467: Markdown Documentation Comments > - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements > for '///' documentation comments > > The javac.1 file has its copyright year reverted to match what is in the > source file (which should have been updated but wasn't). > > Thanks. This pull request has now been integrated. Changeset: 5473e9e4 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/5473e9e488c8fc7d99ea9d97fd0e7dd212636546 Stats: 142 lines in 28 files changed: 51 ins; 52 del; 39 mod 8325280: Update troff manpages in JDK 23 before RC Reviewed-by: alanb, iris - PR: https://git.openjdk.org/jdk/pull/20248
[jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
Before RC we need to update the man pages in the repo from their Markdown sources. All pages at a minimum have 23-ea replaced with 23, and publication year set to 2024 if needed. This also picks up the unpublished changes to java.1 from: - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage for obsoletion of RAMFraction flags - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage for obsoletion of ScavengeBeforeFullGC And a typo crept in to java.1 from: - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal This also picks up the unpublished change to keytool.1 from: - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in Supported Named Extensions - BasicContraints This also picks up the unpublished change to javadoc.1 from: - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should default @since for a nested class to that of its enclosing class and some formatting changes (unclear why - perhaps pandoc version) from the combined changeset for: - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP 467: Markdown Documentation Comments - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements for '///' documentation comments The javac.1 file has its copyright year reverted to match what is in the source file (which should have been updated but wasn't). Thanks. - Commit messages: - 8325280: Update troff manpages in JDK 23 before RC Changes: https://git.openjdk.org/jdk/pull/20248/files Webrev: https://webrevs.openjdk.org/?repo=jdk=20248=00 Issue: https://bugs.openjdk.org/browse/JDK-8325280 Stats: 142 lines in 28 files changed: 51 ins; 52 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/20248.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20248/head:pull/20248 PR: https://git.openjdk.org/jdk/pull/20248
Re: [jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
On Fri, 19 Jul 2024 05:47:15 GMT, David Holmes wrote: > Before RC we need to update the man pages in the repo from their Markdown > sources. All pages at a minimum have 23-ea replaced with 23, and publication > year set to 2024 if needed. > > This also picks up the unpublished changes to java.1 from: > > - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage > for obsoletion of RAMFraction flags > - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage > for obsoletion of ScavengeBeforeFullGC > > And a typo crept in to java.1 from: > - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the > Memory-Access Methods in sun.misc.Unsafe for Removal > > This also picks up the unpublished change to keytool.1 from: > > - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in > Supported Named Extensions - BasicContraints > > This also picks up the unpublished change to javadoc.1 from: > > - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should > default @since for a nested class to that of its enclosing class > > and some formatting changes (unclear why - perhaps pandoc version) from the > combined changeset for: > > - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP > 467: Markdown Documentation Comments > - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements > for '///' documentation comments > > The javac.1 file has its copyright year reverted to match what is in the > source file (which should have been updated but wasn't). > > Thanks. Note this is simply a re-generation of the troff files from their sources. If there are any problems that need fixing then a new JBS issue has to be filed to update the source file and regenerate the troff file. - PR Comment: https://git.openjdk.org/jdk/pull/20248#issuecomment-2238248354
Re: RFR: 8334145: missing from vm_memory_map_.txt in System.dump_map help text
On Fri, 19 Jul 2024 01:44:16 GMT, Chris Plummer wrote: > Fix issue with `` argument. Looks good. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20246#pullrequestreview-2187267950
Re: RFR: 8336691: Test LongArgTest.java intermittent fails java.lang.NoClassDefFoundError: jdk/test/lib/Utils
On Thu, 18 Jul 2024 05:37:31 GMT, David Holmes wrote: >> Hi all, >> The testcase `serviceability/attach/LongArgTest.java` intermittent fails >> `java.lang.NoClassDefFoundError: jdk/test/lib/Utils`. Jtreg doesn't >> automatically compile `jdk/test/lib/Utils.class` and >> `jdk/test/lib/apps/LingeredApp.class` etc.. Maybe it's a jtreg framework >> bug. >> I think it's necessory to compile `jdk.test.lib.Utils` and >> `jdk.test.lib.apps.LingeredApp` explicitly. >> Only change the testcase, the change has been verified, no risk. > > Grepping over all tests I see 666 that import jdk.test.lib.Utils but only 60 > actually build it. Looking just at hotspot we have 413 imports and only 1 > build! For jdk.test.lib.apps.LingeredApp it is 91 versus 4, in hotspot. > > I wonder if this is the jtreg issue where compile-on-demand classes are being > put in the wrong directory depending on which test in a run first needs it? > @dholmes-ora and @alexmenkov should comment in this. Also see my comments > that I just added to the CR. Thanks for the link to https://bugs.openjdk.org/browse/CODETOOLS-7902847 - PR Comment: https://git.openjdk.org/jdk/pull/20228#issuecomment-2235550005
Re: RFR: 8336691: Test LongArgTest.java intermittent fails java.lang.NoClassDefFoundError: jdk/test/lib/Utils
On Thu, 18 Jul 2024 01:49:54 GMT, SendaoYan wrote: > Hi all, > The testcase `serviceability/attach/LongArgTest.java` intermittent fails > `java.lang.NoClassDefFoundError: jdk/test/lib/Utils`. Jtreg doesn't > automatically compile `jdk/test/lib/Utils.class` and > `jdk/test/lib/apps/LingeredApp.class` etc.. Maybe it's a jtreg framework bug. > I think it's necessory to compile `jdk.test.lib.Utils` and > `jdk.test.lib.apps.LingeredApp` explicitly. > Only change the testcase, the change has been verified, no risk. Grepping over all tests I see 666 that import jdk.test.lib.Utils but only 60 actually build it. Looking just at hotspot we have 413 imports and only 1 build! For jdk.test.lib.apps.LingeredApp it is 91 versus 4, in hotspot. I wonder if this is the jtreg issue where compile-on-demand classes are being put in the wrong directory depending on which test in a run first needs it? - PR Comment: https://git.openjdk.org/jdk/pull/20228#issuecomment-2235518097
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 102:
> 100: assert(*value != nullptr, "must be");
> 101: return (*value)->object_is_cleared();
> 102: }
The `is_dead` functions seem oddly placed given they do not relate to the
object stored in the wrapper. Why are they here? And what is the difference
between `object_is_cleared` and `object_is_dead` (as used by `LookupMonitor`) ?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 105:
> 103: };
> 104:
> 105: class LookupMonitor : public StackObj {
I'm not understanding why we need this little wrapper class.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680526331
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680526868
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 60:
> 58:
> 59: // ConcurrentHashTable storing links from objects to ObjectMonitors
> 60: class ObjectMonitorWorld : public CHeapObj {
OMWorld describes the project not the hashtable, this should be called
ObjectMonitorTable or some such.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 62:
> 60: class ObjectMonitorWorld : public CHeapObj {
> 61: struct Config {
> 62: using Value = ObjectMonitor*;
Does this alias really help? We don't state the type that many times and it
looks odd to end up with a mix of `Value` and `ObjectMonitor*` in the same code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680508685
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680508801
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with 10
> additional commits since the last revision:
>
> - Remove try_read
> - Add explicit to single parameter constructors
> - Remove superfluous access specifier
> - Remove unused include
> - Update assert message OMCache::set_monitor
> - Fix indentation
> - Remove outdated comment LightweightSynchronizer::exit
> - Remove logStream include
> - Remove strange comment
> - Fix javaThread include
src/hotspot/share/runtime/deoptimization.cpp line 1641:
> 1639: assert(fr.is_deoptimized_frame(), "frame must be
> scheduled for deoptimization");
> 1640: if (LockingMode == LM_LEGACY) {
> 1641:
> mon_info->lock()->set_displaced_header(markWord::unused_mark());
In the existing code how is this restricted to the LM_LEGACY case?? It appears
to be unconditional which suggests you are changing the non-UOMT LM_LIGHTWEIGHT
logic. ??
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680500696
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Wed, 17 Jul 2024 06:39:14 GMT, David Holmes wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with 10 >> additional commits since the last revision: >> >> - Remove try_read >> - Add explicit to single parameter constructors >> - Remove superfluous access specifier >> - Remove unused include >> - Update assert message OMCache::set_monitor >> - Fix indentation >> - Remove outdated comment LightweightSynchronizer::exit >> - Remove logStream include >> - Remove strange comment >> - Fix javaThread include > > src/hotspot/share/runtime/basicLock.hpp line 46: > >> 44: // Used as a cache the ObjectMonitor* used when locking. Must either >> 45: // be nullptr or the ObjectMonitor* used when locking. >> 46: volatile uintptr_t _metadata; > > The displaced header/markword terminology was very well known to people, > whereas "metadata" is really abstract - people will always need to go and > find out what it actually refers to. Could we not define a union here to > support the legacy and lightweight modes more explicitly and keep the > existing terminology for the setters/getters for the code that uses it? I should have read ahead. I see you do keep the setters/getters. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680497748
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas wrote: >> When inflating a monitor the `ObjectMonitor*` is written directly over the >> `markWord` and any overwritten data is displaced into a displaced >> `markWord`. This is problematic for concurrent GCs which needs extra care or >> looser semantics to use this displaced data. In Lilliput this data also >> contains the klass forcing this to be something that the GC has to take into >> account everywhere. >> >> This patch introduces an alternative solution where locking only uses the >> lock bits of the `markWord` and inflation does not override and displace the >> `markWord`. This is done by keeping associations between objects and >> `ObjectMonitor*` in an external hash table. Different caching techniques are >> used to speedup lookups from compiled code. >> >> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is >> only supported in combination with the LM_LIGHTWEIGHT locking mode (the >> default). >> >> This patch has been evaluated to be performance neutral when >> `UseObjectMonitorTable` is turned off (the default). >> >> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` >> and `UseObjectMonitorTable` works. >> >> # Cleanups >> >> Cleaned up displaced header usage for: >> * BasicLock >> * Contains some Zero changes >> * Renames one exported JVMCI field >> * ObjectMonitor >> * Updates comments and tests consistencies >> >> # Refactoring >> >> `ObjectMonitor::enter` has been refactored an a >> `ObjectMonitorContentionMark` witness object has been introduced to the >> signatures. Which signals that the contentions reference counter is being >> held. More details are given below in the section about deflation. >> >> The initial purpose of this was to allow `UseObjectMonitorTable` to interact >> more seamlessly with the `ObjectMonitor::enter` code. >> >> _There is even more `ObjectMonitor` refactoring which can be done here to >> create a more understandable and enforceable API. There are a handful of >> invariants / assumptions which are not always explicitly asserted which >> could be trivially abstracted and verified by the type system by using >> similar witness objects._ >> >> # LightweightSynchronizer >> >> Working on adapting and incorporating the following section as a comment in >> the source code >> >> ## Fast Locking >> >> CAS on locking bits in markWord. >> 0b00 (Fast Locked) <--> 0b01 (Unlocked) >> >> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to >> avoid inflating by spinning a bit. >> >> If 0b10 (Inflated) is observed or there is to... > > Axel Boldt-Christmas has updated the pull request incrementally with 10 > additional commits since the last revision: > > - Remove try_read > - Add explicit to single parameter constructors > - Remove superfluous access specifier > - Remove unused include > - Update assert message OMCache::set_monitor > - Fix indentation > - Remove outdated comment LightweightSynchronizer::exit > - Remove logStream include > - Remove strange comment > - Fix javaThread include src/hotspot/share/runtime/basicLock.hpp line 46: > 44: // Used as a cache the ObjectMonitor* used when locking. Must either > 45: // be nullptr or the ObjectMonitor* used when locking. > 46: volatile uintptr_t _metadata; The displaced header/markword terminology was very well known to people, whereas "metadata" is really abstract - people will always need to go and find out what it actually refers to. Could we not define a union here to support the legacy and lightweight modes more explicitly and keep the existing terminology for the setters/getters for the code that uses it? - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680496495
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 15 Jul 2024 00:50:30 GMT, Axel Boldt-Christmas wrote: >> When inflating a monitor the `ObjectMonitor*` is written directly over the >> `markWord` and any overwritten data is displaced into a displaced >> `markWord`. This is problematic for concurrent GCs which needs extra care or >> looser semantics to use this displaced data. In Lilliput this data also >> contains the klass forcing this to be something that the GC has to take into >> account everywhere. >> >> This patch introduces an alternative solution where locking only uses the >> lock bits of the `markWord` and inflation does not override and displace the >> `markWord`. This is done by keeping associations between objects and >> `ObjectMonitor*` in an external hash table. Different caching techniques are >> used to speedup lookups from compiled code. >> >> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is >> only supported in combination with the LM_LIGHTWEIGHT locking mode (the >> default). >> >> This patch has been evaluated to be performance neutral when >> `UseObjectMonitorTable` is turned off (the default). >> >> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` >> and `UseObjectMonitorTable` works. >> >> # Cleanups >> >> Cleaned up displaced header usage for: >> * BasicLock >> * Contains some Zero changes >> * Renames one exported JVMCI field >> * ObjectMonitor >> * Updates comments and tests consistencies >> >> # Refactoring >> >> `ObjectMonitor::enter` has been refactored an a >> `ObjectMonitorContentionMark` witness object has been introduced to the >> signatures. Which signals that the contentions reference counter is being >> held. More details are given below in the section about deflation. >> >> The initial purpose of this was to allow `UseObjectMonitorTable` to interact >> more seamlessly with the `ObjectMonitor::enter` code. >> >> _There is even more `ObjectMonitor` refactoring which can be done here to >> create a more understandable and enforceable API. There are a handful of >> invariants / assumptions which are not always explicitly asserted which >> could be trivially abstracted and verified by the type system by using >> similar witness objects._ >> >> # LightweightSynchronizer >> >> Working on adapting and incorporating the following section as a comment in >> the source code >> >> ## Fast Locking >> >> CAS on locking bits in markWord. >> 0b00 (Fast Locked) <--> 0b01 (Unlocked) >> >> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to >> avoid inflating by spinning a bit. >> >> If 0b10 (Inflated) is observed or there is to... > > Axel Boldt-Christmas has updated the pull request incrementally with 10 > additional commits since the last revision: > > - Remove try_read > - Add explicit to single parameter constructors > - Remove superfluous access specifier > - Remove unused include > - Update assert message OMCache::set_monitor > - Fix indentation > - Remove outdated comment LightweightSynchronizer::exit > - Remove logStream include > - Remove strange comment > - Fix javaThread include src/hotspot/share/runtime/basicLock.hpp line 44: > 42: // a sentinel zero value indicating a recursive stack-lock. > 43: // * For LM_LIGHTWEIGHT > 44: // Used as a cache the ObjectMonitor* used when locking. Must either The first sentence doesn't read correctly. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1680492976
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Okay for HotSpot, jdk.hotspot.agent, jdk.jdwp.agent and jdk.management. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2181900237
Re: RFR: 8335921: Fix HotSpot VM build without JVMTI
On Wed, 17 Jul 2024 03:37:36 GMT, Vladimir Kozlov wrote: > Citing David Holmes from bug report: > "We provided the ability to leave out certain VM services (JVMTI, GC's other > than serial, ...) as part of the design of the MinimalVM to support Java SE > Embedded, along with the Compact Profiles of JDK 8. This manifested in the > source code as a set of INCLUDE_XXX ifdef guards. The build system later > exposed these as individual --with-jvm-features=xxx,yyy support. However, it > was never intended (and certainly not tested) that you could mix-and-match > arbitrary subsets of these VM features at will. Consequently if you start > trying to do this you will find things that need fixing." > > I added `INCLUDE_JVMTI` guards in two places where it was missed: JVMCI and > JFR. Affected code was added recently, in the past year. After that I was > able to build VM on all supported platforms. > > Note: building VM without JVMTI is not officially supported feature. We are > not testing it and such failures (missing guards) are not unexpected. > > A lot of tests failed with VM without JVMTI. All are expected failures. I > listed failed tests in bug report. > I fixed (added requires `vm.jvmti`) only one which was part of > [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967) changes which > introduced JFR code without `INCLUDE_JVMTI` guards. > > I ran 2 rounds of testing: > > First, only **tier1** with VM built without JVMTI to see if builds passed and > which tests affected. I wrote comment in bug report which tests failed (all > expected to fail without JVMTI). > > Second round of testing with JVMTI in VM: tier1-4 This seems reasonable to me. It highlights the problem we have with optional components in that you either have to work things so that semantically we have a do-nothing implementation of that component, or else you have to put the guards around every piece of code that would normally interact with it. Thanks. src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.hpp line 35: > 33: JfrJvmtiAgent(); > 34: ~JfrJvmtiAgent(); > 35: static bool create() NOT_JVMTI_RETURN_(true); It initially seemed odd to return `true` here, but looking through the JFR code that interacts with the Agent it seems the right way to view this is that without JVMTI we have a no-op agent. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20209#pullrequestreview-2181875380 PR Review Comment: https://git.openjdk.org/jdk/pull/20209#discussion_r1680403451
Re: RFR: 8336254: Virtual thread implementation + test updates
On Thu, 11 Jul 2024 17:30:21 GMT, Alan Bateman wrote: > Bringover some of the changes accumulated in the loom repo to the main line, > most of these changes are test updates and have been baking in the loom repo > for several months. The motive is partly to reduce the large set of changes > that have accumulated in the loom repo, and partly to improve robustness and > test coverage in the main line. The changes don't include any of the larger > changes in the loom repo that are part of future JEPs. > > Implementation: > - Robustness improvements to not throw OOME when unparking a virtual thread. > - Robustness improvements to reduce class loading when a virtual thread parks > or parks when pinned (jdk.internal.misc.VirtualThreads is removed, > jdk.internal.event.VirtualThreadPinnedEvent is eagerly loaded) > - VirtualThread changes to reduce contention on timer queues when doing > timed-park > > Tests: > - New tests for monitor enter/exit/wait/notify (this is a subset of the tests > in the loom repo, we can't move many tests because they depend on on the > changes to the object monitor implementation) > - New test infrastructure to allow tests use a custom scheduler. This updates > many tests to make use of this infrastructure, the "local" ThreadBuidlers is > removed. > - More test scenarios added to ThreadAPI and JVMTI GetThreadStateTest.java > - New test for ThreadMXBean.getLockedMonitor with synchronized native methods > - Reimplement of JVMTI VThreadEvent test to improve reliability > - Rename some tests to get consistent naming > - Diagnostic output in several stress tests to help trace progress in the > event of a timeout > > Testing: tier1-6 src/java.base/share/classes/java/lang/VirtualThread.java line 273: > 271: // current thread is a ForkJoinWorkerThread so the task > will be pushed > 272: // to the local queue. For other schedulers, it avoids > deadlock that > 273: // would arise due to platform and virtual threads > contenting for a s/contenting/contending/ - PR Review Comment: https://git.openjdk.org/jdk/pull/20143#discussion_r1678800192
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v4]
On Mon, 15 Jul 2024 10:21:23 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which proposes to fix the >> test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? >> >> The JBS issue has comments which explains what causes the timeout. The >> commit in this PR addresses those issues by updating the test specific >> `ClassFileTransformer` to only instrument application specific class instead >> of all (core) classes. The test was introduced several years back to verify >> the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As >> such, the proposed changes in this PR continue to test that feature - we now >> merely use application specific class' native method to verify the semantics >> of that feature. >> >> Additional cleanups have been done in the test to make sure that if any >> exception does occur in the ClassFileTransformer then it does get recorded >> and that then causes the test to fail. >> >> With this change, I have run tier1 through tier6 and the test passes. >> Additionally, without this change I've run the test with a test repeat of >> 100 with virtual threads enabled and the test hangs occasionally (as >> expected). With this proposed fix, I have then run the test with virtual >> threads, around 300 times and it hasn't failed or hung in any of those >> instances. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > update code comment in test Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2178701028
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out [v3]
On Mon, 15 Jul 2024 09:04:24 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which proposes to fix the >> test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? >> >> The JBS issue has comments which explains what causes the timeout. The >> commit in this PR addresses those issues by updating the test specific >> `ClassFileTransformer` to only instrument application specific class instead >> of all (core) classes. The test was introduced several years back to verify >> the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As >> such, the proposed changes in this PR continue to test that feature - we now >> merely use application specific class' native method to verify the semantics >> of that feature. >> >> Additional cleanups have been done in the test to make sure that if any >> exception does occur in the ClassFileTransformer then it does get recorded >> and that then causes the test to fail. >> >> With this change, I have run tier1 through tier6 and the test passes. >> Additionally, without this change I've run the test with a test repeat of >> 100 with virtual threads enabled and the test hangs occasionally (as >> expected). With this proposed fix, I have then run the test with virtual >> threads, around 300 times and it hasn't failed or hung in any of those >> instances. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > better formatting for transform() method of the test's agent Okay. Still seems to be a lot more changes than needed for the key update, but so be it. Thanks test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 50: > 48: // It assumes that a specific non-native library method will call a > specific > 49: // native method. The below may need to be updated based on library > changes. > 50: static String goldenNativeMethodName = "fooBarNativeMethod"; The comment block above seems no longer applicable now this is not a library class method. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2177321848 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677591632
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Fri, 12 Jul 2024 09:41:04 GMT, Kevin Walls wrote: > The jump is from the fact that nobody currently creates an empty CmdLine, to > a ruling that nobody ever can in the future. No my point is either it is impossible to create an empty cmdline or else we should have a test that introduces one. If you cannot write such a test then it must be impossible. Hence we can assert that it cannot be empty. If worried about some future use then add a comment/check that would catch it. - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2227618338
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Fri, 12 Jul 2024 09:22:54 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to fix the > test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? > > The JBS issue has comments which explains what causes the timeout. The commit > in this PR addresses those issues by updating the test specific > `ClassFileTransformer` to only instrument application specific class instead > of all (core) classes. The test was introduced several years back to verify > the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As > such, the proposed changes in this PR continue to test that feature - we now > merely use application specific class' native method to verify the semantics > of that feature. > > Additional cleanups have been done in the test to make sure that if any > exception does occur in the ClassFileTransformer then it does get recorded > and that then causes the test to fail. > > With this change, I have run tier1 through tier6 and the test passes. > Additionally, without this change I've run the test with a test repeat of 100 > with virtual threads enabled and the test hangs occasionally (as expected). > With this proposed fix, I have then run the test with virtual threads, around > 300 times and it hasn't failed or hung in any of those instances. @jaikiran there is a lot of unrelated refactoring and style changes here that obscures what the actual fix is. - PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2176774069
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Fri, 12 Jul 2024 03:04:06 GMT, Serguei Spitsyn wrote: > Do I understand this right that the suggestion is to add an explicit runtime > check for non-emptiness and report an error if an empty line has been > discovered? If so, then the `is_empty()`check can be removed from the > `CmdLine::is_executable()`. Basically yes. If the line can never be empty then assert that is the case. - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2224379855
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent [v3]
On Thu, 11 Jul 2024 22:36:05 GMT, Chris Plummer wrote:
>> The test hits a breakpoint on thread2 with SUSPEND_EVENT_THREAD policy, so
>> only thread2 is suspended. It then does a vm.suspend(), which suspends all
>> threads and bumps the suspendCount of thread2 up to 2. It then does an
>> eventSet.resume(), which decrements the thread2 suspendCount to 1, so now
>> all threads are suspended with a suspendCount of 1. thread2 is then resumed
>> and we expect to hit the next thread2 breakpoint. The problem is that
>> thread2 can't hit the breakpoint until the main thread has proceeded far
>> enough, and if the vm.suspend() that suspended the main thread happens too
>> quickly, it won't have proceeded far enough, so thread2 never hits the
>> breakpoint.
>>
>> Essentially we need a vm.resume() to allow the main thread to run, but we
>> need to do it in a way that does nullify part of what the test is testing
>> for. So in order to allow the vm.resume() but not subvert the intent of the
>> test, we first do a thread2.suspend() so the vm.resume() won't resume
>> thread2.
>>
>> Testing in progress: tier1 and tier5 svc testing
>
> Chris Plummer has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix copyright and jcheck error
FWIW I think the explicit sync with the mainthread seems reasonable too.
test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/resume/resume001.java
line 382:
> 380: if (expresult != returnCode0) {
> 381: vm.resume();
> 382: vm.resume(); // for case error when both VirtualMachine
> and the thread2 were suspended
Pre-existing but I don't understand the comment. Why would you need 2
`vm.resume()` here? If `thread2` was suspended directly don't you need a
`thread2.resume()`?
-
PR Review: https://git.openjdk.org/jdk/pull/20088#pullrequestreview-2173568846
PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1675036756
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent
On Thu, 11 Jul 2024 04:43:49 GMT, Chris Plummer wrote: >>> mainThead is suspended and is holding a println related lock, and thread2 >>> is stuck on the 2nd log call in runt2 awaiting the same lock. >> >> The classic example of why suspension is fraught with peril - the target >> must be guaranteed not to be holding any resource needed by the suspender. I >> think removing the logging may be the best bet here - with comments in the >> code to ensure someone does not add it back. Or else use a more primitive >> (native?) mechanism to do the logging, not System.out.println(). > > Which logging should be removed? Alex suggest the logging between breakpoints > 2 and 3, but even that is not enough. There is logging after breakpoint 3, > and that happens before the vm.resume() is done. I'm not saying this can't be > done right, but I think pruning out logging like this in order to fix the > problem not only removes some valuable logging from the test, but still > leaves us open to this type of issue. > > I think the safer thing to do is to make sure mainThread is no longer logging > (or will attempt to log) when the vmsuspend is done. This could be done by > pruning some of its logging, but that has the same negatives as removing some > thread2 logging. My sleep suggestion is by far the simplest. The "purist" fix > would probably be the checkpoint approach (don't do the vm.suspend until > mainThread has reached a stable point). That ended up getting a bit uglier > than I had hoped, but I can finish up the work so you can have a look at it > if you'd like. Sorry I'm unclear on the different threads involved here. IIUC the vm.suspend comes from the debugger, and the mainthread and thread-2 are both threads in the debuggee, being suspended at different times? - PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1673522390
Re: RFR: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent
On Wed, 10 Jul 2024 23:09:05 GMT, Chris Plummer wrote: >> Adding a `sleep()` between two statements does not scale when the test in >> question is under different loads or run with different options. All it will >> do >> is make a hang more rare (and frustrating to analyze). >> >> We do use short sleeps when we are in a while-loop checking on a condition >> of some sort that indicates that the "thing" for which we are waiting to >> happen >> has happened. > > I believe we've done quite a few short sleeps like this in the past. Scaling > is not really an issue. It should only require at most a few ms, even with > -Xcomp, so we wait 1000ms and then never have to think about the timing > again. This test is ugly. Sometimes you have to fight ugly with ugly. > mainThead is suspended and is holding a println related lock, and thread2 is > stuck on the 2nd log call in runt2 awaiting the same lock. The classic example of why suspension is fraught with peril - the target must be guaranteed not to be holding any resource needed by the suspender. I think removing the logging may be the best bet here - with comments in the code to ensure someone does not add it back. Or else use a more primitive (native?) mechanism to do the logging, not System.out.println(). - PR Review Comment: https://git.openjdk.org/jdk/pull/20088#discussion_r1673290543
Re: RFR: 8335743: jhsdb jstack cannot print some information on the waiting thread [v2]
On Mon, 8 Jul 2024 12:16:51 GMT, KIRIYAMA Takuya wrote:
>> This bug was introduced by JDK-8284161. "Object.wait (long timeoutMillis)"
>> was changed to call "Object.wait0 (long timeoutMillis)" in JDK-8284161.
>>
>> When "jhdsb jstack" is executed, the stack and lock information are printed
>> in "sun.jvm.hotspot.runtime.JavaVFrame.printLockInfo(PrintStream tty, int
>> frameCount)". This method checks whether the method is java.lang.Object.wait
>> (...) and then reports the waitstate only if the check passes.
>> https://github.com/openjdk/jdk/blob/7bc8f9c150cbf457edf6144adba734ecd5ca5a0f/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java#L103
>>
>>
>> if (getMethod().getName().asString().equals("wait") &&
>>
>> getMethod().getMethodHolder().getName().asString().equals("java/lang/Object"))
>> {
>>
>>
>> However, after JDK-8284161, the waiting thread waits on
>> "java.lang.Object.wait0 (long timeoutMillis)", so it no longer reports the
>> waitstate.
>>
>> I changed "printLockInfo(PrintStream tty, int frameCount)" to check for
>> "java.lang.Object.wait0 (...)".
>> I confirmed that the lock information is correctly printed with this fix.
>> I tested hotspot/jtreg/serviceability/sa/ and
>> jdk/sun/tools/jhsdb/heapconfig/, and there were no regressions by this fix.
>>
>> Would anyone review this change, please?
>
> KIRIYAMA Takuya has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8335743: jhsdb jstack cannot print some information on the waiting thread
Update looks good. Thanks
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20049#pullrequestreview-2168167270
Re: RFR: 8334777: Test javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java failed with NullPointerException [v2]
On Fri, 28 Jun 2024 18:14:59 GMT, Kevin Walls wrote: >> Disable running this test with -Xcomp. >> >> The NPE seen in this test is due to a timeout establishing the connection. >> ServerCommunicatorAdmin hits its timeout, during an addNotificationListener >> call on a new connection. >> >> -Xcomp causes this slowdown and the failure is reproducible. There is no >> need to test this test with -Xcomp, we should exclude it as we do for >> various other tests. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > comment Okay - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19930#pullrequestreview-2165445432
Re: RFR: 8335743: jhsdb jstack cannot print some information on the waiting thread
On Fri, 5 Jul 2024 09:23:21 GMT, KIRIYAMA Takuya wrote:
> This bug was introduced by JDK-8284161. "Object.wait (long timeoutMillis)"
> was changed to call "Object.wait0 (long timeoutMillis)" in JDK-8284161.
>
> When "jhdsb jstack" is executed, the stack and lock information are printed
> in "sun.jvm.hotspot.runtime.JavaVFrame.printLockInfo(PrintStream tty, int
> frameCount)". This method checks whether the method is java.lang.Object.wait
> (...) and then reports the waitstate only if the check passes.
> https://github.com/openjdk/jdk/blob/7bc8f9c150cbf457edf6144adba734ecd5ca5a0f/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaVFrame.java#L103
>
>
> if (getMethod().getName().asString().equals("wait") &&
>
> getMethod().getMethodHolder().getName().asString().equals("java/lang/Object"))
> {
>
>
> However, after JDK-8284161, the waiting thread waits on
> "java.lang.Object.wait0 (long timeoutMillis)", so it no longer reports the
> waitstate.
>
> I changed "printLockInfo(PrintStream tty, int frameCount)" to check for
> "java.lang.Object.wait0 (...)".
> I confirmed that the lock information is correctly printed with this fix.
> I tested hotspot/jtreg/serviceability/sa/ and
> jdk/sun/tools/jhsdb/heapconfig/, and there were no regressions by this fix.
>
> Would anyone review this change, please?
Good catch on the underlying issue. I think the test needs an adjustment though.
Thanks
test/hotspot/jtreg/serviceability/sa/LingeredAppWithLock.java line 54:
> 52: Thread objectLock = new Thread(() -> lockMethod(classLock1));
> 53: Thread primitiveLock = new Thread(() -> lockMethod(int.class));
> 54: Thread objectWait = new Thread(() -> waitMethod(new Object()));
Use of `new Object()` could be problematic here as the JIT can recognise that
this is a non-escaping object and elide the synchronization.
-
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20049#pullrequestreview-2162420583
PR Review Comment: https://git.openjdk.org/jdk/pull/20049#discussion_r1668084668
Re: RFR: 8335643: serviceability/dcmd/vm tests fail for ZGC after JDK-8322475 [v3]
On Fri, 5 Jul 2024 10:56:37 GMT, Thomas Stuefe wrote: >> The new System.map facility fails its tests when the JVM is using ZGC. The >> facility is working fine, but the test checks for the java heap to appear as >> committed non-shared memory segment, but on ZGC we reserve the memory as >> shared memory. > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > problemlist Seems okay. Thanks for fixing. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20034#pullrequestreview-2162410407
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" My concern is that the logic was wrong and so you fixed it, but this then screams out for a test that would have detected the error, but you can't write a test because the line can never be empty, so that becomes an invariant rather than a runtime check. - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2209631591
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" But if it can't be empty can we not just assert that and get rid of the is_empty check from is_executable? - PR Comment: https://git.openjdk.org/jdk/pull/20006#issuecomment-2208516938
Re: RFR: 8335610: DiagnosticFramework: CmdLine::is_executable() correction
On Wed, 3 Jul 2024 13:58:51 GMT, Kevin Walls wrote: > CmdLine::is_executable() looks wrong, surely an empty line is not executable. > > With this change, in DCmd::parse_and_execute() we will avoid needlessly > entering the code block to try and execute the command. > > DCmd tests all good: > make images test TEST="test/hotspot/jtreg/serviceability/dcmd > test/jdk/sun/tools/jcmd" Can a line ever be empty? If so we should have a test case for that. - PR Review: https://git.openjdk.org/jdk/pull/20006#pullrequestreview-2157983442
Re: RFR: 8331385: G1: Prefix HeapRegion helper classes with G1
On Mon, 1 Jul 2024 09:35:00 GMT, Thomas Schatzl wrote: > Hi all, > > after [JDK-8330694](https://bugs.openjdk.org/browse/JDK-8330694) which > renamed HeapRegion to G1HeapRegion, there were a few related helper classes > in this CR that were not renamed. > > It's purely mechanical renaming without even further renaming of files etc. > > This change updates them. > > (Fwiw, the "Viewed" checkbox at the top right of the file change helps a lot > review this change incrementally) > > Testing: tier1, tier4, tier5 > > Thanks, > Thomas Seems okay. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19967#pullrequestreview-2155206936
Re: [jdk23] RFR: 8335134: Test com/sun/jdi/BreakpointOnClassPrepare.java timeout
On Mon, 1 Jul 2024 17:10:44 GMT, Chris Plummer wrote: > Clean backport. Need to backport this since it was introduced with the new > test in [JDK-8333542](https://bugs.openjdk.org/browse/JDK-8333542), which has > also been backpoted to 23. Backport looks accurate. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19976#pullrequestreview-2152315588
Re: RFR: 8335154: jcmd VM.classes -verbose=false does not set verbose to false
On Wed, 26 Jun 2024 10:23:45 GMT, Kevin Walls wrote: > VM.classes uses: > > 995VM_PrintClasses vmop(output(), _verbose.is_set()); > > _verbose.is_set() is wrong: it could be set, but set to false. > > _verbose.value() should be used (see other examples such as > StringtableDCmd::execute). > > With this change -verbose=false will turn off verbose mode like all other > DCmds which accept -verbose > > bash-4.2$ jcmd 20193 VM.classes -verbose=false | wc -l > 2490 > bash-4.2$ jcmd 20193 VM.classes -verbose=true | wc -l > 90258 Looks good. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19901#pullrequestreview-2141698713
Re: RFR: 8335154: jcmd VM.classes -verbose=false does not set verbose to false
On Wed, 26 Jun 2024 12:40:07 GMT, Kevin Walls wrote:
>> src/hotspot/share/services/diagnosticCommand.cpp line 995:
>>
>>> 993:
>>> 994: void ClassesDCmd::execute(DCmdSource source, TRAPS) {
>>> 995: VM_PrintClasses vmop(output(), _verbose.value());
>>
>> What if it wasn't set?
>
> It has a default value... Generally "false" for _verbose DCmdArguments.
Ok
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19901#discussion_r1654750758
Re: RFR: 8335154: jcmd VM.classes -verbose=false does not set verbose to false
On Wed, 26 Jun 2024 10:23:45 GMT, Kevin Walls wrote:
> VM.classes uses:
>
> 995VM_PrintClasses vmop(output(), _verbose.is_set());
>
> _verbose.is_set() is wrong: it could be set, but set to false.
>
> _verbose.value() should be used (see other examples such as
> StringtableDCmd::execute).
>
> With this change -verbose=false will turn off verbose mode like all other
> DCmds which accept -verbose
>
> bash-4.2$ jcmd 20193 VM.classes -verbose=false | wc -l
> 2490
> bash-4.2$ jcmd 20193 VM.classes -verbose=true | wc -l
> 90258
src/hotspot/share/services/diagnosticCommand.cpp line 995:
> 993:
> 994: void ClassesDCmd::execute(DCmdSource source, TRAPS) {
> 995: VM_PrintClasses vmop(output(), _verbose.value());
What if it wasn't set?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19901#discussion_r1654726465
Re: RFR: 8334287: Man page update for jstatd deprecation
On Fri, 21 Jun 2024 14:05:51 GMT, Kevin Walls wrote: > Man page update for JDK-8327793 which marked jstatd as deprecated for removal > in JDK 24. Sorry this got left "pending" yesterday src/jdk.jstatd/share/man/jstatd.1 line 49: > 47: future release. > 48: .PP > 49: \f[B]Note:\f[R] This command is experimental and unsupported. Can we combine the new warning with the old note e.g. > This command is deprecated and will be removed in a future release. It was > designated as experimental and unsupported. ? - PR Review: https://git.openjdk.org/jdk/pull/19829#pullrequestreview-2134703147 PR Review Comment: https://git.openjdk.org/jdk/pull/19829#discussion_r1650378180
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual [v4]
On Wed, 19 Jun 2024 07:01:49 GMT, Alan Bateman wrote: > the thread dump can happen at times when there isn't a reference to the > virtual Thread I don't follow. We have checked vt is not null and not the carrier, so we do have it. - PR Comment: https://git.openjdk.org/jdk/pull/19744#issuecomment-2177956108
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual [v4]
On Tue, 18 Jun 2024 11:51:31 GMT, Inigo Mediavilla Saiz wrote: >> Follow up to https://github.com/openjdk/jdk/pull/19482 that was causing >> issues when the PrintMountedVirtualTest.java was >> running with `JTREG_TEST_THREAD_FACTORY=Virtual` in the loom repo. >> >> - Fixes issues where the test observes the thread during transitions. >> - Fixes a potential issue in the test where CountDownLatch.countDown unparks >> the main (virtual) thread and the main thread observes the dummy thread is >> transition . > > Inigo Mediavilla Saiz has updated the pull request incrementally with one > additional commit since the last revision: > > Remove thread name from the output Okay. I still have some general concerns about the underlying implementation issues, but as long as the test will now pass reliably that is okay. FWIW I would have kept the vt name just in case it does have one. But as Alan said that can be added in later if desired. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19744#pullrequestreview-2127148088
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual [v2]
On Mon, 17 Jun 2024 13:25:07 GMT, Inigo Mediavilla Saiz wrote: >> `started.countDown()` is replaced with `started.set(true)` to remove the >> possibility that "Dummy Vthread" unmounts here. > > @AlanBateman explained to me why that is possible in > https://github.com/openjdk/jdk/pull/19482#issuecomment-2166116062 and > https://github.com/openjdk/jdk/pull/19482#issuecomment-2167374446. > > Alan can correct me if I'm wrong, but I think that the dummy thread trying to > unpark the main (virtual) thread needs to run as the carrier to avoid nested > parking. See: > https://github.com/openjdk/jdk/blob/412e306d81209c05f55aee7663f7abb80286e361/src/java.base/share/classes/java/lang/VirtualThread.java#L707 So IIUC you've changed the test to try to avoid the problem that was causing the test to fail, as well as changing the code so the test would not fail. But now we won't/may-not actually test that. ?? I guess I want to see that the original failing test now passes with the fix. - PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1644149144
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual [v2]
On Mon, 17 Jun 2024 13:24:23 GMT, Inigo Mediavilla Saiz
wrote:
>> Follow up to https://github.com/openjdk/jdk/pull/19482 that was causing
>> issues when the PrintMountedVirtualTest.java was
>> running with `JTREG_TEST_THREAD_FACTORY=Virtual` in the loom repo.
>>
>> - Fixes issues where the test observes the thread during transitions.
>> - Fixes a potential issue in the test where CountDownLatch.countDown unparks
>> the main (virtual) thread and the main thread observes the dummy thread is
>> transition .
>
> Inigo Mediavilla Saiz has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve assert and document conditional print
src/hotspot/share/runtime/threads.cpp line 1336:
> 1334: assert(vt != nullptr, "vthread should not be null when
> vthread is mounted");
> 1335: if (vt != thread_oop) {
> 1336: // JavaThread._vthread can refer to the carrier thread.
> Print only if _vthread refers to a virtual thread.
Nit: please put the comment before the `if`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1644146331
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual
On Mon, 17 Jun 2024 09:13:57 GMT, Inigo Mediavilla Saiz
wrote:
> Follow up to https://github.com/openjdk/jdk/pull/19482 that was causing
> issues when the PrintMountedVirtualTest.java was
> running with `JTREG_TEST_THREAD_FACTORY=Virtual` in the loom repo.
>
> - Fixes issues where the test observes the thread during transitions.
> - Fixes a potential issue in the test where CountDownLatch.countDown unparks
> the main (virtual) thread and the main thread observes the dummy thread is
> transition .
src/hotspot/share/runtime/threads.cpp line 1334:
> 1332: if (p->is_vthread_mounted()) {
> 1333: const oop vt = p->vthread();
> 1334: if (vt != thread_oop) {
We need a comment explaining why we have to check for this as without knowing
the implementation quirks it seems non-sensical for a thread to have mounted
itself!
src/hotspot/share/runtime/threads.cpp line 1335:
> 1333: const oop vt = p->vthread();
> 1334: if (vt != thread_oop) {
> 1335: assert(vt != nullptr, "vthread should not be null when
> vthread is mounted");
I think the assert still belongs in the original position doesn't it? Or could
the problem we hit here cause a transient null to appear as well?
test/hotspot/jtreg/serviceability/dcmd/thread/PrintMountedVirtualThread.java
line 43:
> 41: public void run(CommandExecutor executor) throws InterruptedException
> {
> 42: var shouldFinish = new AtomicBoolean(false);
> 43: var started = new AtomicBoolean();
Not sure how this change make things better? Why would we want to sleep and
poll rather than park until signalled?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642745542
PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642744456
PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642747708
Re: RFR: 8334164: The fix for JDK-8322811 should use _filename.is_set() rather than strcmp()
On Thu, 13 Jun 2024 16:55:15 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [8334164](https://bugs.openjdk.org/browse/JDK-8334164). > > Testing: > - [x] Verified specifying the literal `vm_memory_map_.txt` as the map > name does not replace ``. > - [x] Verified `` is still appropriately replaced if a file path is not > specified. > > Thanks, > Sonia Seems fine. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19706#pullrequestreview-2121860936
Re: RFR: 8333962: Obsolete OldSize [v2]
On Sat, 15 Jun 2024 08:51:56 GMT, Albert Mingkun Yang wrote: >> src/hotspot/share/runtime/arguments.cpp line 37: >> >>> 35: #include "gc/shared/gcArguments.hpp" >>> 36: #include "gc/shared/gcConfig.hpp" >>> 37: #include "gc/shared/genArguments.hpp" >> >> Why is this needed? > > `Arguments::set_heap_size` accesses `OldSize`, which is declared in this > header. Got it. Thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1642158519
Re: RFR: 8333962: Obsolete OldSize [v2]
On Fri, 14 Jun 2024 10:19:47 GMT, Albert Mingkun Yang wrote: >> Obsolete OldSize and related code. An internal variable `OldSize` is kept to >> capture the capacity of old-gen size. > > Albert Mingkun Yang has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains one commit: > > obsolete-old-size Marked as reviewed by dholmes (Reviewer). src/hotspot/share/runtime/arguments.cpp line 37: > 35: #include "gc/shared/gcArguments.hpp" > 36: #include "gc/shared/gcConfig.hpp" > 37: #include "gc/shared/genArguments.hpp" Why is this needed? - PR Review: https://git.openjdk.org/jdk/pull/19647#pullrequestreview-2120074670 PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1640761871
Re: RFR: 8333962: Obsolete OldSize
On Tue, 11 Jun 2024 08:17:02 GMT, Albert Mingkun Yang wrote:
> Obsolete OldSize and related code. An internal variable `OldSize` is kept to
> capture the capacity of old-gen size.
Al seems reasonable.
Thanks.
src/hotspot/share/runtime/arguments.cpp line 543:
> 541: { "UseNeon", JDK_Version::undefined(),
> JDK_Version::jdk(23), JDK_Version::jdk(24) },
> 542: { "ScavengeBeforeFullGC", JDK_Version::undefined(),
> JDK_Version::jdk(23), JDK_Version::jdk(24) },
> 543:
Please remove blank line. but you need to sync with master to get recent
updates to this table.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19647#pullrequestreview-2117402095
PR Review Comment: https://git.openjdk.org/jdk/pull/19647#discussion_r1639203788
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v18]
On Wed, 12 Jun 2024 08:45:46 GMT, Inigo Mediavilla Saiz wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > Inigo Mediavilla Saiz has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unneeded line I should have asked before sponsoring this but what testing was done with this? Do all our serviceability tests pass with this change? (I guess I will find out later today in our CI.) - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2164224981
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v18]
On Wed, 12 Jun 2024 08:45:46 GMT, Inigo Mediavilla Saiz wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > Inigo Mediavilla Saiz has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unneeded line Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19482#pullrequestreview-2112802397
Re: RFR: 8332400: isspace argument should be a valid unsigned char [v2]
On Tue, 11 Jun 2024 18:07:10 GMT, Robert Toyonaga wrote: >> ### Summary >> This change ensures we don't get undefined behavior when >> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). >> `isspace` accepts an `int` argument that "the application shall ensure is >> a character representable as an unsigned char or equal to the value of the >> macro EOF.". >> >> Previously, there was no checking of the values passed to `isspace`. I've >> replaced direct calls with a new wrapper `os::is_space` that performs a >> range check and prevents the possibility of undefined behavior from >> happening. For instances outside of Hotspot, I've added casts to `unsigned >> char`. >> >> **Testing** >> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check >> `os::is_space` is working correctly. >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > Replace wrapper with casts. Okay. I hate the fact we have to do this, but okay. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2111852338
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v15]
On Tue, 11 Jun 2024 21:05:38 GMT, Inigo Mediavilla Saiz wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > Inigo Mediavilla Saiz has updated the pull request incrementally with one > additional commit since the last revision: > > Require continuations to run the test Sorry for the delay. Leaving the indentation to another PR is fine. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19482#pullrequestreview-2111836480
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Mon, 10 Jun 2024 13:36:06 GMT, Robert Toyonaga wrote: > But what about when an int is passed to isspace? The current casts to int are incorrect as a negative value would be sign-extended and then fail the range check. I think we have to cast to unsigned char in all cases in the caller, which renders the wrapper and range-check obsolete AFAICS. - PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2159586765
Integrated: 8330205: Initial troff manpage generation for JDK 24
On Mon, 10 Jun 2024 02:31:00 GMT, David Holmes wrote: > Sets the version to 24/24-ea and the copyright year to 2025. > > Content changes related to the start of release (e.g. for removed options in > java.1) are handled separately. > > This initial generation also picks up the unpublished changes from: > > - ~~JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC~~ > (covered by merge) > - JDK-8284500: Typo in Supported Named Extensions - BasicContraints > - JDK-8324342: Doclet should default `@since` for a nested class to that of > its enclosing class > > Thanks This pull request has now been integrated. Changeset: 3a01b47a Author:David Holmes URL: https://git.openjdk.org/jdk/commit/3a01b47ac97714608356ce3faf797c37dc63e9af Stats: 43 lines in 28 files changed: 2 ins; 3 del; 38 mod 8330205: Initial troff manpage generation for JDK 24 Reviewed-by: alanb, iris - PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v3]
On Mon, 10 Jun 2024 17:27:18 GMT, Iris Clark wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Regenerated after merge > > Marked as reviewed by iris (Reviewer). Thanks for the review @irisclark . I've merged in the latest updates that also brought in part of the missing changes. If anything goes wrong I have another PR in preparation that also updates java.1 - PR Comment: https://git.openjdk.org/jdk/pull/19617#issuecomment-2159577044
Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v3]
> Sets the version to 24/24-ea and the copyright year to 2025. > > Content changes related to the start of release (e.g. for removed options in > java.1) are handled separately. > > This initial generation also picks up the unpublished changes from: > > - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC > - JDK-8284500: Typo in Supported Named Extensions - BasicContraints > - JDK-8324342: Doclet should default `@since` for a nested class to that of > its enclosing class > > Thanks David Holmes has updated the pull request incrementally with one additional commit since the last revision: Regenerated after merge - Changes: - all: https://git.openjdk.org/jdk/pull/19617/files - new: https://git.openjdk.org/jdk/pull/19617/files/8472c47d..e7563087 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19617=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19617=01-02 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617 PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v2]
> Sets the version to 24/24-ea and the copyright year to 2025. > > Content changes related to the start of release (e.g. for removed options in > java.1) are handled separately. > > This initial generation also picks up the unpublished changes from: > > - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC > - JDK-8284500: Typo in Supported Named Extensions - BasicContraints > - JDK-8324342: Doclet should default `@since` for a nested class to that of > its enclosing class > > Thanks David Holmes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge - 8330205: Initial troff manpage generation for JDK 24 - Changes: https://git.openjdk.org/jdk/pull/19617/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19617=01 Stats: 53 lines in 28 files changed: 12 ins; 3 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/19617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617 PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Fri, 7 Jun 2024 06:07:17 GMT, Thomas Stuefe wrote: > "To use these functions safely with plain chars (or signed chars), the > argument should first be converted to unsigned char" @tstuefe wow! Okay. That is a surprise to me. A cast to unsigned char doesn't actually do anything. Every char is "representable" as an unsigned char because it holds a bit pattern between 0x00 and 0xff i.e. the function is well defined if the incoming int is either EOF (int -1) or else in the range 0x00 to 0xff. But I did a bit of searching and it seems it comes down to potential arithmetic operations on the "char" the might behave differently depending on the signed-ness. :( - PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2157677944
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Mon, 10 Jun 2024 07:15:51 GMT, Alan Bateman wrote: >> Sets the version to 24/24-ea and the copyright year to 2025. >> >> Content changes related to the start of release (e.g. for removed options in >> java.1) are handled separately. >> >> This initial generation also picks up the unpublished changes from: >> >> - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC >> - JDK-8284500: Typo in Supported Named Extensions - BasicContraints >> - JDK-8324342: Doclet should default `@since` for a nested class to that of >> its enclosing class >> >> Thanks > > Marked as reviewed by alanb (Reviewer). Thanks for the review @AlanBateman ! - PR Comment: https://git.openjdk.org/jdk/pull/19617#issuecomment-2157606095
Re: RFR: 8322811: jcmd System.dump_map help info has conflicting statements
On Fri, 7 Jun 2024 10:40:07 GMT, Thomas Stuefe wrote: > @dholmes-ora this is one of yours. > > This was a tad annoying to fix (fix is simple though), since the jcmd > framework has no good way to allow for default parameters that are not used > literally. E.g. in this case, the real value for the file name will contain > the process pid, which of course cannot be hard-coded. > > New output: > > > Syntax : System.dump_map [options] > > Options: (options must be specified using the or = syntax) > -H : [optional] Human readable format (BOOLEAN, false) > -F : [optional] file path (STRING, vm_memory_map_.txt) That seems a good solution. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19596#pullrequestreview-2106891649
RFR: 8330205: Initial troff manpage generation for JDK 24
Sets the version to 24/24-ea and the copyright year to 2025. Content changes related to the start of release (e.g. for removed options in java.1) are handled separately. This initial generation also picks up the unpublished changes from: - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC - JDK-8284500: Typo in Supported Named Extensions - BasicContraints - JDK-8324342: Doclet should default @since for a nested class to that of its enclosing class Thanks - Commit messages: - 8330205: Initial troff manpage generation for JDK 24 Changes: https://git.openjdk.org/jdk/pull/19617/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19617=00 Issue: https://bugs.openjdk.org/browse/JDK-8330205 Stats: 66 lines in 28 files changed: 12 ins; 13 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/19617.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617 PR: https://git.openjdk.org/jdk/pull/19617
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Fri, 7 Jun 2024 19:56:03 GMT, Chris Plummer wrote: > Allow warning messages such as the following to appear in stderr: > > Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option > UseNotificationThread; support is scheduled for removal in 24.0 > > Tested by running CI tier5, which reproduced the issue. This is the wrong fix. The tests should not be being run with the problematic option. We should not be hiding that by extending the "ignore warnings" support. This is an issue with our CI task definitions. - Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19606#pullrequestreview-2106373613
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Fri, 7 Jun 2024 19:56:03 GMT, Chris Plummer wrote:
> Allow warning messages such as the following to appear in stderr:
>
> Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option
> UseNotificationThread; support is scheduled for removal in 24.0
>
> Tested by running CI tier5, which reproduced the issue.
test/lib/jdk/test/lib/process/OutputAnalyzer.java line 187:
> 185: * If stderr was not empty
> 186: */
> 187: public OutputAnalyzer stderrShouldBeEmptyIgnoreDeprecatedWarnings() {
Maybe this should be renamed to reflect that it is about VM option warnings.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19606#discussion_r1631825632
Re: RFR: 8332400: isspace argument should be a valid unsigned char
On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga wrote:
> ### Summary
> This change ensures we don't get undefined behavior when
> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html).
> `isspace` accepts an `int` argument that "the application shall ensure is a
> character representable as an unsigned char or equal to the value of the
> macro EOF.".
>
> Previously, there was no checking of the values passed to `isspace`. I've
> replaced direct calls with a new wrapper `os::is_space` that performs a range
> check and prevents the possibility of undefined behavior from happening. For
> instances outside of Hotspot, I've added casts to `unsigned char`.
>
> **Testing**
> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check
> `os::is_space` is working correctly.
> - tier1
Sorry I think this is well-intentioned but unnecessary in nearly all cases. If
you pass a char there is no potential problem. Only passing an actual int could
be a problem.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 664:
> 662: char after_key = line[key_len];
> 663: if (strncmp(line, key, key_len) == 0
> 664: && os::is_space(after_key) != 0
I think this is excessive. The value is a char so cannot be a problem. The only
calls to isspace that need checking are those that actually pass an int, and
even then there could be adequate safeguards in place.
src/hotspot/os/linux/os_linux.cpp line 1356:
> 1354: if (s) {
> 1355: // Skip blank chars
> 1356: do { s++; } while (s && os::is_space(*s));
Again not needed.
-
PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2103234054
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630265925
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630266490
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v8]
On Tue, 4 Jun 2024 07:25:41 GMT, Inigo Mediavilla Saiz wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > Inigo Mediavilla Saiz has updated the pull request incrementally with two > additional commits since the last revision: > > - Cleanup test > >- Stop virtualthread >- Remove unneeded imports >- Remove modules that are not needed > - Fix copyright year Test updates look okay. Thanks. - PR Review: https://git.openjdk.org/jdk/pull/19482#pullrequestreview-2095581013
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v7]
On Mon, 3 Jun 2024 13:31:16 GMT, Inigo Mediavilla Saiz wrote:
>> Print the stack traces of mounted virtual threads when calling `jcmd
>> Thread.print`.
>
> Inigo Mediavilla Saiz has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Print mounted virtual thread after carrier
FWIW I considered the mounted VT to be effectively part of the carrier thread's
state so it made sense to me to have it's stack first (and it is the more
interesting stack). But if Alan and Ron want it this way so be it.
I don't understand the logic you are using to get the indentation though.
src/hotspot/share/runtime/javaThread.cpp line 1832:
> 1830: st->print("\t");
> 1831: indentation--;
> 1832: }
Suggestion:
while (indentation-- > 0) {
st->print("\t");
}
Though not sure using `\t` is the best way to indent this as stream indentation
is based on spaces.
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
2:
> 1: /*
> 2: * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights
> reserved.
New file should only have current year in copyright notice.
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
24:
> 22: */
> 23:
> 24: import com.beust.ah.A;
???
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
37:
> 35: import java.util.concurrent.atomic.AtomicBoolean;
> 36: import java.util.concurrent.locks.ReentrantLock;
> 37: import java.util.regex.Pattern;
Seems to be a number of unneeded imports here.
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
46:
> 44: * java.compiler
> 45: * java.management
> 46: * jdk.internal.jvmstat/sun.jvmstat.monitor
These don't all seem necessary.
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
52:
> 50:
> 51: public void run(CommandExecutor executor) throws InterruptedException
> {
> 52: var shouldStop = new AtomicBoolean();
You never set this to stop the thread.
-
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19482#pullrequestreview-2095343672
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625365087
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625370894
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625371025
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625371252
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625371685
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625373779
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v7]
On Tue, 4 Jun 2024 05:27:48 GMT, David Holmes wrote:
>> Inigo Mediavilla Saiz has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Print mounted virtual thread after carrier
>
> src/hotspot/share/runtime/javaThread.cpp line 1832:
>
>> 1830: st->print("\t");
>> 1831: indentation--;
>> 1832: }
>
> Suggestion:
>
> while (indentation-- > 0) {
> st->print("\t");
> }
>
> Though not sure using `\t` is the best way to indent this as stream
> indentation is based on spaces.
Actually I'm not sure what this indentation actually does at this location and
its affect on other user's of this API. I would have expected the caller to set
up the necessary indentation in the stream that gets passed in. I see you inc
the indentation but then use the current indentation to insert multiple tabs -
which should not be necessary if the stream indentation works correctly. ???
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1625368549
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Mon, 3 Jun 2024 22:03:13 GMT, Chris Plummer wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: consistency and stylistical corrections > > src/hotspot/share/prims/jvmti.xml line 1007: > >> 1005: explicitly deallocate. This is indicated in the individual >> 1006: function descriptions. Empty lists, arrays, sequences, etc are >> 1007: returned as a null pointer (C NULL or C++ >> nullptr). > > Why describe what is meant by a "null pointer" here when it is not done > elsewhere? The intent is to provide a definition of what a null pointer is, for both C and C++ programs. Is there a better place to do that so that elsewhere the spec can simply to refer to "a null pointer" or "null"? - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1625338781
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v4]
On Mon, 3 Jun 2024 08:31:46 GMT, Thomas Stuefe wrote: > I also find the duplication of the stack printing code unfortunate. It would > be nice to reuse`JavaThread::print_vthread_stack_on`. I don't understand why > it cannot be const? Just what I was about to query :) I'm not sure what the const issue is. Printing a stack certainly should not modify anything. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2144606292
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump
On Fri, 31 May 2024 02:07:25 GMT, David Holmes wrote: >> Print the stack traces of mounted virtual threads when calling `jcmd >> Thread.print`. > > I'd probably give preference to the stack of the virtual thread, as the stack > of the carrier when a vthread is mounted is generally quite uninteresting: > > "ForkJoinPool-1-worker-1" #25 [33795] daemon prio=5 os_prio=31 cpu=46574.42ms > elapsed=47.15s tid=0x7f81670d1a00 [0x7e9a4000] >Carrying virtual thread #24 > at Main.lambda$main$0(Main.java:7) > at java.lang.VirtualThread.run(java.base/VirtualThread.java:381) > at jdk.internal.vm.Continuation.run(java.base/Continuation.java:262) > at > java.lang.VirtualThread.runContinuation(java.base/VirtualThread.java:283) > at > java.lang.VirtualThread$$Lambda/0x0001220b2868.run(java.base/Unknown > Source) > at > java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1726) > at > java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1717) > at > java.util.concurrent.ForkJoinTask$InterruptibleTask.exec(java.base/ForkJoinTask.java:1641) > at > java.util.concurrent.ForkJoinTask.doExec(java.base/ForkJoinTask.java:507) > at > java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(java.base/ForkJoinPool.java:1455) > at > java.util.concurrent.ForkJoinPool.runWorker(java.base/ForkJoinPool.java:2031) > at > java.util.concurrent.ForkJoinWorkerThread.run(java.base/ForkJoinWorkerThread.java:189) > > * The format proposed by @dholmes-ora definitely makes sense > > You will different get different opinions on how it is presented. Can you > also try putting a new section that lists the mounted virtual threads and > their stack trace. If the virtual thread has a name then it can be shown too. That suggests to me that we have to iterate the list of all threads twice ?? If so that seems not ideal. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2144602366
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v4]
On Fri, 31 May 2024 08:07:36 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: more null pointer corrections The general rules are to either say "a null pointer" (possibly with capital A depending on context), or just "null". And in most cases you could choose either. I made various suggestions but really it is up to you. It is hard to get a sense of consistency from these small fragments. The word "null" should never be in code font as it is not a programming language entity. Thanks for your patience and perseverance on this. src/hotspot/share/prims/jvmti.xml line 1101: > 1099: > 1100: On return, a pointer to the beginning of the allocated > memory. > 1101: If size is zero, null pointer is returned. If saying "null pointer" then it should be "a null pointer". src/hotspot/share/prims/jvmti.xml line 1533: > 1531: > 1532: > 1533: On return, points to the current thread, or > null. remove code font src/hotspot/share/prims/jvmti.xml line 1996: > 1994: > 1995: The thread group to which this thread belongs. > 1996: null pointer if the thread has terminated. Just "Null if ..." src/hotspot/share/prims/jvmti.xml line 2142: > 2140: > 2141: On return, filled with the current contended monitor, or > 2142: null pointer if there is none. Just "null" src/hotspot/share/prims/jvmti.xml line 2262: > 2260: > 2261: > 2262: null pointer is passed to the start > function A null ... src/hotspot/share/prims/jvmti.xml line 2353: > 2351: If thread-local storage has not been set with > 2352: > the returned > 2353: pointer is null. Remove code font src/hotspot/share/prims/jvmti.xml line 4277: > 4275: , > 4276: or . > 4277: Otherwise null pointer. a null ... src/hotspot/share/prims/jvmti.xml line 4322: > 4320: points to the zero if the referrer > 4321: object is not tagged. > 4322: null pointer if the referrer in not an object (that is, Null if ... src/hotspot/share/prims/jvmti.xml line 4769: > 4767: > 4768: > 4769: null pointer is passed as the user supplied > data a null pointer src/hotspot/share/prims/jvmti.xml line 4945: > 4943: > 4944: > 4945: null pointer is passed as the user supplied > data a null pointer src/hotspot/share/prims/jvmti.xml line 5593: > 5591: > 5592: > 5593: null pointer is passed as the user supplied > data a null pointer src/hotspot/share/prims/jvmti.xml line 5637: > 5635: callback will not be called until the appropriate callback has > been called > 5636: for all roots. If the id="object_ref_callback"> callback is > 5637: specified as null pointer then this function returns after Just "as null then" src/hotspot/share/prims/jvmti.xml line 5692: > 5690: > 5691: >null pointer is passed as the user supplied > data > 5692: a null pointer src/hotspot/share/prims/jvmti.xml line 5750: > 5748: > 5749: > 5750: null pointer is passed as the user supplied > data a null pointer src/hotspot/share/prims/jvmti.xml line 6770: > 6768: If a named module is defined to the class loader and it > 6769: contains the package then that named module is returned, > 6770: otherwise null pointer is returned. Just "null" src/hotspot/share/prims/jvmti.xml line 6803: > 6801: > 6802: On return, points to a java.lang.Module object > 6803: or points to null. Remove code font src/hotspot/share/prims/jvmti.xml line 7264: > 7262: modified UTF-8 > string. > 7263: If there is no generic signature attribute for the class, > then, > 7264: on return, points to null. Remove code font src/hotspot/share/prims/jvmti.xml line 7794: > 7792: If the class was not created by a class loader > 7793: or if the class loader is the bootstrap class loader, > 7794: points to null. Remove code font src/hotspot/share/prims/jvmti.xml line 8414: > 8412: modified UTF-8 > string. > 8413: If there is
Re: RFR: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive
On Wed, 29 May 2024 18:12:25 GMT, Sonia Zaldana Calles
wrote:
> Hi folks,
>
> This PR addresses [8332785](https://bugs.openjdk.org/browse/JDK-8332785)
> replacing all naked uses for ```UseSharedSpaces``` with
> ```CDSConfig::is_using_archive```.
>
> Testing:
> - [x] Tier 1 with GHA.
>
> Thanks,
> Sonia
One minor nit but otherwise looks good.
Thanks
src/hotspot/share/cds/cdsConfig.cpp line 308:
> 306:
> 307: bool CDSConfig::has_unsupported_runtime_module_options() {
> 308: assert(CDSConfig::is_using_archive(), "this function is only used with
> -Xshare:{on,auto}");
Nit: you shouldn't need to specify `CDSConfig::`
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19463#pullrequestreview-2089898352
PR Review Comment: https://git.openjdk.org/jdk/pull/19463#discussion_r1621718062
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 31 May 2024 01:43:35 GMT, Serguei Spitsyn wrote: > Okay. I've made a fix to replace in the docs nullptr with null pointer as you > suggested. What I suggested was > returns _a_ null pointer in place of > returns `nulllptr` but "a null pointer" doesn't always look right either e.g. "was a null pointer" would be better as just "was null". I think using non-code-font "null" to represent the concept of null-ness would be fine: "returns null", "is null", "was null" - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2141136079
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump
On Thu, 30 May 2024 14:13:34 GMT, Inigo Mediavilla Saiz wrote: > Print the stack traces of mounted virtual threads when calling `jcmd > Thread.print`. I'd probably give preference to the stack of the virtual thread, as the stack of the carrier when a vthread is mounted is generally quite uninteresting: "ForkJoinPool-1-worker-1" #25 [33795] daemon prio=5 os_prio=31 cpu=46574.42ms elapsed=47.15s tid=0x7f81670d1a00 [0x7e9a4000] Carrying virtual thread #24 at Main.lambda$main$0(Main.java:7) at java.lang.VirtualThread.run(java.base/VirtualThread.java:381) at jdk.internal.vm.Continuation.run(java.base/Continuation.java:262) at java.lang.VirtualThread.runContinuation(java.base/VirtualThread.java:283) at java.lang.VirtualThread$$Lambda/0x0001220b2868.run(java.base/Unknown Source) at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1726) at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(java.base/ForkJoinTask.java:1717) at java.util.concurrent.ForkJoinTask$InterruptibleTask.exec(java.base/ForkJoinTask.java:1641) at java.util.concurrent.ForkJoinTask.doExec(java.base/ForkJoinTask.java:507) at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(java.base/ForkJoinPool.java:1455) at java.util.concurrent.ForkJoinPool.runWorker(java.base/ForkJoinPool.java:2031) at java.util.concurrent.ForkJoinWorkerThread.run(java.base/ForkJoinWorkerThread.java:189) - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2141113251
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump
On Thu, 30 May 2024 14:13:34 GMT, Inigo Mediavilla Saiz wrote: > Print the stack traces of mounted virtual threads when calling `jcmd > Thread.print`. Do you look at `JavaThread::print_vthread_stack_on`? The last thing we need is yet-another-version of stack printing code. - PR Review: https://git.openjdk.org/jdk/pull/19482#pullrequestreview-2089761057
Re: RFR: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
On Wed, 29 May 2024 09:09:16 GMT, Matthias Baesken wrote: > When running with ubsan - enabled binaries (--enable-ubsan), > in the vmTestbase/nsk/jdi tests some cases of memset on nullptr destinations > are detected in get_object_monitor_usage . > > // null out memory for robustness > memset(ret.waiters, 0, ret.waiter_count * sizeof(jthread *)); > memset(ret.notify_waiters, 0, ret.notify_waiter_count * sizeof(jthread *)); > > probably we should add checks there. > Example : > vmTestbase/nsk/jdi/ObjectReference/entryCount/entrycount002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1560:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7ffb2568559c in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1560 > debugee.stderr> #1 0x7ffb27987bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7ffb28ddc2dd in VM_Operation::evaluate() > src/hotspot/share/runtime/vmOperations.cpp:75 > debugee.stderr> #3 0x7ffb28deac41 in > VMThread::evaluate_operation(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:283 > debugee.stderr> #4 0x7ffb28decc4f in VMThread::inner_execute(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:427 > debugee.stderr> #5 0x7ffb28ded7b9 in VMThread::loop() > src/hotspot/share/runtime/vmThread.cpp:493 > debugee.stderr> #6 0x7ffb28ded8a7 in VMThread::run() > src/hotspot/share/runtime/vmThread.cpp:177 > debugee.stderr> #7 0x7ffb28b7e31a in Thread::call_run() > src/hotspot/share/runtime/thread.cpp:225 > debugee.stderr> #8 0x7ffb281c4971 in thread_native_entry > src/hotspot/os/linux/os_linux.cpp:846 > debugee.stderr> #9 0x7ffb2df416e9 in start_thread > (/lib64/libpthread.so.0+0xa6e9) (BuildId: > 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) > debugee.stderr> #10 0x7ffb2d51550e in clone (/lib64/libc.so.6+0x11850e) > (BuildId: f732026552f6adff988b338e92d466bc81a01c37) > > vmTestbase/nsk/jdi/ObjectReference/owningThread/owningthread002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1561:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7f1e070855bb in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1561 > debugee.stderr> #1 0x7f1e09387bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7f1e0a7dc2dd in VM_Operation::evaluate() src/hotsp... Okay, sorry, the zero case handling here is a little awkward. But it seems to me that if the counts are zero it is expected that memset does nothing, so the value of the pointer passed in is irrelevant. Shame it doesn't specify that. We should skip allocation and the subsequent memset when the count is zero. - PR Comment: https://git.openjdk.org/jdk/pull/19450#issuecomment-2141034070
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 07:14:13 GMT, Alan Bateman wrote: >> Okay. I still think that should be hidden behind the >> `java_lang_VirtualThread::is_instance` as it is an implementation detail the >> JVMTI and thread code shouldn't need to know about IMO. Once the alternative >> implementation is removed I expect these explicit checks for >> `BaseVirtualThread` will need to be reverted and we could avoid that if we >> make a change now. > > java_lang_VirtualThread::is_instance returning true when the top is not an > instance of that class would be a bit strange. > java_lang_Thread::is_virtual_thread_instance might be less surprising. Yes you are right, we would need a new API to answer the generic "are you a virtual thread" query. - PR Review Comment: https://git.openjdk.org/jdk/pull/19405#discussion_r1620123058
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 06:31:19 GMT, Alan Bateman wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1486:
>>
>>> 1484: if (owning_thread != nullptr) {
>>> 1485: oop thread_oop = get_vthread_or_thread_oop(owning_thread);
>>> 1486: bool is_virtual =
>>> thread_oop->is_a(vmClasses::BaseVirtualThread_klass());
>>
>> It strikes me that this should be handled by
>> `java_lang_VirtualThread::is_instance` based on whether there is
>> continuation support or not. External code like this should not, IMO, needed
>> to know about `BaseVirtualThread`. @AlanBateman what do you think?
>
> Hopefully the ports will catch up someday and the alternative implementation
> can be removed.
>
> We decided not to rename java.lang.VirtualThread when introducing the
> alternative implementation as it's just too disruptive. The super class that
> both implementations extend is BaseVirtualThread so testing for an instance
> of that is correct for the two implementations.
>
> If it helps the readability then introducing a function to test if a thread
> is a virtual thread might help. It could use VMContinuations if needed but
> right now, testing for an instanceof BaseVirtualThread is okay.
Okay. I still think that should be hidden behind the
`java_lang_VirtualThread::is_instance` as it is an implementation detail the
JVMTI and thread code shouldn't need to know about IMO. Once the alternative
implementation is removed I expect these explicit checks for
`BaseVirtualThread` will need to be reverted and we could avoid that if we make
a change now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19405#discussion_r1620065937
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 01:13:20 GMT, SendaoYan wrote:
>> Hi all,
>> ObjectMonitorUsage.java failed with `unexpected waiter_count` after
>> [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32.
>> There are two changes in this PR:
>> 1. In `JvmtiEnvBase::get_object_monitor_usage` function, change from
>> `java_lang_VirtualThread::is_instance(thread_oop)` to
>> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the
>> alternative implementation.
>> 2. In `Threads::get_pending_threads(ThreadsList *, int, address)` function
>> of threads.cpp file, change from
>> `java_lang_VirtualThread::is_instance(thread_oop)` to
>> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the
>> alternative implementation. This modified function only used by
>> `JvmtiEnvBase::get_object_monitor_usage(JavaThread*, jobject,
>> jvmtiMonitorUsage*)`, so the risk of the modified on threads.cpp file is low.
>>
>>
>>
>> Additional testing:
>> - [x] linux x86_32 run all testcases in serviceability/jvmti, all testcases
>> run successed expect
>> `serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java#default`
>> run failed. This test also run failed before this PR, which has been
>> recorded in [JDK-8333140](https://bugs.openjdk.org/browse/JDK-8333140)
>> - [x] linux x86_64 run all testcases in serviceability/jvmti, all testcases
>> run successed.
>
> SendaoYan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change from java_lang_VirtualThread::is_instance(thread_oop) to
> hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in
> Threads::get_pending_threads()
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1486:
> 1484: if (owning_thread != nullptr) {
> 1485: oop thread_oop = get_vthread_or_thread_oop(owning_thread);
> 1486: bool is_virtual =
> thread_oop->is_a(vmClasses::BaseVirtualThread_klass());
It strikes me that this should be handled by
`java_lang_VirtualThread::is_instance` based on whether there is continuation
support or not. External code like this should not, IMO, needed to know about
`BaseVirtualThread`. @AlanBateman what do you think?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19405#discussion_r1620005105
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v5]
On Wed, 29 May 2024 01:18:57 GMT, Serguei Spitsyn wrote:
>> Leonid Mesnik has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - fixed space.
>> - The result is updated.
>
> src/hotspot/share/prims/jvmtiTrace.cpp line 284:
>
>> 282: JavaThreadState current_state =
>> JavaThread::cast(Thread::current())->thread_state();
>> 283: if (current_state == _thread_in_native || current_state ==
>> _thread_blocked) {
>> 284: return "not readable";
>
> Nit: I'd suggest to make it more detailed, something like like this:
> "" or ""
Yes this would have looked better if the text was more clearly an error message
with angle brackets.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19275#discussion_r1619989123
Re: RFR: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
On Wed, 29 May 2024 12:38:21 GMT, Matthias Baesken wrote: >> When running with ubsan - enabled binaries (--enable-ubsan), >> in the vmTestbase/nsk/jdi tests some cases of memset on nullptr destinations >> are detected in get_object_monitor_usage . >> >> // null out memory for robustness >> memset(ret.waiters, 0, ret.waiter_count * sizeof(jthread *)); >> memset(ret.notify_waiters, 0, ret.notify_waiter_count * sizeof(jthread *)); >> >> probably we should add checks there. >> Example : >> vmTestbase/nsk/jdi/ObjectReference/entryCount/entrycount002/TestDescription.jtr >> >> debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1560:11: runtime >> error: null pointer passed as argument 1, which is declared to never be null >> debugee.stderr> #0 0x7ffb2568559c in >> JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, >> jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1560 >> debugee.stderr> #1 0x7ffb27987bd7 in VM_GetObjectMonitorUsage::doit() >> src/hotspot/share/prims/jvmtiEnvBase.hpp:594 >> debugee.stderr> #2 0x7ffb28ddc2dd in VM_Operation::evaluate() >> src/hotspot/share/runtime/vmOperations.cpp:75 >> debugee.stderr> #3 0x7ffb28deac41 in >> VMThread::evaluate_operation(VM_Operation*) >> src/hotspot/share/runtime/vmThread.cpp:283 >> debugee.stderr> #4 0x7ffb28decc4f in VMThread::inner_execute(VM_Operation*) >> src/hotspot/share/runtime/vmThread.cpp:427 >> debugee.stderr> #5 0x7ffb28ded7b9 in VMThread::loop() >> src/hotspot/share/runtime/vmThread.cpp:493 >> debugee.stderr> #6 0x7ffb28ded8a7 in VMThread::run() >> src/hotspot/share/runtime/vmThread.cpp:177 >> debugee.stderr> #7 0x7ffb28b7e31a in Thread::call_run() >> src/hotspot/share/runtime/thread.cpp:225 >> debugee.stderr> #8 0x7ffb281c4971 in thread_native_entry >> src/hotspot/os/linux/os_linux.cpp:846 >> debugee.stderr> #9 0x7ffb2df416e9 in start_thread >> (/lib64/libpthread.so.0+0xa6e9) (BuildId: >> 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) >> debugee.stderr> #10 0x7ffb2d51550e in clone (/lib64/libc.so.6+0x11850e) >> (BuildId: f732026552f6adff988b338e92d466bc81a01c37) >> >> vmTestbase/nsk/jdi/ObjectReference/owningThread/owningthread002/TestDescription.jtr >> >> debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1561:11: runtime >> error: null pointer passed as argument 1, which is declared to never be null >> debugee.stderr> #0 0x7f1e070855bb in >> JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, >> jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1561 >> debugee.stderr> #1 0x7f1e09387bd7 in VM_GetObjectMonitorUsage::doit() >> src/hotspot/share/prims/jvmtiEnvBase.hpp:594 >> debugee.std... > > Hi Martin and Serguei, thanks for the reviews ! @MBaesken This was not proposed as a trivial PR and so is subject to the 24 hour rule. Please don't push these ubsan "fixes" quickly as we need time to assess their validity and the right way to address them. This fix looks wrong to me because those values cannot be null as it implies the `allocate` function failed which means we would not reach this code! - PR Comment: https://git.openjdk.org/jdk/pull/19450#issuecomment-2138540409
Re: RFR: 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local Ref
On Fri, 24 May 2024 20:03:53 GMT, Chris Plummer wrote: > If PointerLocation discovers that an address is for a JNI local ref, it will > print information about the thread that owns the JNI local ref. For > JavaThreads it calls the printThreadIDOn(tty) method. There's a comment on > the call that says that it 'includes "\n"'. This is actually not true, and a > separate println() is needed. I noticed this when using the clhsdb findpc > command on a JNI local ref and noted that the next "hdsb> " prompt was > printed at the end of the findpc output instead of on a new line. Trivially okay. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19402#pullrequestreview-2083983137
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count
On Sun, 26 May 2024 09:27:00 GMT, SendaoYan wrote: > Hi all, > ObjectMonitorUsage.java failed with `unexpected waiter_count` after > [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32. > It should be predicated with `@requires vm.continuations` to be skipped. > > Additional testing: > - [x] linux x86_32, test has been skiped. > - [x] linux x86_64, test still work. Seems to me that test has been using virtual threads, ignoring the vm.continuations setting since before JDK-8328083 was fixed. It is not clear to me how the test was operating before-hand in that case? If it was running on 32-bit Linux without continuations before then skipping it now seems the wrong fix, as the updated test would seem to have some invalid constraints encoded in it. - PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2079895579
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Mon, 20 May 2024 03:23:25 GMT, Chris Plummer wrote: > Unfortunately checking ownership means comparing jobjects, which you can't > safely do if you are comparing to a jobject that could be freed at any moment Okay but how does this `dbgRawMonitor` lock prevent the GC from clearing a jobject? - PR Comment: https://git.openjdk.org/jdk/pull/19044#issuecomment-2119642215
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v4]
On Fri, 17 May 2024 22:31:32 GMT, Leonid Mesnik wrote:
>> The JvmtiTrace::safe_get_thread_name sometimes crashes when called while
>> current thread is in native thread state.
>>
>> It happens when thread_name is set for tracing from jvmti functions.
>> See:
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649
>>
>>
>> The setup is called and the thread name is used in tracing before the thread
>> transition. There is no good location where this method could be called from
>> vm thread_state only. Some functions like raw monitor enter/exit never
>> transition in vm state. So sometimes it is needed to call this function from
>> native thread state.
>>
>> The change should affect JVMTI trace mode only (-XX:TraceJVMTI).
>>
>> Verified by running jvmti/jdi/jdb tests with tracing enabled.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> wrong thread state
Okay now I get it. Even once the function is made truly safe, we are always
calling it from an unsafe state and so will get the default `Thread::name`
response. So now, after any transition to the VM the name is read again to get
a good value. This seems a good enhancement though I have to wonder if the
apparent changing of the thread name in the tracing might cause problems. The
tracing really needs to include a unique thread identifier.
Thanks
src/hotspot/share/prims/jvmtiEventController.cpp line 961:
> 959: JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent event_type,
> bool added) {
> 960: int *count_addr;
> 961:
Nit: this file doesn't need to be touched.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19275#pullrequestreview-2065287663
PR Review Comment: https://git.openjdk.org/jdk/pull/19275#discussion_r1606209679
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Fri, 17 May 2024 19:32:31 GMT, Chris Plummer wrote: >> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Fix a couple of minor typos in comments. > Note we can't check if the current thread owns a lock without grabbing > dbgRawMonitor. In fact the main purpose of dbgRawMonitor is to allow the > current thread to check if it owns the monitor. That smells fishy to me. A thread can always safely check if it owns something because it can't race with itself and get a wrong answer. - PR Comment: https://git.openjdk.org/jdk/pull/19044#issuecomment-2119525715
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v4]
On Fri, 17 May 2024 22:31:32 GMT, Leonid Mesnik wrote: >> The JvmtiTrace::safe_get_thread_name sometimes crashes when called while >> current thread is in native thread state. >> >> It happens when thread_name is set for tracing from jvmti functions. >> See: >> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649 >> >> >> The setup is called and the thread name is used in tracing before the thread >> transition. There is no good location where this method could be called from >> vm thread_state only. Some functions like raw monitor enter/exit never >> transition in vm state. So sometimes it is needed to call this function from >> native thread state. >> >> The change should affect JVMTI trace mode only (-XX:TraceJVMTI). >> >> Verified by running jvmti/jdi/jdb tests with tracing enabled. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > wrong thread state I don't understand the additional changes because they read the current thread's name, whereas this issue is about reading an arbitrary thread's name when the current thread happens to be in the wrong state. ??? - PR Comment: https://git.openjdk.org/jdk/pull/19275#issuecomment-2119378363
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v2]
On Fri, 17 May 2024 02:08:34 GMT, Leonid Mesnik wrote: >> The JvmtiTrace::safe_get_thread_name sometimes crashes when called while >> current thread is in native thread state. >> >> It happens when thread_name is set for tracing from jvmti functions. >> See: >> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649 >> >> >> The setup is called and the thread name is used in tracing before the thread >> transition. There is no good location where this method could be called from >> vm thread_state only. Some functions like raw monitor enter/exit never >> transition in vm state. So sometimes it is needed to call this function from >> native thread state. >> >> The change should affect JVMTI trace mode only (-XX:TraceJVMTI). >> >> Verified by running jvmti/jdi/jdb tests with tracing enabled. > > Leonid Mesnik 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 four additional > commits since the last revision: > > - copyrights updated. > - Merge branch 'master' of https://github.com/openjdk/jdk into 8332259 > - include updated. > - 8332259 I have to wonder whether this solution will potentially cause problems because the code will now block for safepoints. We could fallback to `Thread::name()` if the current thread is in-native. - PR Review: https://git.openjdk.org/jdk/pull/19275#pullrequestreview-2062389545
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 00:43:18 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: corrected the nullptr clarification But this clarification doesn't actually clarify that the rest of the spec uses `nullptr`. Based on the proposed wording I would expect things like: The function may return nullptr to say The function may return a null pointer - Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19257#pullrequestreview-2062190669
Re: RFR: 8307778: com/sun/jdi/cds tests are not compatible with jtreg test factory
On Wed, 15 May 2024 05:59:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes the > `test/jdk/com/sun/jdi/cds/` tests from being problem listed when jtreg > launches these tests through a virtual thread? > > These tests aren't actually incompatible with virtual threads. The real issue > is that in https://bugs.openjdk.org/browse/JDK-8305608, a test infrastructure > class `test/jdk/com/sun/jdi/VMConnection.java` was changed to use the > `test.class.path` system property instead of the previously used > `test.classes`. > > That change missed out updating the `test/jdk/com/sun/jdi/cds/` tests to pass > along the classpath through the `test.class.path` system property. As a > result these tests still use the old `test.classes` system property to pass > around the test classpath and thus causes these tests to fail. The reason why > this only impacts virtual threads is noted by Chris in this JBS comment > https://bugs.openjdk.org/browse/JDK-8305608?focusedId=14572100=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14572100. > > The commit in this PR updates the `CDSJDITest` to pass along > `test.class.path` instead of `test.classes`. The `CDSJDITest` is only used in > the tests under `test/jdk/com/sun/jdi/cds/`, so nothing else should be > impacted by this change. > > I ran these changes against both launching with platform thread as well as > virtual thread and these tests now pass in both these cases. Looks good. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19244#pullrequestreview-2056988315
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 15:32:27 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Fix another typo
>> - Fix typo
>> - Add more comments
>
> src/hotspot/share/runtime/arguments.cpp line 2271:
>
>> 2269: } else if (match_option(option, "--illegal-native-access=",
>> )) {
>> 2270: if (!create_module_property("jdk.module.illegal.native.access",
>> tail, InternalProperty)) {
>> 2271: return JNI_ENOMEM;
>
> I think it would be helpful to get guidance on if this is the right way to
> add this system property, only because this one not a "module property". The
> configuration (WriteableProperty + InternalProperty) look right.
So my recollection/understanding is that we use this mechanism to convert
module-related `--` flags passed to the VM into system properties that the Java
code can then read, but we set them up such that you are not allowed to specify
them directly via `-D`. Is the question here whether this new property should
be in the `jdk.module` namespace?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1600819327
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]
On Tue, 14 May 2024 18:10:28 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with two > additional commits since the last revision: > > - Address review comments >Improve warning for JNI methods, similar to what's described in JEP 472 >Beef up tests > - Address review comments Hotspot changes look good - notwithstanding discussion about properlty namespace placement. Manpage changes also look good. - PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2056696636
Re: RFR: 8330066: HeapDumpPath and HeapDumpGzipLevel VM options do not mention HeapDumpBeforeFullGC and HeapDumpAfterFullGC [v2]
On Tue, 14 May 2024 01:53:20 GMT, Alex Menkov wrote: >> The fix updates descriptions of `HeapDumpPath`/`HeapDumpGzipLevel` and >> `HeapDumpBeforeFullGC`/`HeapDumpAfterFullGC`/`HeapDumpOnOutOfMemoryError` VM >> options > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > align Updates look good. FWIW the Java command reference doesn't mention the before/after full GC flags either. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19224#pullrequestreview-2054872345
Re: RFR: 8332112: Update nsk.share.Log to don't print summary during VM shutdown hook
On Sun, 12 May 2024 21:34:41 GMT, Leonid Mesnik wrote:
> The nsk.share.Log doing some cleanup and reporting errors in the cleanup
> method. This method is supposed to be executed by finalizer originally.
> However, now it is called only during shutdown hook.
> The cleanup using Cleaner doesn't work. See
> https://bugs.openjdk.org/browse/JDK-8330760
>
> The cleanup() method flush stream and print summary which should be already
> printed by complain method.
>
> This cleanup is not necessary and printing summary usually is just disabled.
> It is enabled if the test called 'complain' method. However, the error should
> have been printed already in this method.
>
> So it would be simple to remove this cleanup and reduce usage of Finalizable
> in vmTestbase tests.
>
> Note: The 'verboseOnErrorEnabled' is just not used.
>
> See isVerboseOnErrorEnabled.
>
> public boolean isVerboseOnErrorEnabled() {
> return errorsSummaryEnabled;
> }
>
>
> Tested with by running tests with different combinations (tier4-7) and tier1.
Okay - seems fine. Thanks.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19209#pullrequestreview-2054686733
