Integrated: 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 This pull request has now been integrated. Changeset: 6e228ce3 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/6e228ce382d1fad6cba0d0df8a507e6bd32a9a4a Stats: 4114 lines in 42 files changed: 2528 ins; 1150 del; 436 mod 8336254: Virtual thread implementation + test updates Reviewed-by: sspitsyn, kevinw - PR: https://git.openjdk.org/jdk/pull/20143
Re: RFR: 8336254: Virtual thread implementation + test updates [v2]
> 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 Alan Bateman 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 eight additional commits since the last revision: - Merge - Fix typo in comment, missing copyright update, test nits - Merge - Drop JLA updates for this update - Merge - Merge - Update copyright headers - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/20143/files - new: https://git.openjdk.org/jdk/pull/20143/files/1fad7dff..2d3bbf46 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=20143=01 - incr: https://webrevs.openjdk.org/?repo=jdk=20143=00-01 Stats: 4362 lines in 184 files changed: 2873 ins; 856 del; 633 mod Patch: https://git.openjdk.org/jdk/pull/20143.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20143/head:pull/20143 PR: https://git.openjdk.org/jdk/pull/20143
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. Marked as reviewed by alanb (Reviewer). Thanks for the tireless effort to update the man pages at the end of each release. - PR Review: https://git.openjdk.org/jdk/pull/20248#pullrequestreview-2187669729 PR Comment: https://git.openjdk.org/jdk/pull/20248#issuecomment-2238647463
Re: RFR: 8336254: Virtual thread implementation + test updates
On Thu, 18 Jul 2024 10:52:43 GMT, Serguei Spitsyn wrote: > Q: There are some new files with a copyright year ranges. I will check but there are a few renames that have to keep the initial year. Also there are some new files that have been checked into the loom repo for a long time so they have their initial year too. - PR Comment: https://git.openjdk.org/jdk/pull/20143#issuecomment-2236251273
Re: RFR: 8336254: Virtual thread implementation + test updates
On Thu, 18 Jul 2024 10:46:16 GMT, Serguei Spitsyn 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
>
> test/jdk/java/lang/Thread/virtual/ThreadAPI.java line 1113:
>
>> : @Test
>> 1112: void testYield3() throws Exception {
>> 1113: assumeTrue(VThreadScheduler.supportsCustomScheduler(), "No
>> support for custom schedulers");
>
> Q: What was the reason to rename `testYield2()` to `testYield3()`?
There are a lot of tests that can't be proposed to main line at this time
because they depend on the object monitor work. This is one such "gap" where
testYield2 can't be added. I can re-number them if you want to avoid the gap
but there are other changes that rename several test methods so I'd prefer to
leave it until then.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20143#discussion_r1682651423
Re: RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v2]
On Wed, 17 Jul 2024 14:07:55 GMT, Thomas Stuefe wrote:
>> Sonia Zaldana Calles has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Updating copyright headers
>
> src/hotspot/share/services/diagnosticCommand.cpp line 1138:
>
>> 1136: stringStream msg;
>> 1137: msg.print("Invalid file path name specified: %s",
>> _filepath.value());
>> 1138: THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>> msg.base());
>
> write to output() and return instead of throwing
Yes, the error needs to be written the stream so that it is printed by the tool
(at the end other of the pipe).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681352690
RFR: 8336254: Virtual thread implementation + test updates
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 - Commit messages: - Drop JLA updates for this update - Merge - Merge - Update copyright headers - Initial commit Changes: https://git.openjdk.org/jdk/pull/20143/files Webrev: https://webrevs.openjdk.org/?repo=jdk=20143=00 Issue: https://bugs.openjdk.org/browse/JDK-8336254 Stats: 4116 lines in 42 files changed: 2528 ins; 1150 del; 438 mod Patch: https://git.openjdk.org/jdk/pull/20143.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20143/head:pull/20143 PR: https://git.openjdk.org/jdk/pull/20143
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 I don't have any other comments, it's good to have this test focus on just wrapping one native method. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2177240476
Re: RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
On Mon, 15 Jul 2024 08:16:25 GMT, Jaikiran Pai wrote: > Hello David, > > > @jaikiran there is a lot of unrelated refactoring and style changes here > > that obscures what the actual fix is. > > I've updated the PR to undo some of the cosmetic changes that were introduced > to cleanup the code a bit. The updated state of the PR now just includes > functional updates to the test to address the issue noted in the linked JBS. Would it be possible to at least re-format the declaration of the "transform" method as it's very messy and hard to see what is declaration vs. body. The format that you had in the first iteration was helpful because it allowed us to clearly see what is in the declaration vs. body. I think I would do the same for the messy premain method but that can be cleanup/re-formatting for another day. - PR Comment: https://git.openjdk.org/jdk/pull/20154#issuecomment-2227969544
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. I think the changes looks okay. It narrows to instrumenting a specific test class, thus removing all the side effects of the transformer code generating wrapper methods for JDK classes. The cleanups looks okay, test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 44: > 42: * @comment The test uses asmlib/Instrumentor.java which relies on > ClassFile API PreviewFeature. > 43: * @run main/native/timeout=240 NativeMethodPrefixApp roleDriver > 44: * @comment The test uses a higher timeout to prevent test timeouts noted > in JDK-6528548 Is /timeout=240 (and the comment) needed now. If I read the old issue correctly it was due to use a JDK mounted on a network file system. test/jdk/java/lang/instrument/libNativeMethodPrefix.c line 24: > 22: */ > 23: > 24: #include "jni.h" I assume this needs `#include `. - PR Review: https://git.openjdk.org/jdk/pull/20154#pullrequestreview-2176976546 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677385666 PR Review Comment: https://git.openjdk.org/jdk/pull/20154#discussion_r1677383597
Re: RFR: 8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors [v4]
On Wed, 10 Jul 2024 20:10:13 GMT, Volker Simonis wrote: >> Since Java 5 the `java.lang.instrument` package provides services that allow >> Java programming language agents to instrument (i.e. modify the bytecode) of >> programs running on the Java Virtual Machine. The `java.lang.instrument` >> functionality is based and implemented on top of the native Java Virtual >> Machine Tool Interface (JVMTI) also introduced in Java 5. But because the >> `java.lang.instrument` API is a pure Java API and uses Java classes to >> instrument Java classes it imposes some usage restrictions which are not >> very well documented in its API specification. >> >> E.g. the section on ["Bytecode >> Instrumentation"](https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#bci) >> in the JVMTI specification explicitly warns that special "*Care must be >> taken to avoid perturbing dependencies, especially when instrumenting core >> classes*". The risk of such "perturbing dependencies" is obviously much >> higher in a Java API like `java.lang.instrument`, but a more detailed >> explanation and warning is missing from its API documentation. >> >> The most evident class file transformation restriction is that while a class >> A is being loaded and transformed it is not possible to use this same class >> directly or transitively from the `ClassFileTransformer::transform()` >> method. Violating this rule will result in a `ClassCircularityError` (the >> exact error type is disputable as can be seen in [8164165: JVM throws >> incorrect exception when ClassFileTransformer.transform() triggers class >> loading of class already being >> loaded](https://bugs.openjdk.org/browse/JDK-8164165), but the result would >> be a `LinkageError in any case). >> >> The risk to run into such a `ClassCircularityError` error increases with the >> amount of code a transforming agent is transitively using from the >> `transform()` method. Using popular libraries like ASM, ByteBuddy, etc. for >> transformation further increases the probability of running into such >> issues, especially if the agent aims to transform core JDK library classes. >> >> By default, the occurrence of a `ClassCircularityError` in >> `ClassFileTransformer::transform()` will be handled gracefully with the only >> consequence that the current transformation target will be loaded unmodified >> (see `ClassFileTransformer` API spec: "*throwing an exception has the same >> effect as returning null*"). But unfortunately, it can also have a subtle >> but at the same time much more far-reaching consequence. If the >> `ClassCircularityError` occurs during the resolution of a constant pool ... > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed @AlanBateman's new suggestions Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20011#pullrequestreview-2174286901
Re: RFR: 8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors [v3]
On Wed, 10 Jul 2024 16:56:37 GMT, Volker Simonis wrote:
>> src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
>> line 179:
>>
>>> 177: * This means that a {@link LinkageError} triggered during
>>> transformation of
>>> 178: * {@code C} in a class {@code D} not directly related to {@code C}
>>> can repeatedly
>>> 179: * occur later in arbitrary user code which uses {@code D}.
>>
>> This paragraph looks okay but I can't help thinking we should have something
>> in normative text to reference that specifies the reentrancy behavior.
>> Maybe I missed it but I thought we have something in the API docs on this.
>
> I haven't found anything either. The only specification-relevant mentioning
> of the issue I found is in the [JVMTI
> Specification](https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#bci)
> referenced at the beginning of the PR:
>
>> Care must be taken to avoid perturbing dependencies, especially when
>> instrumenting core classes.
>
> The example that follows describes an infinite recursion when instrumenting
> the the `j.l.Object()` constructor.
>
> I think the exact reentrancy behavior isn't specified anywhere. Not even the
> exact that should be thrown in such a case is specified (see [8164165: JVM
> throws incorrect exception when ClassFileTransformer.transform() triggers
> class loading of class already being
> loaded](https://bugs.openjdk.org/browse/JDK-8164165) for a discussion of
> different scenarios).
>
> I think the real problem is that the JVMS predates the JVMTI specification
> and the interaction between instrumentation and class loading isn't clearly
> defined. I think it might even be possible to treat class loading errors
> during transformation differently, such that they will not lead to a
> permanent resolution error for the corresponding constant pool entries. I
> know that this will violate the current section § 5.4.3 Resolution
> (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3)
> of the JVM specification which mandates that "if an attempt by the Java
> Virtual Machine to resolve a symbolic reference fails because an error is
> thrown that is an instance of LinkageError (or a subclass), then subsequent
> attempts to resolve the reference always fail with the same error that was
> thrown as a result of the initial resolution attempt". But as I wrote, that
> predates JVMTI and when JVMTI was added, we missed the opportunity to specify
> its exact impact on class loading
and resolution.
>
> But all this is a much bigger discussion. Maybe we should open another issue
> for it?
I've created [JDK-8336296)](https://bugs.openjdk.org/browse/JDK-8336296) for
the spec issues.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1675558673
Re: RFR: 8336257: Additional tests in jmxremote/startstop to match on PID not app name [v2]
On Thu, 11 Jul 2024 15:38:29 GMT, Kevin Walls wrote: >> Follow-on from JDK-8207908. >> >> Two tests are broken by that change: >> sun/management/jmxremote/startstop/JMXStartStopTest.java >> sun/management/jmxremote/startstop/JMXStatusPerfCountersTest.java >> >> These are additional tests which use jcmd and an application name pattern to >> find a process. >> They should use the PID to avoid finding a process from some other >> concurrent test invocation. >> So it's good to have the same kind of change applied here too. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > line Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20138#pullrequestreview-2172467658
Re: RFR: 8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors [v3]
On Tue, 9 Jul 2024 14:18:53 GMT, Volker Simonis wrote:
>> Since Java 5 the `java.lang.instrument` package provides services that allow
>> Java programming language agents to instrument (i.e. modify the bytecode) of
>> programs running on the Java Virtual Machine. The `java.lang.instrument`
>> functionality is based and implemented on top of the native Java Virtual
>> Machine Tool Interface (JVMTI) also introduced in Java 5. But because the
>> `java.lang.instrument` API is a pure Java API and uses Java classes to
>> instrument Java classes it imposes some usage restrictions which are not
>> very well documented in its API specification.
>>
>> E.g. the section on ["Bytecode
>> Instrumentation"](https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#bci)
>> in the JVMTI specification explicitly warns that special "*Care must be
>> taken to avoid perturbing dependencies, especially when instrumenting core
>> classes*". The risk of such "perturbing dependencies" is obviously much
>> higher in a Java API like `java.lang.instrument`, but a more detailed
>> explanation and warning is missing from its API documentation.
>>
>> The most evident class file transformation restriction is that while a class
>> A is being loaded and transformed it is not possible to use this same class
>> directly or transitively from the `ClassFileTransformer::transform()`
>> method. Violating this rule will result in a `ClassCircularityError` (the
>> exact error type is disputable as can be seen in [8164165: JVM throws
>> incorrect exception when ClassFileTransformer.transform() triggers class
>> loading of class already being
>> loaded](https://bugs.openjdk.org/browse/JDK-8164165), but the result would
>> be a `LinkageError in any case).
>>
>> The risk to run into such a `ClassCircularityError` error increases with the
>> amount of code a transforming agent is transitively using from the
>> `transform()` method. Using popular libraries like ASM, ByteBuddy, etc. for
>> transformation further increases the probability of running into such
>> issues, especially if the agent aims to transform core JDK library classes.
>>
>> By default, the occurrence of a `ClassCircularityError` in
>> `ClassFileTransformer::transform()` will be handled gracefully with the only
>> consequence that the current transformation target will be loaded unmodified
>> (see `ClassFileTransformer` API spec: "*throwing an exception has the same
>> effect as returning null*"). But unfortunately, it can also have a subtle
>> but at the same time much more far-reaching consequence. If the
>> `ClassCircularityError` occurs during the resolution of a constant pool ...
>
> Volker Simonis has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressed @AlanBateman's suggestions and updated copyright year
src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
line 158:
> 156: *
> 157: *
> 158: * Note the term class file is used as defined in section {@jvms
> 4} The
The existing sentence uses "section" but I assume it should be "Chapter".
src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
line 167:
> 165: * same time required during the transformation process as this can lead
> to class
> 166: * circularity or linkage errors. Using bytecode transformation
> libraries which depend
> 167: * on core JDK class can increase the risk of such errors.
It's probably impossible to create a BCI library that don't depend on JDK core
classes so maybe it would be better to drop the second sentence.
src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
line 179:
> 177: * This means that a {@link LinkageError} triggered during
> transformation of
> 178: * {@code C} in a class {@code D} not directly related to {@code C} can
> repeatedly
> 179: * occur later in arbitrary user code which uses {@code D}.
This paragraph looks okay but I can't help thinking we should have something in
normative text to reference that specifies the reentrancy behavior. Maybe I
missed it but I thought we have something in the API docs on this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672215548
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672219986
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672255885
Re: Proposal: Option to ignore non-existent -javaagent
On 10/07/2024 16:37, Jaroslav Bachorik wrote: : This may not always be possible. Some systems have rather complex and inlexible launchers - for example Apache Spark with its clusters, drivers and executors and automatic distribution of resources. For that system, if one needs to add an on-startup Java agent via `-javaagent` option the only way is to modify the setup which will add `-javaagent` to all components, pointing to a location where the resource distribution service should be putting the agent jar. However, mistakes happen and the jar may not be there. But because usually the agent is providing tracing or metrics collection, which are all optional, it is not feasible to hard-crash the Java process because of not being able to load the Java agent. Forn this PoV the proposal to allow optionally ignoring non-existing Java agent sounds as a very pragmatic solution, The issue of injecting CLI options to start tool agents was explored back in JSR 163, that is the reason for environment variables such as JAVA_TOOL_OPTIONS. Broader support was added in JDK 9 with JDK_JAVA_OPTIONS, @argfile support, and support for GNU style options. So the JDK has several features to augment the command line or options. Has anyone tried to work with the Apache Spark (or other projects) to make use of any of this support so that additional CLI options are conditionally used? -Alan
Re: RFR: 8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors [v2]
On Fri, 5 Jul 2024 07:28:13 GMT, Volker Simonis wrote: > @AlanBateman would you mind having a quick look at this trivial, > documentation-only PR and let me know if you're OK with it? (Catching up as I was away for a few days). I agree it would be useful to have an API note on this topic. There were several reports of deadlocks, ClassCircularityError, and other hard to diagnose issues when support for instrumentation was originally introduced in JDK 5. I think agent maintainers learned to exclude many of the core classes in java.base but that is always a moving target and there have been a few more sightings/reports recently (maybe due to newer features doing more in Java rather than in the VM). I agree that the ClassFileTransformer class description is the best place for this. My initial reaction was to wonder about the j.l.instrument package description but it is more focused on starting agents and the JAR file attributes so I think your proposed location is best.. As regards the text then it might be better to start with a sentence to say that the "transform" method may be called from critical points in JDK core classes or called to transform JDK core classes. You've got some of this in second sentence but I think better to lead with it before revealing one of the several possible things that can happen. I think part of the lead in needs to say "great care must be taken" or something like that to get across the point that the ClassFileTransformer implementor has the potential to cause many possible issues. For the same class and linkage error then probably best to not use the phrase "transitively requires" as it's too close "requires transitive" used in module declarations. Note that the `@jvms` tag can be used to reference the JVMS section. You'll see a few examples in other classes. - PR Comment: https://git.openjdk.org/jdk/pull/20011#issuecomment-2217495130
Re: Proposal: Option to ignore non-existent -javaagent
On 03/07/2024 11:26, Thiago Henrique Hupner wrote: Dear JDK developers, I'd like to propose adding an option that allows the JVM to ignore a non-existent -javaagent instead of aborting. Currently, if a -javaagent is specified but the agent jar file doesn't exist, the JVM aborts with an error. This can cause issues in environments where the same JVM arguments are used across different systems, but not all systems have the agent installed. In general you can't assume that the same VM options or arguments to the java launcher can be used across different systems, different JDK releases, or in this case specifying a tool agent that may not be installed or may be installed in a different location. For this case it may be better to re-visit the launch script for the application so that it conditionally adds the -javaagent option when the tool agent is needed. -Alan
Re: RFR: 8334287: Man page update for jstatd deprecation [v2]
On Tue, 25 Jun 2024 08:25:00 GMT, Kevin Walls wrote: >> Man page update for JDK-8327793 which marked jstatd as deprecated for >> removal in JDK 24. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > text update Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19829#pullrequestreview-2137853292
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. I assume we should hold off reviewing until the warning and the experiment note are combined. - PR Comment: https://git.openjdk.org/jdk/pull/19829#issuecomment-2186517543
Re: RFR: 8327793: Deprecate jstatd for removal [v4]
On Tue, 11 Jun 2024 21:09:14 GMT, Sean Mullan wrote: >> I also think a period at the end is not necessary. > > For the Security Manager, the warning was worded a little differently: > > "WARNING: The Security Manager is deprecated and will be removed in a future > release" > > I think that wording is more clear. The current wording could be confusing > because JDK 24 is when jstatd is deprecated for removal, and not a future > release. The wording above is more definitive for those that may not > understand what "deprecated for removal" means. Thus, I suggest: > > "WARNING: jstatd is deprecated and will be removed in a future release" @kevinjwalls Are you planning to move to "WARNING" as has been suggested in the previous comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/19658#discussion_r1648897498
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual [v4]
On Wed, 19 Jun 2024 07:31:41 GMT, David Holmes wrote: > I don't follow. We have checked vt is not null and not the carrier, so we do > have it. My comment was about the case where the thread dump happens during a transition so sees the self-reference. In the loom repo the JavaThread has the thread ID of the mounted virtual thread so it can support this case, it doesn't need this check. So improvements in the future when the changes get the main line. - PR Comment: https://git.openjdk.org/jdk/pull/19744#issuecomment-2178017992
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual [v4]
On Wed, 19 Jun 2024 06:18:12 GMT, David Holmes wrote: > 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. I suggested we drop this for now because the thread dump can happen at times when there isn't a reference to the virtual Thread, so can't get the thread name, at least not easily. Definitely something to come back to. The changes in the loom repo means we at least have the thread ID of the mounted virtual thread at all times so this means we can fix the "Carrying .." line and also adjust the changes in this PR to remove the check for the self reference, meaning the "Mounted ..." line and the stack trace won't skipped when the thread dump is taken during a transition. - PR Comment: https://git.openjdk.org/jdk/pull/19744#issuecomment-2177900370
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 The changes in latest version (af85b261) look okay to me. We'll adjust this in the loom repo so that the Carrying and Mounted lines will print the thread ID for all cases. - PR Comment: https://git.openjdk.org/jdk/pull/19744#issuecomment-2176812086
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 I looked at the changes in the latest revision (9a954efe) and they look okay, it basically skips printing the mounted virtual thread when that virtual thread is interacting with the scheduler or timer mechanism, the so called "temporary transitions" that are needed to ensure the correct identity when submitting tasks to the scheduler, and to avoid deadlock when submitting tasks to the scheduler requires locking. It also changes the test so testing with JTREG_TEST_THREAD_FACTORY=Virtual won't observe the dummy thread in a temporary transition. We can improve on this in the loom repo as JavaThread has the thread ID of the mounted virtual thread. Note that the pre-existing "Carrying ..." line has the same issue as we've been discussing here, it just hasn't been noticed perhaps. One thing that I think we should do is remove the thread name from the "Mounted ..." line. I initially thought this was a good idea but there is no reference to this when a virtual thread is mounted and interacting with the scheduler. Virtual threads don't have a name by default so it's not a big a loss as you might think. We can of course re-visit this in the future. - PR Comment: https://git.openjdk.org/jdk/pull/19744#issuecomment-2175840976
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v18]
On Mon, 17 Jun 2024 20:51:34 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> braces
I skimmed through the changes, much more readable now compared to some of the
initial revisions. I kinda agree with one of the comments from a few days ago
that if (allow) { .. } else { ..} is easier to read than putting the disallow
case first. It's not a big deal as this code will be changed very soon to
remove the SM path.
As regards testing then I think the highest priority should be to default/no-SM
case. We need to be confident that there are no regressions in JMX for this
execution mode. We don't have of course want to regression for the SM case but
that is a niche execution mode and not long for this world.
-
PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2175831168
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual
On Mon, 17 Jun 2024 12:37:01 GMT, David Holmes 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 .
>
> 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?
`started.countDown()` is replaced with `started.set(true)` to remove the
possibility that "Dummy Vthread" unmounts here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642778316
Re: RFR: 8334215: serviceability/dcmd/thread/PrintMountedVirtualThread.java failing with JTREG_TEST_THREAD_FACTORY=Virtual
On Mon, 17 Jun 2024 12:35:15 GMT, David Holmes wrote: > 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! Just to note that JavaThread._vthread is always the "current thread". It's the virtual thread when mounted, the carrier (as in itself) when there is no virtual thread mounted. It's one this way so that the C2 instrinic for currentThread doesn't need a branch/test. - PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642772079
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 It's the countDown by "dummy" thread will unpark "main". The queuing and starting of the second FJ worker thread is happening in the context of dummy's carrier (not dummy itself). We want to eventually remove these temporary transitions, at least for the default scheduler, but aren't there yet. I've attached an initial patch to JDK-8334215 for the issues. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2167374446
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v18]
On Thu, 13 Jun 2024 14:37:45 GMT, Inigo Mediavilla Saiz wrote: > Would you mind explaining why unparking the main (virtual) thread can end up > with the main thread observing the dummy thread in transition ? The unpark of main requires queueing the task for main. This can trigger a new FJP worker to be started and there are several points where it may park. For reason that are complicated to go into here, this happens in the context of the carrier. The thread dump at this point is seeing a mounted continuation but JavaThread._vthread is pointing to self temporarily. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2166116062
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've created [JDK-8334215](https://bugs.openjdk.org/browse/JDK-8334215) with issues found when trying to sync up the loom repo. The tests in that repo are typically run with JTREG_TEST_THREAD_FACTORY=Virtual and can tickle issue that we don't always see immediately in main line. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2165253546
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:23:07 GMT, Daniel Fuchs wrote: >> Kevin Walls has updated the pull request incrementally with one additional >> commit since the last revision: >> >> udpates > > test/jdk/javax/management/remote/mandatory/notif/policy.negative line 7: > >> 5: permission javax.management.MBeanPermission >> "[domain:type=NB,name=2]", "addNotificationListener"; >> 6: permission javax.management.MBeanPermission "*", >> "removeNotificationListener"; >> 7: permission javax.security.auth.AuthPermission "doAs"; > > I suspect that this means a doPrivileged is missing somewhere. We should not > require the application to posess new permissions. I think Daniel is right, can you remove this permission and paste in the debug output to see where this is happening? - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636587055
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v17]
On Wed, 12 Jun 2024 07:14:59 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: > > Fix scope of the try block Thanks for taking feedback, I don't have any other comments. test/hotspot/jtreg/serviceability/dcmd/thread/PrintMountedVirtualThread.java line 38: > 36: * @requires vm.continuations > 37: * @library /test/lib > 38: * @modules java.base The `@modules` line isn't needed, it's not possible to have a run-time image without java.base. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19482#pullrequestreview-2112334068 PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1636049421
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
test/hotspot/jtreg/serviceability/dcmd/thread/PrintMountedVirtualThread.java
line 54:
> 52: output.shouldMatch(".*at " +
> Pattern.quote(DummyRunnable.class.getName()) + "\\.compute.*");
> 53: output.shouldMatch("Mounted virtual thread " + "\"Dummy
> Vthread\"" + " #" + vthread.threadId());
> 54: shouldFinish.set(true);
One other suggestion is to use a try-finally block here. Put L48-53 in the
block and set shouldFinish in the finally block. That way if the test fails
then it won't leave a spinning thread to disrupt the next test that runs in the
agent VM.
Also just to say that we've mostly used JUnit for new tests in recent releases,
moving away from TestNG for new tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1635872465
Re: RFR: 8327793: Deprecate jstatd for removal [v4]
On Tue, 11 Jun 2024 18:11:55 GMT, Kevin Walls wrote:
>> jstatd is an RMI server application which monitors HotSpot VMs, and provides
>> an interface to the monitoring tool jstat, for use across a remote RMI
>> connection.
>>
>> RMI is not how modern applications communicate. It is an old transport with
>> long term security concerns, and configuration difficulties with firewalls.
>>
>> The jstatd tool should be removed. Deprecating and removing jstatd will not
>> affect usage of jstat for monitoring local VMs using the Attach API.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove annotations
Changes look okay now. Will you update the CSR to align with the PR?
src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java line 84:
> 82:
> 83: System.err.println("Warning: jstatd is deprecated for removal in
> a future release.");
> 84:
I think we've put "WARNING" in caps in other usages.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19658#pullrequestreview-2111296929
PR Review Comment: https://git.openjdk.org/jdk/pull/19658#discussion_r1635388247
Re: RFR: 8327793: Deprecate jstatd for removal [v2]
On Tue, 11 Jun 2024 16:54:36 GMT, Kevin Walls wrote: >> jstatd is an RMI server application which monitors HotSpot VMs, and provides >> an interface to the monitoring tool jstat, for use across a remote RMI >> connection. >> >> RMI is not how modern applications communicate. It is an old transport with >> long term security concerns, and configuration difficulties with firewalls. >> >> The jstatd tool should be removed. Deprecating and removing jstatd will not >> affect usage of jstat for monitoring local VMs using the Attach API. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > No annotations for src/jdk.jstatd/share/classes/sun/jvmstat/monitor/remote BTW: Are you going to include the update to the man page (jstatd.1) in the PR or is that for another update? - PR Comment: https://git.openjdk.org/jdk/pull/19658#issuecomment-2161239976
Re: RFR: 8327793: Deprecate jstatd for removal
On Tue, 11 Jun 2024 16:52:13 GMT, Kevin Walls wrote: > Sure, happy to not add annotations in sun.jvmstat.monitor.remote > (RemoteHost.java, RemoteVm.java). Right, you can drop it from all the source files except for module-info.java. - PR Comment: https://git.openjdk.org/jdk/pull/19658#issuecomment-2161219889
Re: RFR: 8327793: Deprecate jstatd for removal
On Tue, 11 Jun 2024 13:09:06 GMT, Kevin Walls wrote: > jstatd is an RMI server application which monitors HotSpot VMs, and provides > an interface to the monitoring tool jstat, for use across a remote RMI > connection. > > RMI is not how modern applications communicate. It is an old transport with > long term security concerns, and configuration difficulties with firewalls. > > The jstatd tool should be removed. Deprecating and removing jstatd will not > affect usage of jstat for monitoring local VMs using the Attach API. The sun.jvmstat.monitor.remote package is not exported so I don't think adding `@Deprecated` makes sense. Look at JDK-8245068 when rmid was deprecated and see the warning that it printed. I think it's the warning piece that you'll need here. - PR Comment: https://git.openjdk.org/jdk/pull/19658#issuecomment-2160786798
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v12]
On Mon, 10 Jun 2024 07:36:50 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 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 15 additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into
> txominpelu_8330846_add_stack_vthreads
> - Include virtual thread name in output
> - Incorporate @tstuefe's remarks
> - Remove dead code
> - Remove extra indentation (leave it for the next PR)
> - Cleanup test
>
>- Stop virtualthread
>- Remove unneeded imports
>- Remove modules that are not needed
> - Fix copyright year
> - Print mounted virtual thread after carrier
> - Add indentation for virtual thread stack
> - Update
> test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java
>
>Co-authored-by: Andrey Turbanov
> - ... and 5 more: https://git.openjdk.org/jdk/compare/0b955462...ba3385a4
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
40:
> 38: * @run testng PrintVirtualThreadTest
> 39: */
> 40: public class PrintVirtualThreadTest {
It might be helpful to put "Mounted" into the test name as this is a test to
check that a mounted virtual thread appears in the Thread.print output,
something like PrintMountedVirtualThread.
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
52:
> 50: output.shouldMatch(".*at " +
> Pattern.quote(DummyRunnable.class.getName()) + "\\.run.*");
> 51: output.shouldMatch(".*at " +
> Pattern.quote(DummyRunnable.class.getName()) + "\\.compute.*");
> 52: shouldFinish.set(true);
This just checks that the stack frames are printed. I think you also want to
check that the output contains and line with "Mounted virtual thread and the
thread ID.
test/hotspot/jtreg/serviceability/dcmd/thread/PrintVirtualThreadTest.java line
78:
> 76: while (true) {
> 77: if (shouldFinish.get()) {
> 78: break;
You can use Thread.onSpinWait in the loop to reduce the impact of the tight
loop.
Also you can use `while (!shouldFinish.get())` to avoid the explicit break.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1634372088
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1634377680
PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1634374957
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v8]
On Tue, 4 Jun 2024 08:06:28 GMT, Alan Bateman wrote: >> 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 > > I think the presentation of the carrier the mounted virtual thread in update > (b122cc05) looks quite good. > > I think it would be helpful to include the thread name too. Many virtual > threads are unnamed so it will show as "" but that is okay. In addition to > being useful it means the format of the "Mounted virtual thread ..." line > will be consistent the first part of the line for platform threads. > > I'm wondering if we should remove the existing "Carrying virtual thread ..." > line as part of this. It's redundant now, @pron ? > @AlanBateman @dholmes-ora would you be OK with leaving the current PR as-is > and handling the indentation topic in a separate PR ? I assume the follow-up to improve the alignment will be a small change. No objection if you do it here or in a follow-up PR. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2160047420
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Mon, 10 Jun 2024 11:28:28 GMT, Kevin Walls wrote:
> JMX uses APIs related to the Security Mananger which are deprecated. Use of
> AccessControlContext will be removed when Security Manager is removed.
>
> Until then, updates are needed to not require setting
> -Djava.security.manager=allow to use JMX authentication.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1634:
> 1632: } else {
> 1633: // We have ACC therefore SM is permitted, and
> Subject must be non-null:
> 1634: return Subject.doAs(subject,
> (PrivilegedExceptionAction) () ->
> AccessController.doPrivileged((PrivilegedExceptionAction) () ->
> wrappedClass.cast(mo.get()), acc));
This is not readable. If you create the PAEs explicitly then the casts will go
away and it will be much easier to read.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1633351429
Re: RFR: 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 > - 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). - PR Review: https://git.openjdk.org/jdk/pull/19617#pullrequestreview-2106832118
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v7]
On Wed, 5 Jun 2024 15:58:28 GMT, Thomas Stuefe wrote: >> Thanks ! >> >> Your PR looks very promising @tstuefe, I would indeed prefer to wait for >> your changes as a way to add additional indentation to the stack of the >> virtual thread. >> >> What do you think if I leave the current PR with the indentation that is >> already used for the stack of the carrier thread and I create a separate PR >> based on yours to add the additional indentation ? > > Okay for me, if other reviewers are okay with it. I think it would be better if the stack trace of the mounted virtual thread didn't align/indent the same as the carrier as it kinda looks like it's one larger stack trace. That said, no objection if there is a follow-on PR to do that, in which case might be better to wait until after the fork for JDK 23 so that the two PRs go into the same release. - PR Review Comment: https://git.openjdk.org/jdk/pull/19482#discussion_r1628172341
Re: 8330846: (jstack) Add stacks of mounted virtual threads
On 05/06/2024 07:45, Iñigo Mediavilla wrote: Thanks, Alan. Sorry, I wasn't clear in my message. I wasn't thinking about including unmounted threads in the output of `Thread.print`, I agree with you that besides the formatting things that still need to be fixed there's no need to add more changes to the `Thread.print` command in the context of 8330846. My doubts were related to the fact that 8330846 also refers to including the stack of mounted threads in the output of `jstack` and I'm not sure about what's the best way to implement that. In my previous message, I explained some approaches that I looked into to have mounted virtual threads in `jstack` but I'm not sure about them. jstack is the equivalent of jcmd Thread.print so I don't think there is anything to do. -Alan
Re: 8330846: (jstack) Add stacks of mounted virtual threads
On 04/06/2024 21:52, Iñigo Mediavilla wrote: Hello, While there's ongoing work on: https://github.com/openjdk/jdk/pull/19482 to add the stack trace of mounted virtual threads to the `jcmd Thread.print` command, I'm starting to think about how I could do to print the stack trace for virtual threads from `jstack` but I'm not sure about what would be the best approach, so I'd be happy to get people's thoughts on the topic. JDK-8330846 is about showing the stack traces of mounted virtual threads, I don't think pull/19482 should be expanded to do anything else. For the PR then I think you are just waiting for Thomas Stuefe's change with support for automatic indentation and of course converging on some details of the output. If you are asking if it should include the stack traces of unmounted virtual threads then we decided some time ago to not do that. The Thread.print commands runs as a VM operation so the at a safepoint. Adding more work to that, to walk the heap to find virtual thread objects, would just increase the STW time. The other issue is that the output (text format) doesn't scale to hundreds of thousands of threads. You might ask about de-duplication but that is just adding more overhead to the thread dump and may be better left to tools that process the thread dump. These are issues/reasons for Thread.dump_to_file. It uses the tree of thread groupings to find all threads. -Alan
Re: RFR: 8332070: Convert package.html files in `java.management` to package-info.java [v3]
On Wed, 5 Jun 2024 01:21:36 GMT, Nizar Benalla wrote:
>> This is a simple noreg cleanup. The motivation was that I noticed javac
>> doesn't recognise package.html files well.
>>
>> Some of the contents of the `package.html` files (and code in the package)
>> may be outdated, but I think it is out of scope for this PR.
>>
>> I have also changed three `{@link }` usages with the `href` that would have
>> been in the generated HTML.
>> Because referencing an element from an other module wouldn't work even when
>> using the `module/package.class#member`, if the `module-info.java` file does
>> not have "require ``".
>>
>> I am referring to line 69 in
>> `src/java.management/share/classes/javax/management/monitor/package-info.java`
>> and lines 90 and 120 in
>> `src/java.management/share/classes/javax/management/remote/package-info.java`
>>
>> Adding 2 dependencies just for 3 links didn't seem right.
>>
>> edit: passes all tests, the failing test is a github actions issue
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove double spacing
Marked as reviewed by alanb (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/19399#pullrequestreview-2098000999
Re: RFR: 8332070: Convert package.html files in `java.management` to package-info.java [v2]
On Mon, 27 May 2024 09:01:48 GMT, Nizar Benalla wrote:
>> This is a simple noreg cleanup. The motivation was that I noticed javac
>> doesn't recognise package.html files well.
>>
>> Some of the contents of the `package.html` files (and code in the package)
>> may be outdated, but I think it is out of scope for this PR.
>>
>> I have also changed three `{@link }` usages with the `href` that would have
>> been in the generated HTML.
>> Because referencing an element from an other module wouldn't work even when
>> using the `module/package.class#member`, if the `module-info.java` file does
>> not have "require ``".
>>
>> I am referring to line 69 in
>> `src/java.management/share/classes/javax/management/monitor/package-info.java`
>> and lines 90 and 120 in
>> `src/java.management/share/classes/javax/management/remote/package-info.java`
>>
>> Adding 2 dependencies just for 3 links didn't seem right.
>>
>> edit: passes all tests, the failing test is a github actions issue
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> better indentation - no content changes
Looks okay, just notice that classes/java/lang/management/package-info.java
still has the double space. Not a big deal but since you fixed the others then
maybe do this one too.
-
PR Comment: https://git.openjdk.org/jdk/pull/19399#issuecomment-2148243229
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 I think the presentation of the carrier the mounted virtual thread in update (b122cc05) looks quite good. I think it would be helpful to include the thread name too. Many virtual threads are unnamed so it will show as "" but that is okay. In addition to being useful it means the format of the "Mounted virtual thread ..." line will be consistent the first part of the line for platform threads. I'm wondering if we should remove the existing "Carrying virtual thread ..." line as part of this. It's redundant now, @pron ? - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2146875861
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Tue, 4 Jun 2024 06:38:36 GMT, Serguei Spitsyn wrote: >> 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"? > > Thanks, David. I also feel this clarification is still useful. I think this is the right place but it is only for return values. There are a few functions where a parameter value can be a null pointer, e.g. in GetThreadState, SuspendThread, GetOwnedMonitorInfo the thread parameter can be a null pointer to mean the current thread. I don't think the introduction section has anywhere right now to say what a null pointer means. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1625458511
Re: RFR: 8330846: Add stacks of mounted virtual threads to the HotSpot thread dump [v6]
On Mon, 3 Jun 2024 11:26:27 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: > > Add indentation for virtual thread stack I don't think showing the frames of the mounted virtual thread before the carrier thread frames is the best way to show this. For one thing, it appears immediately after the carrier's thread name/details and state. So I think Ron's suggestion to clearly label it as the mounted virtual thread after the carrier stack trace would be good to try. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2145174545
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() Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2092010428
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. - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2142000785
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`. Thanks for take this one. Here's the result with the changes in 1a75277e. "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 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) Carrying virtual thread #24 at Main.lambda$main$0(Main.java:7) at java.lang.VirtualThread.run(java.base/VirtualThread.java:381) Note that the line "Carrying virtual thread #24" is printed twice. Also it's not immediately clear that there are two stack traces. You'll likely get different opinions on how mounted virtual threads should be presented. A few things to try - indent the stack trace of the mounted virtual thread - list the mounted virtual threads at the end - PR Comment: https://git.openjdk.org/jdk/pull/19482#issuecomment-2140202464
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 06:51:54 GMT, David Holmes wrote: >> 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. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/19405#discussion_r1620105401
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 06:14:21 GMT, David Holmes wrote:
>> 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?
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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19405#discussion_r1620033427
Re: RFR: 8332751: Broken link in VirtualMachine.html
On Wed, 29 May 2024 20:17:30 GMT, Chris Plummer wrote: > Fixed broken javadoc link. I confirmed that it currently is broken as can be > seen in the JDK 22 javadocs: > > https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/jdk.jdi/com/sun/jdi/VirtualMachine.html#allThreads() > > Click on the "virtual threads" link within the allThreads() description to > see the broken link. > > I verified the fix by building the javadocs and checking that the link now > works. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19469#pullrequestreview-2087310583
Re: JDK-8330846 - Status check
I don't know if Alex Menkov is subscribed here, better to ask on serviceability-dev. -Alan On 28/05/2024 11:03, Iñigo Mediavilla wrote: On Tue, May 28, 2024 at 11:28 AM Iñigo Mediavilla wrote: Hello, I've been looking at possible tickets that I could work on under the loom label in JBS, and I've run into : JDK-8330846 - Add stacks of mounted virtual threads to the HotSpot thread dump Link: https://bugs.openjdk.org/browse/JDK-8330846 It seems to be assigned to you, Alex Menkov. Is it something that you're working on or that you'd want to work on, or would you be OK if I took it ? Regards Íñigo Mediavilla Saiz
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count
On Sun, 26 May 2024 14:24:27 GMT, SendaoYan wrote: > > That would mean it's not tested. I suspect the > > java_lang_VirtualThread::is_instance checks will need to be changed to test > > with is_a(vmClasses::BaseVirtualThread_klass()) to allow for the > > alternative implementation. > > Do you mean change like this: No, I meant in JvmtiEnvBase::get_object_monitor_usage that you'll need to go through all places where it tests if the thread is a virtual thread, they will need to be changed to support the alternative implementation. - PR Comment: https://git.openjdk.org/jdk/pull/19405#issuecomment-2132281742
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. That would mean it's not tested. I suspect the java_lang_VirtualThread::is_instance checks will need to be changed to test with is_a(vmClasses::BaseVirtualThread_klass()) to allow for the alternative implementation. - PR Comment: https://git.openjdk.org/jdk/pull/19405#issuecomment-2132184808
Re: RFR: 8332070: Convert package.html files in `java.management` to package-info.java
On Fri, 24 May 2024 18:11:18 GMT, Nizar Benalla wrote:
> This is a simple noreg cleanup. The motivation was that I noticed javac
> doesn't recognise package.html files well.
>
> Some of the contents of the `package.html` files (and code in the package)
> may be outdated, but I think it is out of scope for this PR.
>
> I have also changed three `{@link }` usages with the `href` that would have
> been in the generated HTML.
> Because referencing an element from an other module wouldn't work even when
> using the `module/package.class#member`, if the `module-info.java` file does
> not have "require ``".
>
> I am referring to line 69 in
> `src/java.management/share/classes/javax/management/monitor/package-info.java`
> and lines 90 and 120 in
> `src/java.management/share/classes/javax/management/remote/package-info.java`
>
> Adding 2 dependencies just for 3 links didn't seem right.
>
> edit: tier 1 passes on my machine, will check why a certain test fails later.
> As this change shouldn't cause breakage
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>jtreg:test/hotspot/jtreg:tier1_serviceability 353 353 0 0
>
> ==
> TEST SUCCESS
src/java.management/share/classes/java/lang/management/package-info.java line
27:
> 25:
> 26: /**
> 27: * Provides the management interfaces for monitoring and management of
> the
I assume you used a script to convert these, I'm just curious why puts two
spaces after the *, the usual convention is one. It's not a big deal but if
this is something that can be fixed in the scripts then it would avoid a future
cleanup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19399#discussion_r1614381819
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v7]
On Wed, 15 May 2024 20:29:17 GMT, Serguei Spitsyn wrote: >> The fix is to degrade virtual threads support in the JVM TI >> `GetObjectMonitorUsage` function so that it is specified to only return an >> owner when the owner is a platform thread. Also, virtual threads are not >> listed in the both `waiters` and `notify_waiters` lists returned in the >> `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI >> functions and events for virtual threads, we missed this one. >> >> The main motivation for degrading it now is that the object monitor >> implementation is being updated to allow virtual threads unmount while >> owning monitors. It would add overhead to record monitor usage when >> freezing/unmount, overhead that couldn't be tied to a JVMTI capability as >> the capability can be enabled at any time. >> >> `GetObjectMonitorUsage` was broken for 20+ years >> ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports >> so it seems unlikely that the function is widely used. Degrading it to only >> return an owner when the owner is a platform thread has no compatibility >> impact for tooling that uses it in conjunction with `HotSpot` thread dumps >> or `ThreadMXBean`. >> >> One other point about `GetObjectMonitorUsage` is that it pre-dates >> j.u.concurrent in Java 5 so it can't be used to get a full picture of the >> lock usage in a program. >> >> The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the >> JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` >> methods are updated to match the JVM TI spec. >> >> Also, please, review the related CSR and Release Note: >> - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade >> virtual thread support for GetObjectMonitorUsage >> - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: >> degrade virtual thread support for GetObjectMonitorUsage >> >> Testing: >> - tested impacted and updated tests locally >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: UNDO: removed incorrect simplification that removed a tmp local > skipped Spec + code changes look okay. I didn't study the tests closely but I see you have updated the test coverage to ensure that virtual threads are not reported as the owner, waiting to enter, or waiting to be notified. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19030#pullrequestreview-2073032052
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Wed, 22 May 2024 21:42:14 GMT, Kevin Rushforth wrote: > Further, I confirm that if I pass that option to jlink or jpackage when > creating a custom runtime, there is no warning. Great! What about jpackage without a custom runtime, wondering if --java-options can be tested. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2126320311
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Mon, 20 May 2024 18:47:35 GMT, Phil Race wrote: > Have you looked into / thought about how this will work for jpackaged apps ? > I suspect that both the existing FFM usage and this will be options the > application packager will need to supply when building the jpackaged app - > the end user cannot pass in command line VM options. Seems there should be > some testing of this as some kind of native access could be a common case for > jpackaged apps. I don't see any tests in test/jdk/tools/jpackage that creates an application that uses JNI code. Seems like a good idea to add this via another PR and it specify --java-options so that the application launcher enables native access. It could test jpackage using jlink too. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2121927727
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Mon, 20 May 2024 18:39:31 GMT, Phil Race wrote: >> make/conf/module-loader-map.conf line 105: >> >>> 103: java.smartcardio \ >>> 104: jdk.accessibility \ >>> 105: jdk.attach \ >> >> The list of allowed modules has been rewritten from scratch, by looking at >> the set of modules containing at least one `native` method declaration. > > Should I understand this list to be the set of modules exempt from needing to > specific that native access is allowed ? > ie they always have native access without any warnings, and further that any > attempt to enable warnings, or to disable native access for these modules is > ignored ? Yes, this was added via JDK-8327218. The changes in this PR are just trimming down the list to only the modules that have native code. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607147983
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 08:25:17 GMT, Kevin Walls wrote: >>> > ...Is there any way to run jconsole in a way that would result in it >>> > passing a non-empty delegationSubjects, resulting in this issue still >>> > reproducing? >>> >>> I don't think there is, JConsole has a hard null in this call. Also I don't >>> see in JConsole any calls of getMBeanServerConnection with a Subject being >>> passed, that's the main gateway method to use the removed feature. >>> >>> If there was a way to use Subject Delegation with JConsole (or with >>> anything else), and you try to attach to a jdk-23, then that will fail with >>> the UnsupportedOperationException and that's what we want as the feature is >>> gone. Realistically it's a feature with no known usage as discussed in the >>> deprecation and removal changes. >> >> If jconsole is passing null, why is it triggering this exception? > >> If jconsole is passing null, why is it triggering this exception? > > JConsole passes null, but when running on an older jdk, the older > RMIConnector actually "promotes" it to an array before making the remote > call. If you connect to a jdk-23 with the removal, the exception is thrown. > > (JConsole running on jdk-23 can connect to jdk-23 fine.) > (Also just to note, a jdk-23 JConsole can connect to an older JDK and show > the MBean tab OK.) @kevinjwalls I assume the RN for the removal of the subject delegation feature will need to be adjusted once this current PR is integrated. - PR Comment: https://git.openjdk.org/jdk/pull/19253#issuecomment-2119907523
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 Are there tests that run with TraceJVMTI so that this option is tested? - PR Comment: https://git.openjdk.org/jdk/pull/19275#issuecomment-2119724541
Re: RFR: 8332071: Convert package.html files in `java.management.rmi` to package-info.java
On Thu, 16 May 2024 10:39:43 GMT, Nizar Benalla wrote: > Please review this change. I converted the `package.html` file to > `package-info.java`, because `javac` cannot recognize `package.html`. > I already brought this up [in the mailing > list](https://mail.openjdk.org/pipermail/serviceability-dev/2024-May/055650.html). > The conversion was done in-place, only renaming it in git. > > I also added a couple of `@since` tags, with only 2 changes I don't want to > split these two fixes into separate PRs. > `CREDENTIALS_FILTER_PATTERN` and `SERIAL_FILTER_PATTERN` were first added in > https://bugs.openjdk.org/browse/JDK-8187556 src/java.management.rmi/share/classes/javax/management/remote/rmi/package-info.java line 26: > 24: */ > 25: > 26: /** I assume you'll need to prepend each line with `*` too, which has the side effect of making it appear that every line is changed but I think we just need to get over that. - PR Review Comment: https://git.openjdk.org/jdk/pull/19263#discussion_r1606084149
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Thu, 16 May 2024 20:17:18 GMT, Kevin Walls wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> Kevin Walls has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - add an 'also'
> - typo
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 961:
> 959: * @param delegationSubjects should be {@code null}, but a non-null
> 960: * array is also accepted for compatibility reasons, which must not
> 961: * contain any non-null entries.
The wording is bit unusual for a parameter description. Just wondering if might
be clearer to say "null or an array of null elements" and put add an `@apiNote`
to explain that it allows an array with null elements for compatibility
reasons. What you have is okay too course, I'm just trying to think of another
way to present this odd case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1604740107
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 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 one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
This looks good. Just a few minor comments where future maintainers might
appreciate comments that describe parameters.
src/java.base/share/classes/java/lang/Module.java line 332:
> 330: String caller = currentClass != null ?
> currentClass.getName() : "code";
> 331: if (jni) {
> 332: System.err.printf("""
System.err may change in a running VM. It may be that we will need to change
this at some point to use its initial setting. Not suggesting we changing it
now but we might have to re-visit this.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2062832385
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604653749
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 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 one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/ClassLoader.java line 2448:
> 2446: * Invoked in the VM class linking code.
> 2447: */
> 2448: static long findNative(ClassLoader loader, Class clazz, String
> entryName, String javaName) {
I think this is another place where `@param` descriptions would help as it's
not immediately clear that "javaName" is a method name.
src/java.base/share/classes/java/lang/Runtime.java line 39:
> 37:
> 38: import jdk.internal.access.SharedSecrets;
> 39: import jdk.internal.javac.Restricted;
Runtime has been touched for a while so you'll need to bump the copyright year.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604648529
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604650293
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 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 one > additional commit since the last revision: > > Add note on --illegal-native-access default value in the launcher help src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 288: > 286: * throw exception depending on the configuration. > 287: */ > 288: void ensureNativeAccess(Module m, Class owner, String methodName, > Class currentClass, boolean jni); It might be helpful to future maintainers if we put `@param` descriptions for these parameters. I had to re-read Module.enableNativeAccess to remember the difference between the owner class and the current class. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604644767
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 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 one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/foreign/package-info.java line 170:
> 168: * the special value {@code ALL-UNNAMED} can be used). Access to
> restricted methods
> 169: * from modules not listed by that option is deemed illegal.
> Clients can
> 170: * control how illegal access to restricted method is handled, using the
> command line
I assume this should be "to a restricted method is handled" or "to restricted
methods are handled", either would work here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604637950
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 00:38:07 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmti.xml line 1008: >> >>> 1006: function descriptions. Empty lists, arrays, sequences, etc are >>> 1007: returned as nullptr which is C programming language >>> 1008: null pointer. >> >> Perhaps instead something like >> >> "returned as a null pointer (C NULL or C++ >> nullptr)." >> >> "null pointer" is the generic phrase used in both the C and C++ standards. > > Thank you, Kim. I like this suggestion. Updated now. That part looks okay but I think all the parameters and error descriptions changed by JDK-8324680 will now need to change to use "null" instead of "nullptr". - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604344850
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 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 one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/System.java line 2023:
> 2021: * @throws NullPointerException if {@code filename} is {@code
> null}
> 2022: * @throws IllegalCallerException If the caller is in a module
> that
> 2023: * does not have native access enabled.
The exception description is fine, just noticed the other exception
descriptions start with a lowercase "if", this one is different.
src/java.base/share/man/java.1 line 587:
> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
> 587: command-line option.
"This mode disable all illegal native access except for those modules enabled
the --enable-native-access command-line option".
This can be read to mean that modules granted native access with the command
line option is also illegal native access An alternative is to make the second
part of the sentence a new sentence, something like "Only modules enabled by
the --enable-native-access command line option may perform native access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603878829
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603875920
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
On Thu, 16 May 2024 02:37:40 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) src/hotspot/share/prims/jvmti.xml line 1008: > 1006: function descriptions. Empty lists, arrays, sequences, etc are > 1007: returned as nullptr which is C programming language > 1008: null pointer. Shouldn't this be "NULL"? In any case, I think it would be helpful to expand this a bit to make it clear that usages of "nullptr" in parameter and error descriptions should be read or treated as "NULL" when developing an agent in C rather than C++. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1602803174
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v5]
On Wed, 15 May 2024 10:40:34 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 one
> additional commit since the last revision:
>
> Refine warning text for JNI method binding
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 871:
> 869: return IllegalNativeAccess.WARN;
> 870: } else {
> 871: fail("Value specified to --illegal-access not recognized:"
Typo in the message, should be --illegal-native-access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601898238
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 10:34:01 GMT, Maurizio Cimadamore wrote: > I don't fully agree that this option is not module related (which is why I > gave it that name). The very definition of illegal native access is related > to native access occurring from a module that is outside a specific set. So I > think it's 50/50 as to whether this option is module-related or not. Of > course I can fix the code if there's something clearly better. It maps here to a jdk.module.* property so I suppose it's okay. The functions were introduced to create jdk.module.* properties where the values were a set module names or a module path. Maybe these functions should be renamed at some point (not here) as they are more widely useful now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601421535
Re: Converting existing package.html files to package-info.java
On 15/05/2024 09:16, Magnus Ihse Bursie wrote: On 2024-05-15 02:13, Nizar Benalla wrote: Hello, I discovered after some recent work around javadoc that `javac` does not recognize `package.html` files, which predate `package-info.java`. Some tools that want to analyze doc comments need to deal with this in special ways. Maybe optional support for these files can be added into `javac`. But with only 21 `package.html` files in the JDK in modules that do not have internal in the name, I want to suggest simply converting them as it will be done once and won't need maintenance There are 9 package.html files I'd like to convert into package-info.java in java.management, one in java.management.rmi (and if this passes, one in java.naming) I want to hear what you think, Nizar That sounds like a straightforward and beneficial conversion. As you say, the package.html format is old, and provides no benefits to package-info.java. I agree. There was an effort a few years ago to replace html package descriptions with package-info.java but it seems that a few of them weren't done at the time. -Alan
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 00:54:43 GMT, David Holmes wrote:
>> 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?
That's my recollection too. The usage here isn' related to modules which makes
me wonder if this function should be renamed (not by this PR of course) of if
we should be using PropertyList_unique_add (with AddProperty,
WriteableProperty, InternalProperty) instead. There will be further GNU style
options coming that will likely need to map to an internal system property in
the same way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601002132
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 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 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598673962
Re: RFR: 8332098: Add missing `@since` tags to `jdk.jdi`
On Sun, 12 May 2024 01:58:38 GMT, Nizar Benalla wrote: > Please review this simple change where I add `@since` tags to the > package-info file of the following packages > ```com.sun.jdi > com.sun.jdi.connect > com.sun.jdi.connect.spi > com.sun.jdi.event > com.sun.jdi.request > > I used the unix `grep` command to find the oldest `@since` in each package > and used that value, as it's hard to get the source code of JDK 1-5 > TIA Marked as reviewed by alanb (Reviewer). JPDA was added to the JDK in JDK 1.3. The support for pluggable connectors (added com.sun.jdi.connect.spi package) was JDK 5. So I think this looks okay. - PR Review: https://git.openjdk.org/jdk/pull/19200#pullrequestreview-2051829289 PR Comment: https://git.openjdk.org/jdk/pull/19200#issuecomment-2106779971
Re: RFR: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
On Fri, 3 May 2024 18:31:21 GMT, Chris Plummer wrote: > What "debugging option" are you referring to. `-Djdk.tracePinnedThreads=full`. When this system property is set then it means the onPinned callback is running the printing code. This is happen in a transition when running with JVMTI enabled. It dates from early development in the loom repo and was a mistake to bring it into the main line. - PR Comment: https://git.openjdk.org/jdk/pull/19054#issuecomment-2093556941
Re: RFR: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
On Thu, 2 May 2024 22:40:02 GMT, Chris Plummer wrote: > It seems maybe we should instead be trying to avoid these events by > preloading the classes as was suggested as an option in the CR. I don't think preloading PinnedThreadPrinter will solve it completely. First usage could potentially load a lot of other classes, e.g. the current implementation uses streams and several other APIs. Going forward, this debugging option should be removed. It's already removed in the loom repo. It has been the source of several issues. - PR Comment: https://git.openjdk.org/jdk/pull/19054#issuecomment-2092406784
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v2]
On Thu, 2 May 2024 21:44:43 GMT, Chris Plummer wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: Corrections in: 1) JVMTI/JDWP spec; 2) test vthread checks; 3) >> test comments > > src/hotspot/share/prims/jvmti.xml line 8280: > >> 8278: >> 8279: The number of platform threads waiting to own this >> monitor, or 0 >> 8280: if only virtual threads are waiting or no threads are >> waiting > > This is now exactly the same as `waiter_count` above. I don't think this is > what you intended. Indeed, looks like the description for waiter_count has been pasted in here in error. - PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1588796716
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v2]
On Thu, 2 May 2024 07:33:09 GMT, Serguei Spitsyn wrote: >> The fix is to degrade virtual threads support in the JVM TI >> `GetObjectMonitorUsage` function so that it is specified to only return an >> owner when the owner is a platform thread. Also, virtual threads are not >> listed in the both `waiters` and `notify_waiters` lists returned in the >> `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI >> functions and events for virtual threads, we missed this one. >> >> The main motivation for degrading it now is that the object monitor >> implementation is being updated to allow virtual threads unmount while >> owning monitors. It would add overhead to record monitor usage when >> freezing/unmount, overhead that couldn't be tied to a JVMTI capability as >> the capability can be enabled at any time. >> >> `GetObjectMonitorUsage` was broken for 20+ years >> ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports >> so it seems unlikely that the function is widely used. Degrading it to only >> return an owner when the owner is a platform thread has no compatibility >> impact for tooling that uses it in conjunction with `HotSpot` thread dumps >> or `ThreadMXBean`. >> >> One other point about `GetObjectMonitorUsage` is that it pre-dates >> j.u.concurrent in Java 5 so it can't be used to get a full picture of the >> lock usage in a program. >> >> The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the >> JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` >> methods are updated to match the JVM TI spec. >> >> Also, please, review the related CSR and Release Note: >> - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade >> virtual thread support for GetObjectMonitorUsage >> - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: >> degrade virtual thread support for GetObjectMonitorUsage >> >> Testing: >> - tested impacted and updated tests locally >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: Corrections in: 1) JVMTI/JDWP spec; 2) test vthread checks; 3) test > comments The update to the API specs looks okay. I think it was just a cut & paste error that the wrong text was copied into the description of the waiter_count and notify_waiter_count fields. I assume you'll update the CSR so it has the updated text. - PR Comment: https://git.openjdk.org/jdk/pull/19030#issuecomment-2090636854
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v13]
On Mon, 18 Mar 2024 23:45:29 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: rename is_interrupted() to get_and_clear_interrupted(); update > comment I don't have any more comments either, the src changes looks okay. I have not reviewed the test. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18093#pullrequestreview-1945459563
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests
On Mon, 18 Mar 2024 16:47:24 GMT, Bill Huang wrote: > This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. test/jdk/java/nio/channels/unixdomain/Bind.java line 191: > 189: server.bind(null); > 190: UnixDomainSocketAddress usa = > (UnixDomainSocketAddress)server.getLocalAddress(); > 191: usa.getPath().toFile().deleteOnExit(); The test already deletes the file, I think you just want a try-finally here, same comment on a few other tests. - PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1529408542
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v12]
On Sat, 16 Mar 2024 22:33:49 GMT, Serguei Spitsyn wrote:
>> Please, review this fix correcting the JVMTI `RawMonitorWait()`
>> implementation.
>> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to
>> update the interrupt status of the interrupted waiting thread. The issue is
>> that when it calls `jt->is_interrupted(true)` to fetch and clear the
>> `interrupt status` of the virtual thread, this information is not propagated
>> to the `java.lang.Thread` instance.
>> In the `VirtualThread` class implementation the `interrupt status` for
>> virtual thread is stored for both virtual and carrier threads. It is because
>> the implementation of object monitors for virtual threads is based on
>> pinning virtual threads, and so, always operates on carrier thread. The fix
>> is to clear the interrupt status for both virtual and carrier
>> `java.lang.Thread` instances.
>>
>> Testing:
>> - tested with new test
>> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is
>> passed with the fix and failed without it
>> - ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: made current changes limitedto just RawMonitorWait
src/hotspot/share/runtime/javaThread.cpp line 596:
> 594: // Checks and clears the interrupt status for platform or virtual thread.
> 595: // Used by the JVMTI RawMonitorWait only.
> 596: bool JavaThread::is_interrupted() {
Reading code that uses JavaThread::is_interrupted() won't be clear that it
resets the interrupt status. Can this be something that
get_and_clear_interrupted or something that makes it clearer?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1527652412
Re: RFR: JDK-8327468: Do not restart close if errno is EINTR [macOS/linux]
On Fri, 8 Mar 2024 10:12:06 GMT, Matthias Baesken wrote:
> There are a number of places remaining in the linux/macOS native JDK codebase
> where we use the RESTARTABLE macro with close, but this is unwanted on
> Linux/macOS.
src/jdk.attach/linux/native/libattach/VirtualMachineImpl.c line 200:
> 198: if (res == -1) {
> 199: JNU_ThrowIOExceptionWithLastError(env, "close");
> 200: }
I assume it would be better to not stop when errno is EINTR. If there is a
profiler or some other tool firing signals at threads then there isn't anything
that can be done here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18164#discussion_r1517532415
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v8]
On Fri, 8 Mar 2024 05:56:24 GMT, Serguei Spitsyn wrote: > > Can you check if > > vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002/TestDescription.java > > is passing now? That test is a bit annoying in that it fails every time we > > touch j.l.Object so you might have to update > > BI04/bi04t002/newclass02/java.base/java/lang/Object.java again. > > Checked, it is passed now. Can you look at? That test has a copy of Object.java so needs to be updated every time we touch Object.java. It's an annoying tax and I hope there is a JBS issue to replace that test. In this case, it will be benign because the instrumented version will just clear the interrupt status of both threads twice, but at the same time, it will be out of sync with Object.wait. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1985153918
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Thu, 7 Mar 2024 06:45:01 GMT, David Holmes wrote: >> Fixed as suggested by Alan. >> >>> Also need to think through if Object.wait will need to be changed as part >>> of this. >> >> Still need to look at this. > > Use of `ObjectLocker` here will introduce a new pinning point for Loom. We > have been removing as many uses of `ObjectLocker` as we can. I also think > this will need to be moved back to Java code when the pinning currently > inherent in calling `Object.wait` is addressed. Yes, and it may be that once Object.wait is implemented that we can remove the need to propagate the interrupt status (there are some TBDs here). I think the change here is okay for now but we still have the choice of limiting the change to just JVMTI RawMonitorWait. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1515760599
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Wed, 6 Mar 2024 16:30:23 GMT, Matthias Baesken wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Introduce windows jni_util_md.h Latest version looks good to me. As have been pointed out, there at least 2 files where the copyright header was updated but there are no changes, I assume left over from previous iterations. I assume the RESTARTABLE-close in libattach/VirtualMachineImpl.c will be tracked as a separate issue. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1921724534
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v8]
On Thu, 7 Mar 2024 00:13:56 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: dropped the catch of InterruptedException in Object.wait Can you check if vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002/TestDescription.java is passing now? That test is a bit annoying in that it fails every time we touch j.l.Object so you might have to update BI04/bi04t002/newclass02/java.base/java/lang/Object.java again. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1982739813
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v7]
On Wed, 6 Mar 2024 10:44:15 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > optimize holding the interruptLock in JavaThread::is_interrupted I think the updated implementation looks right. I think you can drop the catch of InterruptedException in Object.wait now. Easy to check this by running test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java as it has tests for interrupting Object.wait. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1981574037
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v2]
On Wed, 6 Mar 2024 15:45:16 GMT, Matthias Baesken wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > introduce unix jni_util_md.h src/java.base/aix/native/libnio/ch/Pollset.c line 29: > 27: #include "jni.h" > 28: #include "jni_util.h" > 29: #include "jni_util_md.h" When I suggested jni_util_md.h then I meant create one for Unix and another for Windows, then have jni_util.h include it. This will avoid needing to add an include to all these files. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514724921
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
On Wed, 6 Mar 2024 14:25:09 GMT, Matthias Baesken wrote: > We could maybe also use the existing > https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/include/jni_md.h > - what do you think ? jni_md.h goes into the JDK run-time image (for jni.h to include) so I don't think we should be changing that. jni_util.* is JDK internal so I think we're just missing a platform dependent include file. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514586726
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
On Wed, 6 Mar 2024 09:26:25 GMT, Matthias Baesken wrote: > We define the RESTARTABLE macro again and again at a lot of places in the JDK > native codebase. This could be centralized to avoid repeating it again and > again ! src/java.base/share/native/libjava/jni_util.h line 243: > 241: } while((_result == -1) && (errno == EINTR)); \ > 242: } while(0) > 243: jni_util.h is for all platforms so not the right place for a Unix specific macro. We need think where the right place for this, might have to introduce a jni_util_md.h. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514555111
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Wed, 6 Mar 2024 09:14:18 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > removed trailing spaces in javaClasses.cpp and libInterruptRawMonitor.cpp src/hotspot/share/runtime/javaThread.cpp line 573: > 571: > 572: Handle thread_h(current, vthread_or_thread()); > 573: ObjectLocker lock(Handle(current, > java_lang_Thread::interrupt_lock(thread_h())), current); For this version then I assume you should limit it when its a virtual thread and when clear_interrupted is true. Also need to think through if Object.wait will need to be changed as part of this. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1514126817
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 01:06:32 GMT, Serguei Spitsyn wrote: >>> AFAIK, as it is not a good idea to post the JVMTI events recursively >> >> We already do. When the debug agent is handling a JVMTI event and >> RawMonitorWait is interrupted, that results in the debug agent later on >> calling InterrtupThread before exiting the event handler. For virtual >> threads that results in an upcall to Thread.interrupt(), and therefore the >> potential for a JVMTI event to be triggered (and that is exactly what >> happens frequently in the com/sun/jdi/InterruptHang.java test). At least in >> that case the debug agent is somewhat in control of the situation because it >> is just doing "cleanup" before exiting the event handler. For example, no >> locks are being held. Having RawMonitorWait do a java upcall sounds a >> potential big can of worms. > > Thank you for sharing this, Chris. It sounds like we need to introduce a > mechanism to temporarily hide the JVMTI events. The question is if it is > worth the complexity we add with it, especially if it is used just in a > couple of cases. > If we do it with a Java upcall to the `VirtualThread.getAndClearInterrupt()` > then we also have to skip the JVMTI events, AFAIK, as it is not a good idea > to post the JVMTI events recursively. I meant it would need to use ObjectLocker to hold the lock in the VM when changing both fields. It should only need to do this for RawMonitorWait at this time. For Object.wait, the clearing of the interrupt status is done in the Java code, something that will change once Object.wait changes to freeze/unmount when not pinned. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1512274259
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v2]
On Mon, 4 Mar 2024 13:24:05 GMT, Kevin Walls wrote: >> The deprecated Subject Delegation feature in JMX will be removed. >> >> This was marked in JDK 21 as deprecated for removal (JDK-8298966). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > RMIConnection comments update src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java line 126: > 124: * @param className The class name of the MBean to be instantiated. > 125: * @param name The object name of the MBean. May be null. > 126: * @param delegationSubject No longer supported and should be null. I think this interface will require all methods that take a delegation subject to specify UOE then the subject is not null. Yes, this will invalidate implementations that are outside of the JDK but such implementations need to be updated anyway. src/java.management/share/classes/javax/management/remote/JMXConnector.java line 158: > 156: */ > 157: @Deprecated(since="21", forRemoval=true) > 158: public default MBeanServerConnection > getMBeanServerConnection(Subject delegationSubject) Specifying the UOE in the param description is problematic. I think you want to specify delegationSubject as not used, must be null, then declare UOE as one of the exceptions thrown by the method. - PR Review Comment: https://git.openjdk.org/jdk/pull/18025#discussion_r1511174715 PR Review Comment: https://git.openjdk.org/jdk/pull/18025#discussion_r1511172106
Re: Class loader leaked when ForkJoinPool is used in loaded class, with Java 19+
core-libs-dev is the place to send this.
-Alan
On 04/03/2024 12:11, S A wrote:
Hi all,
after moving our application to Java 21 (up from Java 17), we noticed
a class loader leak. A memory snapshot points to a ProtectionDomain
object held by a ForkJoinWorkerThread, the protection domain holds the
class loader and prevents GC.
To reproduce outside of our application, we use this snippet:
import java.lang.ref.WeakReference;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Paths;
public class TestClassloaderLeak {
public static void main(String[] args) throws Exception {
WeakReference wr = load();
gc();
System.out.println("wr=" + wr.get());
}
private static void gc() {
System.gc();
System.runFinalization();
}
private static WeakReference load() throws Exception {
URLClassLoader cl = new URLClassLoader(new URL[] { url() },
TestClassloaderLeak.class.getClassLoader());
WeakReference wr = new WeakReference<>(cl);
Class ca = cl.loadClass("A");
ca.getConstructor(String.class).newInstance("A");
cl.close();
return wr;
}
private static URL url() throws MalformedURLException {
return Paths.get("/data/tmp/testleak/lib/").toUri().toURL();
}
}
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.ForkJoinTask;
public class A {
public final String s;
public A(String s) {
this.s = s;
ForkJoinTask task = ForkJoinPool.commonPool().submit(() ->
{ System.out.println("A constructor"); });
try {
task.get();
} catch (Exception e) {
e.printStackTrace(System.out);
}
}
}
Place the compiled A.class at the hard-coded location
"/data/tmp/testleak/lib/", then run the main method with JVM flags
"-Xlog:class+unload". Observe that no class is unloaded, which is not
the case if the main() code runs twice, or if the snippet is executed
e.g. with Java 17.
It seems each time ForkJoinPool creates a new ForkJoinWorkerThread
from a loaded class, it is no longer possible to GC the class loader
of the class using ForkJoinPool.
The leak occurs after this commit, with Java 19+:
https://github.com/openjdk/jdk19u/commit/00e6c63cd12e3f92d0c1d007aab4f74915616ffb
What possible workarounds can we use to avoid this problem?
Essentially, our application loads and runs user code. The user
changes their code, runs their code again - we use a new class loader
to run the changed code in the same JVM. We unload the previous class
loader, to free up memory and to avoid problems with hot swapping code
(an old version of a loaded class can cause an error when trying to
hot swap the latest loaded version of that class). So the leaked class
loader is giving us trouble.
Best regards and thanks,
Simeon
