Integrated: 8344011: Remove usage of security manager from Class and reflective APIs
On Wed, 13 Nov 2024 10:32:34 GMT, Alan Bateman wrote: > Remove code required for the now defunct SecurityManager execution mode from > java.lang.Class, friends, and reflection APIs. Careful review is required so > I've set Reviewer to 2. I've tried to keep the changes as easy to review as > possible and not go over board with cleanup. > > sun.reflect.misc.ReflectUtil are been hollowed out. A future pass will remove > empty methods and qualified exports once the changes in "far away" code and > modules is done. > > In Lookup's class description, the removal of the sentence "avoid package > access checks for classes accessible to the lookup class" and the link to > the removed "Security manager interactions" section is in > discussion/non-normative text, just missed in the JEP 486 update that remove > the linked section. > > runtime/cds/appcds/StaticArchiveWithLambda.java is updated as creating the > archive no longer skips a generated class. > > Testing: tier1-5 This pull request has now been integrated. Changeset: abacece8 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/abacece8265996aaec888c8f109f2e476ec7a8e3 Stats: 1254 lines in 26 files changed: 6 ins; 1094 del; 154 mod 8344011: Remove usage of security manager from Class and reflective APIs Reviewed-by: liach, yzheng, rriggs - PR: https://git.openjdk.org/jdk/pull/22063
Re: RFR: 8344011: Remove usage of security manager from Class and reflective APIs [v2]
On Wed, 13 Nov 2024 22:26:35 GMT, Sean Mullan wrote: >> 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 three additional >> commits since the last revision: >> >> - StaticArchiveWithLambda no longer skips dynamically generated class when >> creating archive >> - Merge branch 'master' into JDK-8344011 >> - Initial commit > > src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line > 1: > >> 1: /* > > Line 82: is `GetReflectionFactoryAction` still needed? There's a sequencing issue here, this can't be removed until Roger's work on removing SM use from serialization are in. In general, we'll the efforts to remove usage of the SM from the JDK will require multiple passes due to dependencies like this. - PR Review Comment: https://git.openjdk.org/jdk/pull/22063#discussion_r1841596240
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io [v3]
On Wed, 13 Nov 2024 19:00:36 GMT, Brian Burkhalter wrote:
>> Uses of `InternalLock` are removed and `synchronized` is reinstated.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8343039: Remove JavaIOPrint{Stream,Writer}Access and the use thereof
src/java.base/share/classes/java/io/BufferedInputStream.java line 242:
> 240: initialSize = size;
> 241: if (getClass() == BufferedInputStream.class) {
> 242: buf = EMPTY;
Maybe keep "lazily create buffer when not subclassed" as the comment.
src/java.base/share/classes/java/io/BufferedOutputStream.java line 88:
> 86: }
> 87:
> 88: this.buf = new byte[initialSize]; // resizable if initialSize <
> maxSize
Something has got messed up here, it creates the byte[] twice now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22048#discussion_r1841062387
PR Review Comment: https://git.openjdk.org/jdk/pull/22048#discussion_r1841064710
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io [v5]
On Wed, 13 Nov 2024 20:16:44 GMT, Brian Burkhalter wrote: >> Uses of `InternalLock` are removed and `synchronized` is reinstated. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8343039: Remove InternalLock reference from > java/lang/ProcessBuilder/Basic.java test/jdk/java/lang/ProcessBuilder/Basic.java line 2876: Can you check if java.base/jdk.internal.misc can be dropped from the `@modules` in the test description? - PR Review Comment: https://git.openjdk.org/jdk/pull/22048#discussion_r184376
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io [v3]
On Wed, 13 Nov 2024 19:00:36 GMT, Brian Burkhalter wrote:
>> Uses of `InternalLock` are removed and `synchronized` is reinstated.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8343039: Remove JavaIOPrint{Stream,Writer}Access and the use thereof
I did a pass over the latest update, a lot of changes, it's good that Chen is
reviewing too as would be too easy to introduce a bug with this refactor.
-
PR Comment: https://git.openjdk.org/jdk/pull/22048#issuecomment-2474647438
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io [v3]
On Wed, 13 Nov 2024 19:00:36 GMT, Brian Burkhalter wrote:
>> Uses of `InternalLock` are removed and `synchronized` is reinstated.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8343039: Remove JavaIOPrint{Stream,Writer}Access and the use thereof
src/java.base/share/classes/java/io/PrintWriter.java line 422:
> 420: */
> 421: public void close() {
> 422: Object lock = this.lock;
I assume L422 can be deleted.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22048#discussion_r1841074351
Re: RFR: 8344011: Remove usage of security manager from Class and reflective APIs
On Wed, 13 Nov 2024 10:32:34 GMT, Alan Bateman wrote: > Remove code required for the now defunct SecurityManager execution mode from > java.lang.Class, friends, and reflection APIs. Careful review is required so > I've set Reviewer to 2. I've tried to keep the changes as easy to review as > possible and not go over board with cleanup. > > sun.reflect.misc.ReflectUtil are been hollowed out. A future pass will remove > empty methods and qualified exports once the changes in "far away" code and > modules is done. > > In Lookup's class description, the removal of the sentence "avoid package > access checks for classes accessible to the lookup class" and the link to > the removed "Security manager interactions" section is in > discussion/non-normative text, just missed in the JEP 486 update that remove > the linked section. > > runtime/cds/appcds/StaticArchiveWithLambda.java is updated as creating the > archive no longer skips a generated class. > > Testing: tier1-5 runtime/cds/appcds/StaticArchiveWithLambda.java is update to not assume that the archive create skips a generated classes (confirmed with Ioi). - PR Comment: https://git.openjdk.org/jdk/pull/22063#issuecomment-2474485817
Re: RFR: 8344011: Remove usage of security manager from Class and reflective APIs [v2]
> Remove code required for the now defunct SecurityManager execution mode from > java.lang.Class, friends, and reflection APIs. Careful review is required so > I've set Reviewer to 2. I've tried to keep the changes as easy to review as > possible and not go over board with cleanup. > > sun.reflect.misc.ReflectUtil are been hollowed out. A future pass will remove > empty methods and qualified exports once the changes in "far away" code and > modules is done. > > In Lookup's class description, the removal of the sentence "avoid package > access checks for classes accessible to the lookup class" and the link to > the removed "Security manager interactions" section is in > discussion/non-normative text, just missed in the JEP 486 update that remove > the linked section. > > runtime/cds/appcds/StaticArchiveWithLambda.java is updated as creating the > archive no longer skips a generated class. > > Testing: tier1-5 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 three additional commits since the last revision: - StaticArchiveWithLambda no longer skips dynamically generated class when creating archive - Merge branch 'master' into JDK-8344011 - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/22063/files - new: https://git.openjdk.org/jdk/pull/22063/files/b46c4fea..1687ad54 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22063&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22063&range=00-01 Stats: 7342 lines in 85 files changed: 5659 ins; 433 del; 1250 mod Patch: https://git.openjdk.org/jdk/pull/22063.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22063/head:pull/22063 PR: https://git.openjdk.org/jdk/pull/22063
Integrated: 8343981: Remove usage of security manager from Thread and related classes
On Tue, 12 Nov 2024 10:41:48 GMT, Alan Bateman wrote: > Removes the SecurityManager usage from Thread + friends. > > In Thread, the getContextClassLoader method is no longer caller-sensitive > method. > > JavaLangAccess.newThreadWithAcc is removed and jdk.internal.access is no > longer exported to java.naming. The usage of newThreadWithAcc is removed from > com.sun.jndi.ldap.VersionHelper. There will be further work on the > java.naming module to remove usage of SM, the change here is specific to the > usage of ewThreadWithAcc. This pull request has now been integrated. Changeset: 5e01c40b Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/5e01c40b19a5bf4d0266747ca73aca4193799d97 Stats: 282 lines in 10 files changed: 0 ins; 228 del; 54 mod 8343981: Remove usage of security manager from Thread and related classes Reviewed-by: rriggs, yzheng - PR: https://git.openjdk.org/jdk/pull/22035
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io
On Wed, 13 Nov 2024 16:14:11 GMT, Brian Burkhalter wrote:
> `InternalLock` is also in `sun.nio.cs.Stream{De,En}coder`.
Yes, these two will need to be updated. I don't mind if this is split up into
several PRs but if InternalLock isn't removed by the current PR then you'll
need to change its title :-)
-
PR Comment: https://git.openjdk.org/jdk/pull/22048#issuecomment-2474195205
Re: RFR: 8344034: Remove security manager dependency in Serialization [v4]
On Wed, 13 Nov 2024 14:44:56 GMT, Roger Riggs wrote:
>> After [JDK-8338411](https://bugs.openjdk.org/browse/JDK-8338411),
>> Serialization implementation dependencies on SecurityManager, doPrivildged,
>> and AccessController are removed.
>> Some refactoring to cleanup the remaining code is expected.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove overlooked SecurityManager use in ObjectInputFilter and
> ObjectStreamClass
src/java.base/share/classes/java/io/ObjectStreamField.java line 166:
> 164: @SuppressWarnings("removal")
> 165: @CallerSensitive
> 166: public Class getType() {
This method is no longer caller sensitive, also no need to import ReflectUtil
now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22041#discussion_r1840484140
Re: RFR: 8344039: Remove security manager dependency in java.time [v2]
On Wed, 13 Nov 2024 14:13:59 GMT, Sean Mullan wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unnecessary suppress warnings removal
>> ServiceLoader configuration exceptions (in a static initialization) are
>> rethrown as Error.
>> Remove dependency on SecurityException thrown by ServiceLoader (as
>> obsolete)
>
> src/java.base/share/classes/java/time/zone/ZoneRulesProvider.java line 1:
>
>> 1: /*
>
> Lines 171-172:
>
> if (ex.getCause() instanceof SecurityException) {
> continue; // ignore the security exception, try the next
> provider
> }
>
> You may be able to remove this code, but may be better to leave it as-is for
> now until the SL code has been checked for cleanup.
The SL changes are in JDK-8344011.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22042#discussion_r1840444817
Re: RFR: 8344034: Remove security manager dependency in Serialization [v3]
On Tue, 12 Nov 2024 21:59:43 GMT, Roger Riggs wrote: >> After [JDK-8338411](https://bugs.openjdk.org/browse/JDK-8338411), >> Serialization implementation dependencies on SecurityManager, doPrivildged, >> and AccessController are removed. >> Some refactoring to cleanup the remaining code is expected. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 227 commits: > > - Cleanup of suppress warnings and caller sensitive from review comments. > - Merge branch 'master' into 8344034-sm-removal-serialization > - JDK-8344034: Remove security manager dependency in Serialization > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Move remaining JEP 486 failing tests into correct groups. > - Move JEP 486 failing tests into hotspot_runtime group. > - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - ... and 217 more: https://git.openjdk.org/jdk/compare/63eb4853...3f850839 ObjectInputFilter.setSerialFilter and setSerialFilterFactory, have they been missed? - PR Comment: https://git.openjdk.org/jdk/pull/22041#issuecomment-2473301863
RFR: 8344011: Remove usage of security manager from Class and reflective APIs
Remove code required needed for the now defunct SecurityManager execution mode from java.lang.Class, friends, and reflection APIs. Careful review is required so I've set Reviewer to 2. I've tried to keep the changes as easy to review as possible and not go over board with cleanup. sun.reflect.misc.ReflectUtil are been hollowed out. A future pass will be removed to remove empty methods and qualified exports once the changes in "far away" code and modules is done. runtime/cds/appcds/StaticArchiveWithLambda.java is temporarily excluded as the loading of a dynamically generated class at archive time. I'm checking with Calvin and Ioi on whether this test should be updated or replaced. Testing: tier1-5 - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/22063/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22063&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8344011 Stats: 1254 lines in 26 files changed: 8 ins; 1094 del; 152 mod Patch: https://git.openjdk.org/jdk/pull/22063.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22063/head:pull/22063 PR: https://git.openjdk.org/jdk/pull/22063
Re: RFR: 8342486: Implement JEP draft: Structured Concurrency (Fifth Preview) [v2]
> Changes for [JEP draft: Structured Concurrency (Fifth > Preview)](https://openjdk.org/jeps/8340343). The JEP isn't on the technical > roadmap yet. The proposal is to re-preview the API with some changes, > specifically: > > - A > [StructuredTaskScope](https://download.java.net/java/early_access/loom/docs/api/java.base/java/util/concurrent/StructuredTaskScope.html) > is now opened with a static factory method instead of a constructor. Once > opened, the API usage is unchanged: fork subtasks individually, join them as > a unit, process outcome, and close. > - In conjunction with moving to using a static open method, policy and > desired outcome is now selected by specifying a Joiner to the open method > rather than extending STS. A Joiner handles subtask completion and produces > the result for join to return. Joiner.onComplete is the equivalent of > overriding handleComplete previously. This change means that the subclasses > ShutdownOnFailure and ShutdownOnSuccess are removed, replaced by factory > methods on Joiner to get an equivalent Joiner. > - The join method is changed to return the result or throw > STS.FailedException, replacing the need for an API in subclasses to obtain > the outcome. This removes the hazard that was forgetting to call > throwIfFailed to propagate exceptions. > - Configuration that was provided with parameters for the constructor is > changed so that can be provided by a configuration function. > - joinUntil is replaced by allowing a timeout be configured by the > configuration function. This allows the timeout to apply the scope rather > than the join method. > > The underlying implementation is unchanged except that ThreadFlock.shutdown > and wakeup methods are no longer confined. The STS API implementation moves > to non-public StructuedTaskScopeImpl because STS is now an interface. A > non-public Joiners class is added with the built-in Joiner implementations. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Pull latest API docs + impl from loom repo - Merge branch 'master' into JDK-8342486 - Sync up from loom repo - Initial commit - Changes: https://git.openjdk.org/jdk/pull/21934/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21934&range=01 Stats: 4079 lines in 15 files changed: 1852 ins; 1498 del; 729 mod Patch: https://git.openjdk.org/jdk/pull/21934.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21934/head:pull/21934 PR: https://git.openjdk.org/jdk/pull/21934
Re: RFR: 8342380: Implement JEP draft: Warn about use of Memory-Access Methods in `sun.misc.Unsafe` [v2]
> Change default value of the command line option > --sun-misc-unsafe-memory-access from "allow" to "warn". 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 five additional commits since the last revision: - Merge branch 'master' into JDK-8342380 - Update test - Merge branch 'master' into JDK-8342380 - Update usage/man page - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/21762/files - new: https://git.openjdk.org/jdk/pull/21762/files/85264b80..a9f99896 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21762&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21762&range=00-01 Stats: 225679 lines in 3239 files changed: 123548 ins; 77490 del; 24641 mod Patch: https://git.openjdk.org/jdk/pull/21762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21762/head:pull/21762 PR: https://git.openjdk.org/jdk/pull/21762
Re: RFR: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec [v4]
On Wed, 13 Nov 2024 10:58:41 GMT, Jaikiran Pai wrote:
>> src/java.base/windows/classes/java/lang/ProcessImpl.java line 516:
>>
>>> 514: }
>>> 515:
>>> 516: return null;
>>
>> I think this is an oversight - returning `null` from the constructor would
>> be an compile error.
>
> Actually, looking at the GitHub actions job, it appears to have caught one
> other compilation error in this file:
>
>
> jdk\jdk\src\java.base\windows\classes\java\lang\ProcessImpl.java:423: error:
> ';' expected
> final String value =
> System.getProperty("jdk.lang.Process.allowAmbiguousCommands", "true"));
>
> ^
> 1 error
I assume Roger will run at least tier1 on all platforms before integrating.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22024#discussion_r1840100951
Re: RFR: 8310996: Add JFR event for connect operations [v4]
On Wed, 6 Nov 2024 00:15:44 GMT, Tim Prinzing wrote: >> Adds a JFR event for socket connect operations. >> >> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also >> check for connect events. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > suggested changes @tprinzing Are you planning on all the tests? - PR Comment: https://git.openjdk.org/jdk/pull/21528#issuecomment-2473184324
Re: RFR: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec [v4]
On Wed, 13 Nov 2024 02:17:23 GMT, Roger Riggs wrote: >> Refactor removing the dependencies on SecurityManager, doPrivileged, and >> AccessController. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Revert unnecessary copyright change in ProcessEnvironment.java. > Fix code style in ProcessImpl > Drop unneeded suppress warnings. > Remove obsolete @throws SecurityExceptions I did another pass on this, looks good. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22024#pullrequestreview-2432569542
Re: RFR: 8343984: Fix Unsafe address overflow [v8]
On Tue, 12 Nov 2024 23:34:48 GMT, Shaojin Wen wrote: >> In the JDK code, there are some places that may cause Unsafe offset >> overflow. The probability of occurrence is low, but if it occurs, it will >> cause JVM crash. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/java/lang/StringLatin1.java > > Co-authored-by: Andrey Turbanov Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/22027#pullrequestreview-2432158366
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io
On Tue, 12 Nov 2024 21:45:41 GMT, Brian Burkhalter wrote: > Uses of `InternalLock` are removed and `synchronized` is reinstated. I think we should remove jdk.internal.misc.InternalLock and the usages in Throwable as part of this change. I don't mind if JavaIOPrintStreamAccess is removed now or in a later PR. - PR Comment: https://git.openjdk.org/jdk/pull/22048#issuecomment-2472658184
Re: RFR: 8343981: Remove usage of security manager from Thread and related classes [v4]
On Tue, 12 Nov 2024 22:08:42 GMT, Roger Riggs wrote:
>> Alan Bateman has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 227 commits:
>>
>> - Merge branch 'master' into JDK-8343981
>> - Merge commit 'db85090553ab14a84c3ed0a2604dd56c5b6e6982' into JDK-8343981
>> - Initial commit
>> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>> - Merge branch 'master' into jep486
>> - Move remaining JEP 486 failing tests into correct groups.
>> - Move JEP 486 failing tests into hotspot_runtime group.
>> - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing
>> - Merge branch 'master' into jep486
>> - Merge branch 'master' into jep486
>> - ... and 217 more: https://git.openjdk.org/jdk/compare/63eb4853...636ebd19
>
> src/java.naming/share/classes/com/sun/jndi/ldap/VersionHelper.java line 111:
>
>> 109:
>> 110: Thread createThread(Runnable r) {
>> 111: return new Thread(r);
>
> Can the refactoring go back to the callers of createThread() to just create
> threads themselves.
> And remove this trivial method.
Yes, but in the PR that does the cleanup of java.naming module.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22035#discussion_r1839618104
Re: RFC: Introduce JDK property jdk.patched for indicating --patch-module at runtime
On 13/11/2024 03:44, David Holmes wrote: The VM already sets properties like jdk.module.patch.N=<...> when processing --patch-mods. Doesn't that suffice if all you need is a boolean flag to indicate any patching has occurred? I would have thought you'd like to know which module has been patched, or to be able to ask if a given module has been patch - in which case an actual API method on java.lang.Module would seem reasonable. Or is this an area where module patching is only part of the implementation of Modules in the JDK, not part of the actual Java SE Module specification? It's very JDK specific and --patch-module is complex due to being a repeated option. The jdk.module.patch. properties are the internal way to communicate the values provide to --patch-module during startup. The properties are removed to avoid wider parts of the system having a dependency on its private protocol. -Alan
Re: RFR: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io
On Wed, 13 Nov 2024 03:11:41 GMT, Chen Liang wrote: >> Uses of `InternalLock` are removed and `synchronized` is reinstated. > > src/java.base/share/classes/java/io/BufferedOutputStream.java line 88: > >> 86: } >> 87: >> 88: this.buf = new byte[initialSize]; // resizable if initialSize < >> maxSize > > Same remark as for BIS. > > Also this initial vs max size feature was added when virtual thread was first > implemented as the internal locks. Is this implementation detail still > necessary for the virtual threads? Think about 100_000 virtual threads, all blocked on sockets with input/output streams that have wrapped with buffered streams, as in BIS and BOS. This is the motivation for using a reduced initial size. - PR Review Comment: https://git.openjdk.org/jdk/pull/22048#discussion_r1839629562
Re: RFR: 8340133: Add concise usage message to the java executable [v8]
On Tue, 12 Nov 2024 18:28:33 GMT, Kevin Bourrillion wrote: > About the version string: I think I first proposed including it. If we don't, > then I think that `-version` ought to be in the shortlist of options we show > here (not just in full help), so it doesn't become a _three_ step goose chase > for the user to find it. > > To me, it feels more user-friendly to just give the version, than to tell the > user how they can ask for it and make them ask again. And it seems to take > about the same amount of space either way. But maybe there are > counterarguments I don't know of. I'm not opposed to it but if is printed then I think it should be the same as the first line of the java -version output. Believe it or not, the launcher has weirdness in this area where it prints slightly different version output when it's printing to stdout or stderr. The current changes in the PR use the format for stdout but it is printing to stderr. So maybe it will look better if that is fixed. To see what I mean, try `java` && `java -version` to see the difference. - PR Comment: https://git.openjdk.org/jdk/pull/21411#issuecomment-2471358404
Re: RFR: 8343981: Remove usage of security manager from Thread and related classes [v4]
On Tue, 12 Nov 2024 19:06:31 GMT, Daniel Fuchs wrote: > The proposed changes look reasonable to me. Good simplification of the code. > I had some uncertainty about the JNDI changes but I couldn't find any place > in JNDI where a Subject would be extracted from the ACC associated with the > created thread so I'd say that looks fine. ACC is dead and Subject is re-implemented on ScopedValue now so I think we are okay. There will be many other places where we will also drop the capture and use of the ACC. - PR Comment: https://git.openjdk.org/jdk/pull/22035#issuecomment-2471365088
Re: RFR: 8343981: Remove usage of security manager from Thread and related classes [v4]
> Removes the SecurityManager usage from Thread + friends. > > In Thread, the getContextClassLoader method is no longer caller-sensitive > method. > > JavaLangAccess.newThreadWithAcc is removed and jdk.internal.access is no > longer exported to java.naming. The usage of newThreadWithAcc is removed from > com.sun.jndi.ldap.VersionHelper. There will be further work on the > java.naming module to remove usage of SM, the change here is specific to the > usage of ewThreadWithAcc. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 227 commits: - Merge branch 'master' into JDK-8343981 - Merge commit 'db85090553ab14a84c3ed0a2604dd56c5b6e6982' into JDK-8343981 - Initial commit - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Merge branch 'master' into jep486 - Move remaining JEP 486 failing tests into correct groups. - Move JEP 486 failing tests into hotspot_runtime group. - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing - Merge branch 'master' into jep486 - Merge branch 'master' into jep486 - ... and 217 more: https://git.openjdk.org/jdk/compare/63eb4853...636ebd19 - Changes: https://git.openjdk.org/jdk/pull/22035/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22035&range=03 Stats: 282 lines in 10 files changed: 0 ins; 228 del; 54 mod Patch: https://git.openjdk.org/jdk/pull/22035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22035/head:pull/22035 PR: https://git.openjdk.org/jdk/pull/22035
Re: RFR: 8343981: Remove usage of security manager from Thread and related classes [v3]
> Removes the SecurityManager usage from Thread + friends. > > In Thread, the getContextClassLoader method is no longer caller-sensitive > method. > > JavaLangAccess.newThreadWithAcc is removed and jdk.internal.access is no > longer exported to java.naming. The usage of newThreadWithAcc is removed from > com.sun.jndi.ldap.VersionHelper. There will be further work on the > java.naming module to remove usage of SM, the change here is specific to the > usage of ewThreadWithAcc. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 226 commits: - Merge commit 'db85090553ab14a84c3ed0a2604dd56c5b6e6982' into JDK-8343981 - Initial commit - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Merge branch 'master' into jep486 - Move remaining JEP 486 failing tests into correct groups. - Move JEP 486 failing tests into hotspot_runtime group. - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing - Merge branch 'master' into jep486 - Merge branch 'master' into jep486 - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - ... and 216 more: https://git.openjdk.org/jdk/compare/db850905...4f320f92 - Changes: https://git.openjdk.org/jdk/pull/22035/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22035&range=02 Stats: 283 lines in 11 files changed: 0 ins; 228 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/22035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22035/head:pull/22035 PR: https://git.openjdk.org/jdk/pull/22035
Re: RFR: 8343984: Fix Unsafe address overflow [v6]
On Tue, 12 Nov 2024 15:54:00 GMT, Shaojin Wen wrote: >> In the JDK code, there are some places that may cause Unsafe offset >> overflow. The probability of occurrence is low, but if it occurs, it will >> cause JVM crash. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > more fix unsafe address overflow Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/22027#pullrequestreview-2430082452
Re: RFR: 8343981: Remove usage of security manager from Thread and related classes [v2]
> Removes the SecurityManager usage from Thread + friends. > > In Thread, the getContextClassLoader method is no longer caller-sensitive > method. > > JavaLangAccess.newThreadWithAcc is removed and jdk.internal.access is no > longer exported to java.naming. The usage of newThreadWithAcc is removed from > com.sun.jndi.ldap.VersionHelper. There will be further work on the > java.naming module to remove usage of SM, the change here is specific to the > usage of ewThreadWithAcc. Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 225 commits: - Initial commit - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Merge branch 'master' into jep486 - Move remaining JEP 486 failing tests into correct groups. - Move JEP 486 failing tests into hotspot_runtime group. - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing - Merge branch 'master' into jep486 - Merge branch 'master' into jep486 - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 - Merge branch 'master' into jep486 - ... and 215 more: https://git.openjdk.org/jdk/compare/d0077eec...7bffe787 - Changes: https://git.openjdk.org/jdk/pull/22035/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22035&range=01 Stats: 69197 lines in 1892 files changed: 2475 ins; 62825 del; 3897 mod Patch: https://git.openjdk.org/jdk/pull/22035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22035/head:pull/22035 PR: https://git.openjdk.org/jdk/pull/22035
Re: RFR: 8344034: Remove security manager dependency in Serialization
On Tue, 12 Nov 2024 15:09:23 GMT, Roger Riggs wrote:
> After [JDK-8338411](https://bugs.openjdk.org/browse/JDK-8338411),
> Serialization implementation dependencies on SecurityManager, doPrivildged,
> and AccessController are removed.
> Some refactoring to cleanup the remaining code is expected.
src/java.base/share/classes/java/io/ObjectInputStream.java line 1551:
> 1549: * is "safe", FALSE otherwise.
> 1550: */
> 1551: @SuppressWarnings("removal")
Can the `@SuppressWarnings` be removed?
src/java.base/share/classes/java/io/ObjectInputStream.java line 2651:
> 2649:
> 2650: Callback(ObjectInputValidation obj, int priority, Callback
> next)
> 2651: {
I assume the "{" can move back to the previous line now.
src/java.base/share/classes/java/io/ObjectStreamClass.java line 279:
> 277: * @return the {@code Class} instance that this descriptor
> represents
> 278: */
> 279: @CallerSensitive
I assume this is no longer caller-sensitive.
src/java.base/share/classes/java/io/ObjectStreamClass.java line 918:
> 916: if (cons != null) {
> 917: try {
> 918: return cons.newInstance();
Do the ObjectStreamClass.newInstance still need suppress-warnings?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22041#discussion_r1838462339
PR Review Comment: https://git.openjdk.org/jdk/pull/22041#discussion_r1838463499
PR Review Comment: https://git.openjdk.org/jdk/pull/22041#discussion_r1838465737
PR Review Comment: https://git.openjdk.org/jdk/pull/22041#discussion_r1838468010
Re: RFR: 8343984: Fix Unsafe address overflow [v7]
On Tue, 12 Nov 2024 16:37:28 GMT, Shaojin Wen wrote: > @liach provided a new idea, which is to change the type of > jdk.internal.misc.Unsafe.ARRAY_XXX_OFFSET from int to long. I think @liach's > idea is better, that is the way to completely solve this problem. I discussed this briefly with him today, he's looking into the implications of doing that. - PR Comment: https://git.openjdk.org/jdk/pull/22027#issuecomment-2471038775
RFR: 8343981: Remove usage of security manager from Thread and related classes
Removes the SecurityManager usage from Thread + friends. In Thread, the getContextClassLoader method is no longer caller-sensitive method. JavaLangAccess.newThreadWithAcc is removed and jdk.internal.access is no longer exported to java.naming. The usage of newThreadWithAcc is removed from com.sun.jndi.ldap.VersionHelper. There will be further work on the java.naming module to remove usage of SM, the change here is specific to the usage of ewThreadWithAcc. - Depends on: https://git.openjdk.org/jdk/pull/21498 Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/22035/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22035&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8343981 Stats: 283 lines in 10 files changed: 0 ins; 228 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/22035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22035/head:pull/22035 PR: https://git.openjdk.org/jdk/pull/22035
Re: RFR: 8343984: Fix Unsafe address overflow [v5]
On Tue, 12 Nov 2024 12:10:07 GMT, Alan Bateman wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> rename address to offset, from @AlanBateman > > Marked as reviewed by alanb (Reviewer). > @AlanBateman @minborg Sorry, after you approved it, I found that there are > still offset overflows in ZipUtils and UnixUserDefinedFileAttributeView, so I > submitted the changes again. Please help review it again. Looks good, can you bump the copyright header on UnixUserDefinedFileAttributeView too? - PR Comment: https://git.openjdk.org/jdk/pull/22027#issuecomment-2470974166
Re: RFR: 8340133: Add concise usage message to the java executable [v7]
On Tue, 12 Nov 2024 13:18:34 GMT, Jan Lahoda wrote: >> The idea behind printing the short version is to identify what tool is >> speaking. It can be removed, if desired. > > `shortVersionString` changed to `conciseVersionString` in: > https://github.com/openjdk/jdk/pull/21411/commits/e9e9ad4bdd0a11244cb1ef84fdeea036e94718c7 > The idea behind printing the short version is to identify what tool is > speaking. It can be removed, if desired. It may be helpful to some but I'm consider it may be confusing in other cases, esp. because it may include "LTS" in the output. So I think maybe drop this for now, or get wider input on print the version string as the first line, it can always be added later if needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/21411#discussion_r1838309819
Re: RFR: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
On Tue, 12 Nov 2024 01:41:27 GMT, Roger Riggs wrote: > Refactor removing the dependencies on SecurityManager, doPrivileged, and > AccessController. src/java.base/share/classes/java/lang/ProcessBuilder.java line : > 1109: + (dir == null ? "" : " (in directory \"" + dir + "\")") > 1110: + e.getMessage(), > : e); Can you check the format of the exception? It looks like original put ": " before the exception message. With the change it looks like it will immediately follow without spacing. - PR Review Comment: https://git.openjdk.org/jdk/pull/22024#discussion_r1838239493
Re: RFR: 8343839: Detect patched modules and abort run-time image link early
On Tue, 12 Nov 2024 13:51:17 GMT, Severin Gehwolf wrote:
>> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 625:
>>
>>> 623: // Do not permit linking from run-time image when the
>>> current image
>>> 624: // is being patched
>>> 625: if
>>> (jdk.internal.misc.VM.getSavedProperties().get("jdk.module.patch.0") !=
>>> null) {
>>
>> I don't think we should introduce this dependency. Instead, I think we
>> should look at adding a property such as jdk.patched, with a boolean value,
>> to indicate if the current runtime is patched or not. This could be useful
>> for java -XshowSettings output too.
>
>> Instead, I think we should look at adding a property such as jdk.patched
>
> You mean a new standard property - observable with
> `System.getProperty("jdk.patched")` as well - which would also show up with
> `java -XshowSettings:properties`? Is that the direction you are envisioning?
Yes, I'm wondering if a JDK-specific (rather than standard) property could be
more widely useful as it would allow observability tools to know that the JDK
has been patched. It would be good to get wider input on this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22037#discussion_r1838203008
Re: RFR: 8340133: Add concise usage message to the java executable [v7]
On Mon, 11 Nov 2024 14:23:46 GMT, Alan Bateman wrote: >> Jan Lahoda 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 11 additional >> commits since the last revision: >> >> - Merge branch 'master' into JDK-8340133-2 >> - Using correct pplaceholders. >> - Adjusting text as suggested. >> - Cleaning up the concise message: >>- using 2 spaces instead of 4, >>- rewording the "for more use --help" part of the message as suggested to >> avoid the word "launcher". >> - Using lowercase for the keys in the help, using 'source-file' program >> instead of 'single-file' program. >> - Using an enum instead of booleans, as suggested. >> - Adjusting the concise help as suggested: 'using main class of a JAR >> archive' and '.jar'/'.java' >> - Adjusting the concise help based on review suggestions. >> - Cleanup. >> - Adjusting/improving the concise help. >> - ... and 1 more: https://git.openjdk.org/jdk/compare/d7e57952...b4d7b493 > > src/java.base/share/classes/sun/launcher/resources/launcher.properties line > 241: > >> 239: \ -jar .jar to execute the main class of a JAR >> archive\n\ >> 240: \ -m [/] to execute the main class of a module\n\ >> 241: \ .java to compile and execute a source-file >> program\n\n\ > > I'm not sure about the description of . It uses "compiled class", > maybe you meant "compiled main class" or something else to connect it > ""? > > "-jar .jar" may be confusing because the "java -help" uses "-java > ". I think the usages need to be the same. > @AlanBateman I'd like to respectfully disagree that this was an improvement. > If consistency was the important objection here, maybe it would have been > better to update "java -help" to include the .jar? I assume your disagreement is with Jan's latest update rather than my comment. My comment was just pointing out that it's confusing to use `-jar .jar` in one case and `-jar ` in the other. - PR Review Comment: https://git.openjdk.org/jdk/pull/21411#discussion_r1838159799
Re: RFR: 8343984: Fix Unsafe address overflow [v5]
On Tue, 12 Nov 2024 11:24:24 GMT, Shaojin Wen wrote: >> In the JDK code, there are some places that may cause Unsafe offset >> overflow. The probability of occurrence is low, but if it occurs, it will >> cause JVM crash. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > rename address to offset, from @AlanBateman Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/22027#pullrequestreview-2429423006
Re: RFR: 8343839: Detect patched modules and abort run-time image link early
On Tue, 12 Nov 2024 11:00:36 GMT, Severin Gehwolf wrote:
> Please review this fix to how patched modules are being handled when linking
> from the run-time image. During review of
> [JDK-8311302](https://bugs.openjdk.org/browse/JDK-8311302) it was pointed out
> that module patching should be detected earlier and the link should get
> aborted in that case.
>
> This patch implements it, by looking at the `jdk.module.patch.0` property
> like [other code in the
> JDK](https://github.com/openjdk/jdk/blob/2c1e4c381615ce52276f4bf331a1e7a845af4b6e/src/hotspot/share/cds/cdsConfig.cpp#L282)
> does. After this patch module patching is being detected before any archives
> are being read or the actual linking process starts (contrary to the previous
> solution).
>
> Testing:
> - [x] GHA testing (mac aarch64 test failures are infra related)
> - [x] Local testing of existing test, which covers it
>
> Thoughts?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 625:
> 623: // Do not permit linking from run-time image when the
> current image
> 624: // is being patched
> 625: if
> (jdk.internal.misc.VM.getSavedProperties().get("jdk.module.patch.0") != null)
> {
I don't think we should introduce this dependency. Instead, I think we should
look at adding a property such as jdk.patched, with a boolean value, to
indicate if the current runtime is patched or not. This could be useful for
java -XshowSettings output too.
src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 128:
> 126: \ --keep-packaged-modules is not supported
> 127: err.runtime.link.modified.file={0} has been modified
> 128: err.runtime.link.patched.module=The current runtime includes module
> patches.\
Not sure about "module patches", its more that the current runtime has been
patched with --patch-module.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22037#discussion_r1837935419
PR Review Comment: https://git.openjdk.org/jdk/pull/22037#discussion_r1837936172
Re: RFR: 8343984: Fix Unsafe address overflow [v3]
On Tue, 12 Nov 2024 08:59:30 GMT, Shaojin Wen wrote: >> In the JDK code, there are some places that may cause Unsafe offset >> overflow. The probability of occurrence is low, but if it occurs, it will >> cause JVM crash. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > from @minborg src/java.base/share/classes/java/lang/StringLatin1.java line 836: > 834: UNSAFE.putByte(val, address, (byte)(c1)); > 835: UNSAFE.putByte(val, address + 1, (byte)(c2)); > 836: UNSAFE.putByte(val, address + 2, (byte)(c3)); While you are here, I wonder if this should be renamed to offset to make it clearer. - PR Review Comment: https://git.openjdk.org/jdk/pull/22027#discussion_r1837720784
Re: RFR: 8340133: Add concise usage message to the java executable [v7]
On Mon, 11 Nov 2024 08:13:18 GMT, Jan Lahoda wrote: >> Currently, running `java` without any parameters will lead to an output that >> is a full `--help`, which is over 100 lines (on my computer at least), and >> it feels overwhelming. And many people might actually want to run >> JShell/REPL, not the `java` executable, but it is difficult find out about >> JShell. >> >> The proposal herein is to print a much shorter help, together with a pointer >> to JShell, when the executable does not know what to do. I.e. there is >> nothing specified to start, and no option like `--help` is specified. In >> particular, on my machine, it prints: >> >> openjdk 24-internal 2025-03-18 >> >> Usage: java [java options...] [application arguments...] >> >> Where is one of: >> to execute the main method of a compiled class >> -jar .jar to execute the main class of a JAR archive >> -m [/] to execute the main class of a module >> .java to compile and execute a source-file program >> >> Where key java options include: >> --class-path >> where is a list of directories and JAR archives to search >> for class files, separated by ":" >> --module-path >> where is a list of directories and JAR archives to search >> for modules, separated by ":" >> >> For additional help on usage: java --help >> For an interactive Java environment:jshell >> >> >> Hopefully, this may be easier both for people trying to run something, and >> for people that are really looking for JShell. >> >> What do you think? >> >> Thanks! > > Jan Lahoda 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 11 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8340133-2 > - Using correct pplaceholders. > - Adjusting text as suggested. > - Cleaning up the concise message: >- using 2 spaces instead of 4, >- rewording the "for more use --help" part of the message as suggested to > avoid the word "launcher". > - Using lowercase for the keys in the help, using 'source-file' program > instead of 'single-file' program. > - Using an enum instead of booleans, as suggested. > - Adjusting the concise help as suggested: 'using main class of a JAR > archive' and '.jar'/'.java' > - Adjusting the concise help based on review suggestions. > - Cleanup. > - Adjusting/improving the concise help. > - ... and 1 more: https://git.openjdk.org/jdk/compare/7f4880aa...b4d7b493 src/java.base/share/classes/sun/launcher/resources/launcher.properties line 241: > 239: \ -jar .jar to execute the main class of a JAR > archive\n\ > 240: \ -m [/] to execute the main class of a module\n\ > 241: \ .java to compile and execute a source-file > program\n\n\ I'm not sure about the description of . It uses "compiled class", maybe you means "compiled main class" or something else to connect it ? "-jar .jar" may be confusing because the "java -help" uses "-java ". I think the usages need to be the same. - PR Review Comment: https://git.openjdk.org/jdk/pull/21411#discussion_r1836748117
Re: RFR: 8343925: [BACKOUT] JDK-8342650 Move getChars to DecimalDigits
On Mon, 11 Nov 2024 12:34:44 GMT, Shaojin Wen wrote: > 8343925 Feedback PR #21593 test/jdk/java/util/BitSet/HugeToString.java crash, > > I can't reproduce the problem on a MacBook M1 Max, but I agree that more > testing is needed, so let's roll it back first. Thanks for the BACKOUT, looks right. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22012#pullrequestreview-2427435024
Re: RFR: 8340133: Add concise usage message to the java executable [v7]
On Mon, 11 Nov 2024 14:16:25 GMT, Alan Bateman wrote:
>> Jan Lahoda 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 11 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into JDK-8340133-2
>> - Using correct pplaceholders.
>> - Adjusting text as suggested.
>> - Cleaning up the concise message:
>>- using 2 spaces instead of 4,
>>- rewording the "for more use --help" part of the message as suggested to
>> avoid the word "launcher".
>> - Using lowercase for the keys in the help, using 'source-file' program
>> instead of 'single-file' program.
>> - Using an enum instead of booleans, as suggested.
>> - Adjusting the concise help as suggested: 'using main class of a JAR
>> archive' and '.jar'/'.java'
>> - Adjusting the concise help based on review suggestions.
>> - Cleanup.
>> - Adjusting/improving the concise help.
>> - ... and 1 more: https://git.openjdk.org/jdk/compare/7f4880aa...b4d7b493
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 598:
>
>> 596: static void printConciseUsageMessage(boolean printToStderr) {
>> 597: initOutput(printToStderr);
>> 598:
>> ostream.println(SharedSecrets.getJavaLangAccess().shortVersionString());
>
> What is the reason for printing the short version string at the start of the
> short usage message?
In passing, it may be better to pick "short" or "concise", right now it's a mix
in the method and resource keys.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21411#discussion_r1836751174
Re: RFR: 8340133: Add concise usage message to the java executable [v7]
On Mon, 11 Nov 2024 08:13:18 GMT, Jan Lahoda wrote:
>> Currently, running `java` without any parameters will lead to an output that
>> is a full `--help`, which is over 100 lines (on my computer at least), and
>> it feels overwhelming. And many people might actually want to run
>> JShell/REPL, not the `java` executable, but it is difficult find out about
>> JShell.
>>
>> The proposal herein is to print a much shorter help, together with a pointer
>> to JShell, when the executable does not know what to do. I.e. there is
>> nothing specified to start, and no option like `--help` is specified. In
>> particular, on my machine, it prints:
>>
>> openjdk 24-internal 2025-03-18
>>
>> Usage: java [java options...] [application arguments...]
>>
>> Where is one of:
>> to execute the main method of a compiled class
>> -jar .jar to execute the main class of a JAR archive
>> -m [/] to execute the main class of a module
>> .java to compile and execute a source-file program
>>
>> Where key java options include:
>> --class-path
>> where is a list of directories and JAR archives to search
>> for class files, separated by ":"
>> --module-path
>> where is a list of directories and JAR archives to search
>> for modules, separated by ":"
>>
>> For additional help on usage: java --help
>> For an interactive Java environment:jshell
>>
>>
>> Hopefully, this may be easier both for people trying to run something, and
>> for people that are really looking for JShell.
>>
>> What do you think?
>>
>> Thanks!
>
> Jan Lahoda 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 11 additional commits since
> the last revision:
>
> - Merge branch 'master' into JDK-8340133-2
> - Using correct pplaceholders.
> - Adjusting text as suggested.
> - Cleaning up the concise message:
>- using 2 spaces instead of 4,
>- rewording the "for more use --help" part of the message as suggested to
> avoid the word "launcher".
> - Using lowercase for the keys in the help, using 'source-file' program
> instead of 'single-file' program.
> - Using an enum instead of booleans, as suggested.
> - Adjusting the concise help as suggested: 'using main class of a JAR
> archive' and '.jar'/'.java'
> - Adjusting the concise help based on review suggestions.
> - Cleanup.
> - Adjusting/improving the concise help.
> - ... and 1 more: https://git.openjdk.org/jdk/compare/414bcbf6...b4d7b493
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 598:
> 596: static void printConciseUsageMessage(boolean printToStderr) {
> 597: initOutput(printToStderr);
> 598:
> ostream.println(SharedSecrets.getJavaLangAccess().shortVersionString());
What is the reason for printing the short version string at the start of the
short usage message?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21411#discussion_r1836738224
Re: RFR: 8343925: [BACKOUT] JDK-8342650 Move getChars to DecimalDigits
On Mon, 11 Nov 2024 12:34:44 GMT, Shaojin Wen wrote: > 8343925 Feedback PR #21593 test/jdk/java/util/BitSet/HugeToString.java crash, > > I can't reproduce the problem on a MacBook M1 Max, but I agree that more > testing is needed, so let's roll it back first. Changes in this area need to be very carefully reviewed and tested. I think continue with the current plan to blackout the original change and seeing wider review and testing for the REDO. Chen is testing the blackout now. - PR Comment: https://git.openjdk.org/jdk/pull/22012#issuecomment-2468230634
Re: RFR: 8342650: Move getChars to DecimalDigits [v4]
On Sun, 10 Nov 2024 08:58:18 GMT, Shaojin Wen wrote: >> Move getChars methods of StringLatin1 and StringUTF16 to DecimalDigits to >> reduce duplication >> >> 1. HexDigits and OctalDigits also include getCharsLatin1 and getCharsUTF16 >> 2. Putting these two methods into DecimalDigits can avoid the need to expose >> them in JavaLangAccess >> 3. Eliminate duplicate code in BigDecimal > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add benchmark I see Shaojin has created an issue to exclude the test but I think better to back this out quickly. - PR Comment: https://git.openjdk.org/jdk/pull/21593#issuecomment-2468030968
Re: RFR: 8331467: ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider
On Sun, 10 Nov 2024 14:48:26 GMT, jyxzwd wrote:
>> I got it.Thank you for the detailed explanation!Maybe we should consider
>> another way to load the custom FileSystemProvider.
>
> If we load the custom DefaultFileSystemProvider by SystemClassLoader,then we
> will step into the method
> SystemModuleFinders$SystemModuleReader#findImageLocation again, and the var
> imageReader will be null.But we can not define a custom classLoader to load
> the custom DefaultFileSystemProvider since it needs to maintain JDK 8 source
> compatibility.So the only way that I can think of is to use the
> installedProvider which has the 'file' scheme to directly get the 'modules'
> path value.
>
> In ImageReaderFactory:
>
> private static final Path BOOT_MODULES_JIMAGE = null;
>
> static {
> // check installed providers
> for (FileSystemProvider provider :
> FileSystemProvider.installedProviders()) {
> if ("file".equalsIgnoreCase(provider.getScheme())) {
> try {
> BOOT_MODULES_JIMAGE =
> provider.getFileSystem(URI.create("file:/")).getPath(JAVA_HOME, "lib",
> "modules");
> if (BOOT_MODULES_JIMAGE != null) break;
> } catch (UnsupportedOperationException uoe) {
> }
> }
> }
> }
>
> What do you think of this?I am new to openjdk source development.So I truely
> appreciate the time you dedecate to my question!
I assume this will lead to recursive initialisation, e.g.
`FileSystems.getFileSystem(URI.create("jrt:/"))` will use
FileSystemProvider.installedProviders to iterate over the installed providers.
I don't have time to look into this more but I think it will have to
reflectively get the FileSystem so that the code isn't compiled with references
to the built-in provider, as in something like this (not tested):
private static final Path BOOT_MODULES_JIMAGE;
static {
FileSystem fs;
if (ImageReaderFactory.class.getClassLoader() == null) {
// fs = DefaultFileSystemProvider.theFileSystem()
try {
fs = (FileSystem)
Class.forName("sun.nio.fs.DefaultFileSystemProvider")
.getMethod("theFileSystem")
.invoke(null);
} catch (Exception e) {
throw new ExceptionInInitializerError(e);
}
} else {
fs = FileSystems.getDefault();
}
BOOT_MODULES_JIMAGE = fs.getPath(JAVA_HOME, "lib", "modules");
}
Also just to say that we need to create a good test for this issue. I expect it
will be rarely to interpose on the default file system and use jrtfs at the
same time, but that is what this bug report is about so we'll need to create a
good test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21997#discussion_r1835741459
Re: JDK-8019345 & 8294241: Deprecate URL public constructors
On 10/11/2024 07:05, Peter Firmstone wrote: So we have an RFC3986 compliant URI implementation, that's not compatible with Java's RFC2396 URI and now we use deprecated URL public constructors... But I guess they're only deprecated, not going to be removed, we can't change our code to use RFC2396 URI without unintended consequences... Having said that, I would discourage developers from writing any new code that uses RFC2396 URI. My understanding is Java's RFC2396 cannot be changed for backward compatibility reasons, it's not strictly compliant with RFC2396 and varies from the spec with a larger allowed character set (unescaped)... Perhaps a new RFC3986 URI implementation could utilise a provider mechanism, with some static methods that allow the developer to select which RFC compliant URI version they want to use? We haven't used Java's URI class for a long time, as there are significant performance benefits to using RFC3986 et al, such as not relying on DNS lookup for identity. It's probably best to bring discussion about RFC 2396 vs. 3986 to net-dev as that is where both java.net.URI and legacy java.net.URL are maintained. Also make sure to clearly distinguish the two APIs in any discussion as it might confuse lurkers to bring up DNS lookup in the context of parsing input as a URI. As regards efforts to move to the newer RFC then I think you know the previous effort to upgrade to the newer RFC had to be backed out (before the release GA) due to compatibility issues. There have been efforts since then, including prototyping several API directions. I can't tell if you are asking about that or specifically the legacy URL constructors that have been deprecated for several releases, so maybe make that clearer in your mail to net-dev too. -Alan
Re: RFR: 8331467: ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider
On Sat, 9 Nov 2024 13:06:30 GMT, jyxzwd wrote:
>> src/java.base/share/classes/jdk/internal/jimage/ImageReaderFactory.java line
>> 51:
>>
>>> 49: private static final String JAVA_HOME =
>>> System.getProperty("java.home");
>>> 50: private static final Path BOOT_MODULES_JIMAGE =
>>> 51:
>>> sun.nio.fs.DefaultFileSystemProvider.create().getPath(JAVA_HOME, "lib",
>>> "modules");
>>
>> This is JDK internal so if the classes in jrt-fs.jar are loaded by a custom
>> class loader, in for example JDK 17 or 21, then this will fail.
>
> I was wondering, in JDK 17 or JDK 21, isn't classes in jrt-fs.jar already
> included under the java.base module? This would mean that the classes in
> jrt-fs.jar are actually already loaded by the bootstrapClassLoader, so a
> custom class loader wouldn’t typically need to load them again.
> Could you kindly advise if there are any scenarios in JDK 17 or JDK 21 where
> custom loading of jrt-fs.jar would still be necessary?
> Thank you very much for your guidance.
The jrt file system provider supports both the current JDK and a remote/target
JDK. When the JDK is not the current JDK then it loads the jrt file system
provider from target's JDK jrt-fs.jar. To understand this more, try this
example where you set targetJDK to the file path of another JDK on your system.
String targetJDK = .
var fs = FileSystems.newFileSystem(URI.create("jrt:/"), Map.of("java.home",
targetJDK));
byte[] classBytes =
Files.readAllBytes(fs.getPath("/modules/java.base/java/lang/String.class"));
Run with `-Xlog:class+load` and you'll see jrtfs and support jimage class files
loaded from the target JDK. If you dig deeper you'll see they are loaded by a
custom class loader, they are defined by the boot loader and aren't in
java.base. Hopefully this makes it clear why classes in jdk.internal.jimage or
jdk.internal.jrtfs can't reference JDK internal classes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21997#discussion_r1835482157
Re: RFR: 8331467: ImageReaderFactory can cause a ClassNotFoundException if the default FileSystemProvider is not the system-default provider
On Sat, 9 Nov 2024 02:18:22 GMT, jyxzwd wrote:
> Use the built-in file system provider rather than the custom file system
> provider.
> Add "public static FileSystemProvider create" method in
> DefaultFileSystemProvider which is from java8API to be compatible against
> runtime.
src/java.base/share/classes/jdk/internal/jimage/ImageReaderFactory.java line 51:
> 49: private static final String JAVA_HOME =
> System.getProperty("java.home");
> 50: private static final Path BOOT_MODULES_JIMAGE =
> 51:
> sun.nio.fs.DefaultFileSystemProvider.create().getPath(JAVA_HOME, "lib",
> "modules");
This is JDK internal so if the classes in jrt-fs.jar are loaded by a custom
class loader, in for example JDK 17 or 21, then this will fail.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21997#discussion_r1835320287
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v48]
On Fri, 8 Nov 2024 17:07:55 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright headers Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2424410814
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v47]
On Fri, 8 Nov 2024 10:42:15 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused imports from VMProps Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2424083294
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v47]
On Fri, 8 Nov 2024 10:42:15 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused imports from VMProps I'm worked through the make + src changes and don't have any issues. I skimmed tests some didn't study closely. You will need to bump the copyright header of several jlink src files before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2465008109
Re: RFR: 8342707: Prepare Gatherers for graduation from Preview [v4]
On Fri, 8 Nov 2024 13:56:38 GMT, Viktor Klang wrote: >> Make final adjustments to drop PreviewFeature and updating the @ since >> markers. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Updating the copyright year of the Gatherer benchmarks Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21686#pullrequestreview-2423918386
Re: RFR: 8342707: Prepare Gatherers for graduation from Preview [v2]
On Mon, 4 Nov 2024 16:10:47 GMT, Viktor Klang wrote: >> Make final adjustments to drop PreviewFeature and updating the @ since >> markers. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Updating copyright years and removing an unneccessary import for Gatherers Looks fine, you'll need to sync up the PR to resolve the merge conflict, and you'll need to update the copyright header of several files (esp. tests) before integrate. - PR Comment: https://git.openjdk.org/jdk/pull/21686#issuecomment-2464686608
Re: RFR: 8211033: Clean up the processing -classpath argument not to set LM_CLASS
On Fri, 8 Nov 2024 08:51:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.org/browse/JDK-8211033? > > As noted in that issue, this is a clean up of the code which determines the > "mode" through with the `java` application is being launched. In its current > form the presence of `--classpath` (or its equivalent arguments) was > unnecessary updating the mode to `LM_CLASS`. The commit in this PR removes > that part to allow for the mode to be detected based merely on the presence > (or absence) of `-m`, `-jar`, `--source` options. If neither is specified, > the file extension is checked to determine the launch mode. > > Given the nature of this clean up, no new tests have been introduced. > Existing tests in tier1, tier2, tier3 continue to pass with this change. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21971#pullrequestreview-2423527963
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Fri, 8 Nov 2024 10:34:43 GMT, Severin Gehwolf wrote: >> That's okay, I wasn't initially sure why they were changed. I'm looking at >> JRTArchiveFile.toEntry and wondering there should be a follow-up issue (not >> this PR) to fail early if running on a patched run-time even though it would >> be an odd configuration to attempt to do that. > > There already is. See: > https://github.com/openjdk/jdk/pull/14787/files#diff-b6b47eacb6060eb0a583a253f322f5d274063e082a12a72e8628a6e1ba6cdd3eR466-R471 > > It's also tested with > [PatchedJDKModuleJlinkTest.java](https://github.com/openjdk/jdk/pull/14787/files#diff-11b8a26307346fd7ca016a349243cabd3b982964aaf4335298e28e956b3968eb). > Do we need more? Yes, I know there is a test. It's "no such file" case in JRTArchiveFile.toEntry that I'm looking at. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834151588
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Fri, 8 Nov 2024 09:58:13 GMT, Severin Gehwolf wrote: >> test/langtools/tools/javac/plugin/AutostartPlugins.java line 33: >> >>> 31: * jdk.jlink >>> 32: * @build toolbox.ToolBox toolbox.JavacTask toolbox.JarTask >>> 33: * @run main/othervm AutostartPlugins >> >> Why do the two javac plugin tests changed to /othervm? I assume they fail >> with a JDK built with --generate-linkable-runtime but I can't immediately >> see why. > > jtreg patches the `java.base` module and those langtools tests *link* from > the run-time image (in such a config). Having a patched JDK module is not > supported (and I don't think we ever should). Running in a separate JVM > doesn't have this problem. See > https://github.com/openjdk/jdk/pull/14787/commits/d8e1e834508589725adb8d70acb862a1270678ca. > Does that make sense? That's okay, I wasn't initially sure why they were changed. I'm looking at JRTArchiveFile.toEntry and wondering there should be a follow-up issue (not this PR) to fail early if running on a patched run-time even though it would be an odd configuration to attempt to do that. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834095994
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Thu, 7 Nov 2024 13:27:41 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with six > additional commits since the last revision: > > - Mandy's feedback > - Remove "jlink.runtime.linkable" property from VMProps > >It's no longer used in any tests. > - Fix JLinkDedupTestBatchSizeOne.java > - Fix plugins/IncludeLocalesPluginTest.java > >Similar to GenerateJLIClassesPluginTest.java, the default module path >doesn't need to be explicitly specified. > - Fix plugins/GenerateJLIClassesPluginTest.java test > >The test doesn't need the default module path and default jmods >generated from the helper. Using this approach makes it work for >linkable run-time images as well. > - Simplify runtimeImage tests test/jtreg-ext/requires/VMProps.java line 28: > 26: import java.lang.module.ModuleFinder; > 27: import java.lang.module.ModuleReader; > 28: import java.lang.module.ModuleReference; I assume they aren't needed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834029491
RFR: 8342486: Implement JEP draft: Structured Concurrency (Fourth Preview)
Changes for [JEP draft: Structured Concurrency (Fourth Preview)](https://openjdk.org/jeps/8340343). The JEP isn't on the technical roadmap yet. The proposal is to re-preview the API with some changes, specifically: - A [StructuredTaskScope](https://download.java.net/java/early_access/loom/docs/api/java.base/java/util/concurrent/StructuredTaskScope.html) is now opened with a static factory method instead of a constructor. Once opened, the API usage is unchanged: fork subtasks individually, join them as a unit, process outcome, and close. - In conjunction with moving to using a static open method, policy and desired outcome is now selected by specifying a Joiner to the open method rather than extending STS. A Joiner handles subtask completion and produces the result for join to return. Joiner.onComplete is the equivalent of overriding handleComplete previously. This change means that the subclasses ShutdownOnFailure and ShutdownOnSuccess are removed, replaced by factory methods on Joiner to get an equivalent Joiner. - The join method is changed to return the result or throw STS.FailedException, replacing the need for an API in subclasses to obtain the outcome. This removes the hazard that was forgetting to call throwIfFailed to propagate exceptions. - Configuration that was provided with parameters for the constructor is changed so that can be provided by a configuration function. - joinUntil is replaced by allowing a timeout be configured by the configuration function. This allows the timeout to apply the scope rather than the join method. The underlying implementation is unchanged except that ThreadFlock.shutdown and wakeup methods are no longer confined. The STS API implementation moves to non-public StructuedTaskScopeImpl because STS is now an interface. A non-public Joiners class is added with the built-in Joiner implementations. - Commit messages: - Sync up from loom repo - Initial commit Changes: https://git.openjdk.org/jdk/pull/21934/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21934&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342486 Stats: 4114 lines in 15 files changed: 1888 ins; 1519 del; 707 mod Patch: https://git.openjdk.org/jdk/pull/21934.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21934/head:pull/21934 PR: https://git.openjdk.org/jdk/pull/21934
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v46]
On Thu, 7 Nov 2024 13:27:41 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with six > additional commits since the last revision: > > - Mandy's feedback > - Remove "jlink.runtime.linkable" property from VMProps > >It's no longer used in any tests. > - Fix JLinkDedupTestBatchSizeOne.java > - Fix plugins/IncludeLocalesPluginTest.java > >Similar to GenerateJLIClassesPluginTest.java, the default module path >doesn't need to be explicitly specified. > - Fix plugins/GenerateJLIClassesPluginTest.java test > >The test doesn't need the default module path and default jmods >generated from the helper. Using this approach makes it work for >linkable run-time images as well. > - Simplify runtimeImage tests test/langtools/tools/javac/plugin/AutostartPlugins.java line 33: > 31: * jdk.jlink > 32: * @build toolbox.ToolBox toolbox.JavacTask toolbox.JarTask > 33: * @run main/othervm AutostartPlugins Why do the two javac plugin tests changed to /othervm? I assume they fail with a JDK built with --generate-linkable-runtime but I can't immediately see why. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1834017867
Re: RFR: 8211033: Clean up the processing -classpath argument not to set LM_CLASS
On Fri, 8 Nov 2024 08:51:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.org/browse/JDK-8211033? > > As noted in that issue, this is a clean up of the code which determines the > "mode" through with the `java` application is being launched. In its current > form the presence of `--classpath` (or its equivalent arguments) was > unnecessary updating the mode to `LM_CLASS`. The commit in this PR removes > that part to allow for the mode to be detected based merely on the presence > (or absence) of `-m`, `-jar`, `--source` options. If neither is specified, > the file extension is checked to determine the launch mode. > > Given the nature of this clean up, no new tests have been introduced. > Existing tests in tier1, tier2, tier3 continue to pass with this change. On the surface this looks okay but just to double check: this has no impact on `java -cp -jar app.jar`. The class path in that case is `app.jar`, the class path specified to -cp is ignored. - PR Comment: https://git.openjdk.org/jdk/pull/21971#issuecomment-2464224551
Re: RFR: 8342486: Implement JEP draft: Structured Concurrency (Fourth Preview)
On Wed, 6 Nov 2024 15:47:55 GMT, Alan Bateman wrote:
> Changes for [JEP draft: Structured Concurrency (Fourth
> Preview)](https://openjdk.org/jeps/8340343). The JEP isn't on the technical
> roadmap yet. The proposal is to re-preview the API with some changes,
> specifically:
>
> - A
> [StructuredTaskScope](https://download.java.net/java/early_access/loom/docs/api/java.base/java/util/concurrent/StructuredTaskScope.html)
> is now opened with a static factory method instead of a constructor. Once
> opened, the API usage is unchanged: fork subtasks individually, join them as
> a unit, process outcome, and close.
> - In conjunction with moving to using a static open method, policy and
> desired outcome is now selected by specifying a Joiner to the open method
> rather than extending STS. A Joiner handles subtask completion and produces
> the result for join to return. Joiner.onComplete is the equivalent of
> overriding handleComplete previously. This change means that the subclasses
> ShutdownOnFailure and ShutdownOnSuccess are removed, replaced by factory
> methods on Joiner to get an equivalent Joiner.
> - The join method is changed to return the result or throw
> STS.FailedException, replacing the need for an API in subclasses to obtain
> the outcome. This removes the hazard that was forgetting to call
> throwIfFailed to propagate exceptions.
> - Configuration that was provided with parameters for the constructor is
> changed so that can be provided by a configuration function.
> - joinUntil is replaced by allowing a timeout be configured by the
> configuration function. This allows the timeout to apply the scope rather
> than the join method.
>
> The underlying implementation is unchanged except that ThreadFlock.shutdown
> and wakeup methods are no longer confined. The STS API implementation moves
> to non-public StructuedTaskScopeImpl because STS is now an interface. A
> non-public Joiners class is added with the built-in Joiner implementations.
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line
122:
> 120: * {@link #close() close} method always waits for threads executing
> subtasks to finish,
> 121: * even if the scope is cancelled, so execution cannot continue beyond
> the {@code close}
> 122: * method until the interrupted threads finish.
Viktor has suggested that we move this paragraph up or at least make it more
prominent.
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line
337:
> 335: * href="{@docRoot}/java.base/java/util/concurrent/package-summary.html#MemoryVisibility">
> 336: * happen-before any actions taken by that subtask, which in
> turn
> 337: * happen-before the subtask result is {@linkplain Subtask#get()
> retrieved}.
Chen has suggested that this be expanded to specify a HB between onComplete and
join. Viktor and I have discussed extending the existing text to deal with
subtask outcome that "contributes" to the scope result but did not agree
wording (note that this is a pre-existing short-coming in the spec, not a new
issue).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r1832464575
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r1832468226
Re: RFR: 8317542: Specjvm::xml have scalability issue for high vCPU numbers [v2]
On Thu, 7 Nov 2024 16:42:55 GMT, Vladimir Ivanov wrote: >> The synchronization block may be substituted by the 'volatile' variable >> smaller synchronization block. >> It reduce the total blocking time for the specjvm2008::xml.validation >> workload and improve the reported score. >> Scores for the 112vCPU on the with 28GB heap increased from 17915.83 to >> 22943.2. >> Unit tests was not affected: >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jaxp 516 516 0 0 >>jtreg:test/jdk/javax/xml 7070 0 0 >> == >> TEST SUCCESS >> >> The tier1 is OK too. > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8317542: Specjvm::xml have scalability issue for high vCPU numbers src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xpath/regex/RegularExpression.java line 2019: > 2017: int length; > 2018: Match match; > 2019: private Semaphore inuse = new Semaphore(1, true); Does it require fairness? The existing code using object monitors so I assume not. I assume "inuse" can be final. - PR Review Comment: https://git.openjdk.org/jdk/pull/21815#discussion_r1833031188
Re: RFR: 8311302: Implement JEP 493: Linking Run-Time Images without JMODs [v32]
On Fri, 14 Jun 2024 06:49:34 GMT, Alan Bateman wrote: >>> My aim will be to bring this into JDK 24 with a JEP then. Hopefully we can >>> bring this to a successful conclusion that way. >> >> @AlanBateman JEP draft is here: >> https://openjdk.org/jeps/8333799 >> >> Could you please help review it? Thanks! > >> Could you please help review it? Thanks! > > Yes, on my list. > Therefore, paging @AlanBateman @mlchung @mbreinhold @magicus for their final > thoughts. Thank you! ACK. I'll do another pass on the changes in the PR. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2461950790
Re: RFR: 8343770: Build fails due to use of sun.misc.Unsafe in LoopOverRandom
On Thu, 7 Nov 2024 15:12:01 GMT, Maurizio Cimadamore wrote: > This small PR fixes a build failure due to usage of sun.misc.Unsafe in a new > benchmark. > > FFM benchmarks appear to be broken -- because of JDK-8332744 -- but we will > fix that separately. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21958#pullrequestreview-2421275027
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v2]
On Wed, 6 Nov 2024 17:38:59 GMT, Patricio Chilano Mateo wrote: >> Good work! I'll approve the GC related changes. >> >> There are some simplifications I think can be done in the ObjectMonitor >> layer, but nothing that should go into this PR. >> >> Similarly, (even if some of this is preexisting issues) I think that the way >> we describe the frames and the different frame transitions should be >> overhauled and made easier to understand. There are so many unnamed >> constants and adjustments which are spread out everywhere, which makes it >> hard to get an overview of exactly what happens and what interactions are >> related to what. You and Dean did a good job at simplifying and adding >> comments in this PR. But I hope this can be improved in the fututre. >> >> A small note on `_cont_fastpath`, as it is now also used for synchronised >> native method calls (native wrapper) maybe the comment should be updated to >> reflect this. >> >> // the sp of the oldest known interpreted/call_stub frame inside the >> // continuation that we know about > >> A small note on `_cont_fastpath`, as it is now also used for synchronised >> native method calls (native wrapper) maybe the comment should be updated to >> reflect this. >> >> ``` >> // the sp of the oldest known interpreted/call_stub frame inside the >> // continuation that we know about >> ``` >> > Updated comment. > @pchilano `CancelTimerWithContention.java` test is failing on s390x with > Timeout Error. One weird thing is that it only fails when I run whole tier1 > test suite. But when ran independently test passes. > > One thing I would like to mention is that I ran test by **disabling** > VMContinuations, as Vthreads are not yet supported by s390x. We added this test to provoke contention on the delay queues, lots of timed-Object.wait with notification before the timeout is reached. This code is not used when running with -XX:+UnlockExperimentalVMOptions -XX:-VMContinuation. In that execution mode, each virtual thread is backed by a platform/native thread and in this test it will ramp up 10_000 virtual threads. The output in your log suggests it gets to ~4700 threads before the jtreg timeout kicks in. It might be that when you run the test on its own that there is enough resources for the test to pass, but not enough resources (just too slow) when competing with other tests. I think we can add `@requires vm.continuations` to this test. It's not useful to run with the alternative virtual thread implementation. - PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2461756432
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Thu, 24 Oct 2024 22:13:27 GMT, David Holmes wrote: >> We don't unmount the virtual thread here, we just temporarily change the >> thread identity. You could think of this method as >> switchIdentityToCarrierThread if that helps. > > Sorry to belabour this but why are we temporarily changing the thread > identity? What is the bigger operation that in underway here? We've had these temporary transitions from day 1. The changes in this PR remove one usage, they don't add any new usages. The intention is to make this nuisance go away. The last usage requires changes to the timer support, working on it. For now, it's easiest to think of it as a "java on java" issue where critical code is in Java rather than the VM. The timer issue arises when a virtual thread does a timed park needs to schedule and cancel a timer. This currently requires executing Java code that may contend on a timer or trigger a timer thread to start. This has implications for thread state, the park blocker, and the parking permit. Adding support for nested parking gets very messy, adds overhead, and is confusing for serviceability observers. The exiting behavior is to just temporarily switch the thread identity (as in Thread::currentThread) so it executes in the context of the carrier rather than the virtual thread. As I said, we are working to make this go away, it would have been nice to have removed in advance of the changes here. Update: The temporary transitions are now removed in the fibers branch (loom repo). This removes the switchToCarrierThread and switchToVirtualThread methods, and removes the need to introduce setCurrentLockId that is hard to explain in the discussions here. Need to decide if we should include it in this PR or try to do it before or after. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816425590
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Wed, 23 Oct 2024 06:11:26 GMT, David Holmes wrote: >> Sequence number, nouce, anything will work here as it's just to deal with >> the scenario where the timeout task for a previous wait may run concurrently >> with a subsequent wait. > > Suggestion: `timedWaitCounter` ? We could rename it to timedWaitSeqNo if needed. >> A virtual thread is mounted but doing a timed-park that requires temporarily >> switching to the identity of the carrier (identity = Therad.currentThread) >> when queuing the timer task. As mentioned in a reply to Axel, we are close >> to the point of removing this (nothing to do with object monitors of course, >> we've had the complexity with temporary transitions since JDK 19). >> >> More context here is that there isn't support yet for a carrier to own a >> monitor before a virtual thread is mounted, and same thing during these >> temporary transitions. If support for custom schedulers is exposed then that >> issue will need to be addressed as you don't want some entries on the lock >> stack owned by the carrier and the others by the mounted virtual thread. >> Patricio has mentioned inflating any held monitors before mount. There are a >> couple of efforts in this area going on now, all would need that issue fixed >> before anything is exposed. > > Okay but > 1. We have the current virtual thread > 2. We have the current carrier for that virtual thread (which is iotself a > java.alng.Thread object > 3. We have Thread.setCurrentLockId which ... ? which thread does it update? > And what does "current" refer to in the name? Thread identity switches to the carrier so Thread.currentThread() is the carrier thread and JavaThread._lock_id is the thread identifier of the carrier. setCurrentLockId changes JavaThread._lock_id back to the virtual thread's identifier. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1812537648 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810636960
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Thu, 24 Oct 2024 02:47:14 GMT, David Holmes wrote: >> Not really, it is the state we set when the virtual thread is mounted and >> runs again. In this case it will just run to re-contest for the monitor. > > So really UNBLOCKED is UNBLOCKING and mirrors BLOCKING , so we have: > > RUNNING -> BLOCKING -> BLOCKED > BLOCKED -> UNBLOCKING -> RUNNABLE > > I'm just trying to get a better sense of what we can infer if we see these > "transition" states. We named it UNBLOCKED when unblocked, like UNPARKED when unparked, as that accurately describes the state at this point. It's not mounted but may be scheduled to continue. In the user facing APIs this is mapped to "RUNNABLE", it's the equivalent of OS thread queued to the OS scheduler. So I think the name is good and would prefer not change it. >> When we unmount on a timed-wait call we schedule a wakeup task at the end of >> `afterYield`. There are two mechanisms that avoid the scheduled task to run >> and wake up the virtual thread on a future timed-wait call, since in this >> call the virtual thread could have been already notified before the >> scheduled task runs. The first one is to cancel the scheduled task once we >> return from the wait call (see `Object.wait(long timeoutMillis)`). Since the >> task could have been already started though, we also use `timedWaitSeqNo`, >> which the wake up task checks here to make sure it is not an old one. Since >> we synchronize on `timedWaitLock` to increment `timedWaitSeqNo` and change >> state to `TIMED_WAIT` before scheduling the wake up task in `afterYield`, >> here either a wrong `timedWaitSeqNo` or a state different than `TIMED_WAIT` >> means there is nothing to do. The only exception is checking for `SUSPENDED` >> state, in which case we just loop to retry. > > Thanks for the explanation but that needs to be documented somewhere. The comment in afterYield has been expanded in the loom repo, we may be able to bring that update in. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814517084 PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1818670426
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Wed, 6 Nov 2024 00:01:21 GMT, Patricio Chilano Mateo wrote: >>> Is this posted after the VirtualThreadMount extension event posted? >>> >> It's posted before. We post the mount event at the end of thaw only if we >> are able to acquire the monitor: >> https://github.com/openjdk/jdk/blob/124efa0a6b8d05909e10005f47f06357b2a73949/src/hotspot/share/runtime/continuationFreezeThaw.cpp#L1620 > >> The JvmtiExport::post_monitor_waited() is called at the line 1801. >> Does it post the MonitorWaited event for this virtual thread as well? >> > That's the path a virtual thread will take if pinned. This case is when we > were able to unmount the vthread. It is the equivalent, where the vthread > finished the wait part (notified, interrupted or timed-out case) and it's > going to retry acquiring the monitor. Just to add that the 2 extension events (VirtualThreadMount and VirtualThreadUnmount) are not part of any supported/documented interface. They are a left over from the exploration phase of virtual threads when we assumed the debugger agent would need to track the transitions. So at some point I think we need to figure out how to make them go away as they are an attractive nuisance (esp. if the event callback were to upcall and execute Java code). - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1830657204
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Wed, 23 Oct 2024 09:53:53 GMT, Richard Reingruber wrote: >> This is the implementation of JEP 491: Synchronize Virtual Threads without >> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for >> further details. >> >> In order to make the code review easier the changes have been split into the >> following initial 4 commits: >> >> - Changes to allow unmounting a virtual thread that is currently holding >> monitors. >> - Changes to allow unmounting a virtual thread blocked on synchronized >> trying to acquire the monitor. >> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` >> and its timed-wait variants. >> - Changes to tests, JFR pinned event, and other changes in the JDK libraries. >> >> The changes fix pinning issues for all 4 ports that currently implement >> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added >> recently and stand in its own commit after the initial ones. >> >> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default >> locking mode, (and `LM_MONITOR` which comes for free), but not when using >> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been >> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), >> with the intention to remove `LM_LEGACY` code in future releases. >> >> >> ## Summary of changes >> >> ### Unmount virtual thread while holding monitors >> >> As stated in the JEP, currently when a virtual thread enters a synchronized >> method or block, the JVM records the virtual thread's carrier platform >> thread as holding the monitor, not the virtual thread itself. This prevents >> the virtual thread from being unmounted from its carrier, as ownership >> information would otherwise go wrong. In order to fix this limitation we >> will do two things: >> >> - We copy the oops stored in the LockStack of the carrier to the stackChunk >> when freezing (and clear the LockStack). We copy the oops back to the >> LockStack of the next carrier when thawing for the first time (and clear >> them from the stackChunk). Note that we currently assume carriers don't hold >> monitors while mounting virtual threads. >> >> - For inflated monitors we now record the `java.lang.Thread.tid` of the >> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This >> allows us to tie the owner of the monitor to a `java.lang.Thread` instance, >> rather than to a JavaThread which is only created per platform thread. The >> tid is already a 64 bit field so we can ignore issues of the counter >> wrapping around. >> >> General notes about this part: >> >> - Since virtual th... > > src/hotspot/share/runtime/javaThread.hpp line 166: > >> 164: // current _vthread object, except during creation of the primordial >> and JNI >> 165: // attached thread cases where this field can have a temporary value. >> 166: int64_t _lock_id; > > Following the review I wanted to better understand when `_lock_id` changes. > There seems to be another exception to the rule that `_lock_id` is equal to > the `tid` of the current `_vthread`. I think they won't be equal when > switching temporarily from the virtual to the carrier thread in > `VirtualThread::switchToCarrierThread()`. Right, and we hope this temporary. We had more use of temporary transitions when the feature was initially added in JDK 19, now we mostly down to the nested parking issue. That will go away when we get to replacing the timer code, and we should be able to remove the switchXXX method and avoid the distraction/complexity that goes with them. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1812385061
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Tue, 22 Oct 2024 19:02:50 GMT, Patricio Chilano Mateo wrote: >> Just to say that we hope to eventually remove these "temporary transitions". >> This PR brings in a change that we've had in the loom repo to not need this >> when calling out to the scheduler. The only significant remaining use is >> timed-park. Once we address that then we will remove the need to switch the >> thread identity and remove some complexity, esp. for JVMTI and >> serviceability. >> >> In the mean-time, yes, the JavaThread.lock_id will temporarily switch to the >> carrier so a thread-dump/safepoint at just the right time looks like it >> print will be tid of the carrier rather than the mounted virtual thread. So >> we should fix that. (The original code in main line skipped this case so was >> lossy when taking a thread dump when hitting this case, David might remember >> the discussion on that issue). > > The problem is that within that window we don't have access to the virtual > thread's tid. The current thread has already been changed and we haven't yet > set the lock id back. Since this will be a rare corner case maybe we can just > print tid unavailable if we hit it. We could also add a boolean to > setCurrentThread to indicate we don't want to change the lock_id, but not > sure it's worth it. It should be rare and once we make further progress on timers then the use of temporary transitions will mostly disappear. I think the main thing for the thread dump is not to print a confusing "Carrying virtual thread" with the tid of the carrier. This came up in [pull/19482](https://github.com/openjdk/jdk/pull/19482) when the thread was extended. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1812377091
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Mon, 21 Oct 2024 15:41:45 GMT, Axel Boldt-Christmas
wrote:
>> This is the implementation of JEP 491: Synchronize Virtual Threads without
>> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for
>> further details.
>>
>> In order to make the code review easier the changes have been split into the
>> following initial 4 commits:
>>
>> - Changes to allow unmounting a virtual thread that is currently holding
>> monitors.
>> - Changes to allow unmounting a virtual thread blocked on synchronized
>> trying to acquire the monitor.
>> - Changes to allow unmounting a virtual thread blocked in `Object.wait()`
>> and its timed-wait variants.
>> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>>
>> The changes fix pinning issues for all 4 ports that currently implement
>> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added
>> recently and stand in its own commit after the initial ones.
>>
>> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default
>> locking mode, (and `LM_MONITOR` which comes for free), but not when using
>> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been
>> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)),
>> with the intention to remove `LM_LEGACY` code in future releases.
>>
>>
>> ## Summary of changes
>>
>> ### Unmount virtual thread while holding monitors
>>
>> As stated in the JEP, currently when a virtual thread enters a synchronized
>> method or block, the JVM records the virtual thread's carrier platform
>> thread as holding the monitor, not the virtual thread itself. This prevents
>> the virtual thread from being unmounted from its carrier, as ownership
>> information would otherwise go wrong. In order to fix this limitation we
>> will do two things:
>>
>> - We copy the oops stored in the LockStack of the carrier to the stackChunk
>> when freezing (and clear the LockStack). We copy the oops back to the
>> LockStack of the next carrier when thawing for the first time (and clear
>> them from the stackChunk). Note that we currently assume carriers don't hold
>> monitors while mounting virtual threads.
>>
>> - For inflated monitors we now record the `java.lang.Thread.tid` of the
>> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This
>> allows us to tie the owner of the monitor to a `java.lang.Thread` instance,
>> rather than to a JavaThread which is only created per platform thread. The
>> tid is already a 64 bit field so we can ignore issues of the counter
>> wrapping around.
>>
>> General notes about this part:
>>
>> - Since virtual th...
>
> src/hotspot/share/runtime/javaThread.cpp line 1545:
>
>> 1543: if (is_vthread_mounted()) {
>> 1544: // _lock_id is the thread ID of the mounted virtual thread
>> 1545: st->print_cr(" Carrying virtual thread #" INT64_FORMAT,
>> lock_id());
>
> What is the interaction here with `switchToCarrierThread` and the window
> between?
>
> carrier.setCurrentThread(carrier);
> Thread.setCurrentLockId(this.threadId());
>
> Will we print the carrier threads id as a virtual threads id? (I am guessing
> that is_vthread_mounted is true when switchToCarrierThread is called).
Just to say that we hope to eventually remove these "temporary transitions".
This PR brings in a change that we've had in the loom repo to not need this
when calling out to the scheduler. The only significant remaining use is
timed-park. Once we address that then we will remove the need to switch the
thread identity and remove some complexity, esp. for JVMTI and serviceability.
In the mean-time, yes, the JavaThread.lock_id will temporarily switch to the
carrier so a thread-dump/safepoint at just the right time looks like it print
will be tid of the carrier rather than the mounted virtual thread. So we should
fix that. (The original code in main line skipped this case so was lossy when
taking a thread dump when hitting this case, David might remember the
discussion on that issue).
> src/java.base/share/classes/jdk/internal/vm/Continuation.java line 62:
>
>> 60: NATIVE(2, "Native frame or on stack"),
>> 61: MONITOR(3, "Monitor held"),
>> 62: CRITICAL_SECTION(4, "In critical section");
>
> Is there a reason that the `reasonCode` values does not match the
> `freeze_result` reason values used in `pinnedReason(int reason)` to create
> one of these?
>
> I cannot see that it is used either. Only seem to be read for JFR
> VirtualThreadPinned Event which only uses the string.
That's a good question as they should match. Not noticed as it's not currently
used. As it happens, this has been reverted in the loom repo as part of
improving this code and fixing another issue.
Related is the freeze_result enum has new members, e.g. freeze_unsupported for
LM_LEGACY, that don't have a mapping to a Pinned, need to check if we could
t
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Wed, 23 Oct 2024 00:56:34 GMT, Coleen Phillimore wrote: >> This is the implementation of JEP 491: Synchronize Virtual Threads without >> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for >> further details. >> >> In order to make the code review easier the changes have been split into the >> following initial 4 commits: >> >> - Changes to allow unmounting a virtual thread that is currently holding >> monitors. >> - Changes to allow unmounting a virtual thread blocked on synchronized >> trying to acquire the monitor. >> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` >> and its timed-wait variants. >> - Changes to tests, JFR pinned event, and other changes in the JDK libraries. >> >> The changes fix pinning issues for all 4 ports that currently implement >> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added >> recently and stand in its own commit after the initial ones. >> >> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default >> locking mode, (and `LM_MONITOR` which comes for free), but not when using >> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been >> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), >> with the intention to remove `LM_LEGACY` code in future releases. >> >> >> ## Summary of changes >> >> ### Unmount virtual thread while holding monitors >> >> As stated in the JEP, currently when a virtual thread enters a synchronized >> method or block, the JVM records the virtual thread's carrier platform >> thread as holding the monitor, not the virtual thread itself. This prevents >> the virtual thread from being unmounted from its carrier, as ownership >> information would otherwise go wrong. In order to fix this limitation we >> will do two things: >> >> - We copy the oops stored in the LockStack of the carrier to the stackChunk >> when freezing (and clear the LockStack). We copy the oops back to the >> LockStack of the next carrier when thawing for the first time (and clear >> them from the stackChunk). Note that we currently assume carriers don't hold >> monitors while mounting virtual threads. >> >> - For inflated monitors we now record the `java.lang.Thread.tid` of the >> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This >> allows us to tie the owner of the monitor to a `java.lang.Thread` instance, >> rather than to a JavaThread which is only created per platform thread. The >> tid is already a 64 bit field so we can ignore issues of the counter >> wrapping around. >> >> General notes about this part: >> >> - Since virtual th... > > src/hotspot/share/runtime/javaThread.cpp line 2002: > >> 2000: #ifdef SUPPORT_MONITOR_COUNT >> 2001: >> 2002: #ifdef LOOM_MONITOR_SUPPORT > > If LOOM_MONITOR_SUPPORT is not true, this would skip this block and assert > for LIGHTWEIGHT locking. Do we need this #ifdef ? LOOM_MONITOR_SUPPORT was only needed when there were ports missing. All 4 are included now so this goes away. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1812389702
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Tue, 5 Nov 2024 07:48:40 GMT, Erik Gahlin wrote: >> This is the implementation of JEP 491: Synchronize Virtual Threads without >> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for >> further details. >> >> In order to make the code review easier the changes have been split into the >> following initial 4 commits: >> >> - Changes to allow unmounting a virtual thread that is currently holding >> monitors. >> - Changes to allow unmounting a virtual thread blocked on synchronized >> trying to acquire the monitor. >> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` >> and its timed-wait variants. >> - Changes to tests, JFR pinned event, and other changes in the JDK libraries. >> >> The changes fix pinning issues for all 4 ports that currently implement >> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added >> recently and stand in its own commit after the initial ones. >> >> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default >> locking mode, (and `LM_MONITOR` which comes for free), but not when using >> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been >> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), >> with the intention to remove `LM_LEGACY` code in future releases. >> >> >> ## Summary of changes >> >> ### Unmount virtual thread while holding monitors >> >> As stated in the JEP, currently when a virtual thread enters a synchronized >> method or block, the JVM records the virtual thread's carrier platform >> thread as holding the monitor, not the virtual thread itself. This prevents >> the virtual thread from being unmounted from its carrier, as ownership >> information would otherwise go wrong. In order to fix this limitation we >> will do two things: >> >> - We copy the oops stored in the LockStack of the carrier to the stackChunk >> when freezing (and clear the LockStack). We copy the oops back to the >> LockStack of the next carrier when thawing for the first time (and clear >> them from the stackChunk). Note that we currently assume carriers don't hold >> monitors while mounting virtual threads. >> >> - For inflated monitors we now record the `java.lang.Thread.tid` of the >> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This >> allows us to tie the owner of the monitor to a `java.lang.Thread` instance, >> rather than to a JavaThread which is only created per platform thread. The >> tid is already a 64 bit field so we can ignore issues of the counter >> wrapping around. >> >> General notes about this part: >> >> - Since virtual th... > > src/hotspot/share/jfr/metadata/metadata.xml line 160: > >> 158: >> 159: >> 160: > > Previously, the event was in the "Java Application" category. I think that > was a better fit because it meant it was visualized in the same lane in a > thread graph. See here for more information about the category: > > https://docs.oracle.com/en/java/javase/21/docs/api/jdk.jfr/jdk/jfr/Category.html > > (Note: The fact that the event is now written in the JVM doesn't determine > the category.) Thanks for spotting this, it wasn't intended to change the category. I think it's that Event element was copied from another event when adding it to metadata.xml and value from `@Category` wasn't carried over. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828915229
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Mon, 4 Nov 2024 02:12:40 GMT, David Holmes wrote:
>> This is the implementation of JEP 491: Synchronize Virtual Threads without
>> Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for
>> further details.
>>
>> In order to make the code review easier the changes have been split into the
>> following initial 4 commits:
>>
>> - Changes to allow unmounting a virtual thread that is currently holding
>> monitors.
>> - Changes to allow unmounting a virtual thread blocked on synchronized
>> trying to acquire the monitor.
>> - Changes to allow unmounting a virtual thread blocked in `Object.wait()`
>> and its timed-wait variants.
>> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
>>
>> The changes fix pinning issues for all 4 ports that currently implement
>> continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added
>> recently and stand in its own commit after the initial ones.
>>
>> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default
>> locking mode, (and `LM_MONITOR` which comes for free), but not when using
>> `LM_LEGACY` mode. Note that the `LockingMode` flag has already been
>> deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)),
>> with the intention to remove `LM_LEGACY` code in future releases.
>>
>>
>> ## Summary of changes
>>
>> ### Unmount virtual thread while holding monitors
>>
>> As stated in the JEP, currently when a virtual thread enters a synchronized
>> method or block, the JVM records the virtual thread's carrier platform
>> thread as holding the monitor, not the virtual thread itself. This prevents
>> the virtual thread from being unmounted from its carrier, as ownership
>> information would otherwise go wrong. In order to fix this limitation we
>> will do two things:
>>
>> - We copy the oops stored in the LockStack of the carrier to the stackChunk
>> when freezing (and clear the LockStack). We copy the oops back to the
>> LockStack of the next carrier when thawing for the first time (and clear
>> them from the stackChunk). Note that we currently assume carriers don't hold
>> monitors while mounting virtual threads.
>>
>> - For inflated monitors we now record the `java.lang.Thread.tid` of the
>> owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This
>> allows us to tie the owner of the monitor to a `java.lang.Thread` instance,
>> rather than to a JavaThread which is only created per platform thread. The
>> tid is already a 64 bit field so we can ignore issues of the counter
>> wrapping around.
>>
>> General notes about this part:
>>
>> - Since virtual th...
>
> src/hotspot/share/classfile/javaClasses.cpp line 2107:
>
>> 2105:
>> 2106: jlong java_lang_VirtualThread::waitTimeout(oop vthread) {
>> 2107: return vthread->long_field(_timeout_offset);
>
> Not sure what motivated the name change but it seems odd to have the method
> named differently to the field it accesses. ??
It was initially parkTimeout and waitTimeout but it doesn't require two fields
as you can't be waiting in Object.wait(timeout) and LockSupport.parkNanos at
the same time. So the field was renamed, the accessors here should probably be
renamed too.
> src/hotspot/share/prims/jvm.cpp line 4012:
>
>> 4010: }
>> 4011: ThreadBlockInVM tbivm(THREAD);
>> 4012: parkEvent->park();
>
> What code does the unpark to wake this thread up? I can't quite see how this
> unparker thread operates as its logic seems dispersed.
It's very similar to the "Reference Handler" thread. That thread calls into the
VM to get the pending-Reference list. Now we have "VirtualThread-unblocker"
calling into the VM to get the list of virtual threads to unblock.
ObjectMonitor::ExitEpilog will the unpark this thread when the virtual thread
successor is on the list to unblock.
> src/java.base/share/classes/java/lang/Thread.java line 655:
>
>> 653: * early startup to generate the identifier for the primordial
>> thread. The
>> 654: * counter is off-heap and shared with the VM to allow it assign
>> thread
>> 655: * identifiers to non-Java threads.
>
> Why do non-JavaThreads need an identifier of this kind?
JFR. We haven't changed anything there, just the initial tid.
> src/java.base/share/classes/java/lang/VirtualThread.java line 115:
>
>> 113: * RUNNING -> WAITING// transitional state during wait on
>> monitor
>> 114: * WAITING -> WAITED // waiting on monitor
>> 115: *WAITED -> BLOCKED// notified, waiting to be unblocked
>> by monitor owner
>
> Waiting to re-enter the monitor?
yes
> src/java.base/share/classes/java/lang/VirtualThread.java line 178:
>
>> 176: // timed-wait support
>> 177: private long waitTimeout;
>> 178: private byte timedWaitNonce;
>
> Strange name - what does this mean?
Sequence number, nouce, anything will work here as it's just to deal with the
scenario w
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Thu, 31 Oct 2024 03:52:31 GMT, Dean Long wrote:
> For some reason github thinks VirtualThreadPinnedEvent.java was renamed to
> libSynchronizedNative.c and libTracePinnedThreads.c was renamed to
> LockingMode.java. Is there a way to fix that?
I don't know which view this is but just to say that
VirtualThreadPinnedEvent.java and libTracePinnedThreads.c are removed.
libSynchronizedNative.c is part of a new test (as it happens, it was previously
reviewed as pull/18600 but we had to hold it back as it needed a fix from the
loom repo that is part of the JEP 491 implementation). You find is easier to
just fetch and checkout the branch to look at the changes locally. Personally I
have this easier for large change and makes it easier to see renames and/or
removals.
> src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 108:
>
>> 106: processDeregisterQueue();
>> 107:
>> 108: if (Thread.currentThread().isVirtual()) {
>
> It looks like we have two implementations, depending on if the current thread
> is virtual or not. The two implementations differ in the way they signal
> interrupted. Can we unify the two somehow?
When executed on a platform thread is will block in epoll_wait or kqueue so it
has to handle EINTR. It doesn't block in sys call when executed in a virtual
thread. So very different implementations.
> src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java line
> 57:
>
>> 55: static {
>> 56: try {
>> 57:
>> MethodHandles.lookup().ensureInitialized(AnchorCertificates.class);
>
> Why is this needed? A comment would help.
That's probably a good idea. It’s caused by pinning due to the
sun.security.util.AnchorCertificates’s class initializer, some of the http
client tests are running into this. Once monitors are out of the way then class
initializers, both executing, and waiting for, will be a priority.
> test/hotspot/gtest/nmt/test_vmatree.cpp line 34:
>
>> 32:
>> 33: using Tree = VMATree;
>> 34: using TNode = Tree::TreapNode;
>
> Why is this needed?
We had to rename the alias to avoid a conflict with the Node in compile.hpp.
Just lucky not to run into this in main-line. It comes and goes, depends on
changes to header files that are transitively included by the test. I think
Johan had planned to change this in main line but it may have got forgotten.
> test/hotspot/jtreg/compiler/codecache/stress/OverloadCompileQueueTest.java
> line 42:
>
>> 40: *
>> -XX:CompileCommand=exclude,java.lang.Thread::beforeSleep
>> 41: *
>> -XX:CompileCommand=exclude,java.lang.Thread::afterSleep
>> 42: *
>> -XX:CompileCommand=exclude,java.util.concurrent.TimeUnit::toNanos
>
> I'm guessing these changes have something to do with JDK-8279653?
It should have been added when Thread.sleep was changed but we got lucky.
> test/hotspot/jtreg/serviceability/jvmti/events/MonitorContendedEnter/mcontenter01/libmcontenter01.cpp
> line 73:
>
>> 71: /*
>> == */
>> 72:
>> 73: static int prepare(JNIEnv* jni) {
>
> Is this a bug fix?
Testing ran into a couple of bugs in JVMTI tests. One of was tests that was
stashing the JNIEnv into a static.
> test/jdk/java/lang/reflect/callerCache/ReflectionCallerCacheTest.java line 30:
>
>> 28: * by reflection API
>> 29: * @library /test/lib/
>> 30: * @requires vm.compMode != "Xcomp"
>
> If there is a problem with this test running with -Xcomp and virtual threads,
> maybe it should be handled as a separate bug fix.
JBS has several issues related to ReflectionCallerCacheTest.java and -Xcomp,
going back several releases. It seems some nmethod is keeping objects alive and
is preventing class unloading in this test. The refactoring of j.l.ref in JDK
19 to workaround pinning issues made it go away. There is some minimal revert
in this PR to deal with the potential for preemption when polling a reference
queue and it seems the changes to this Java code have brought back the issue.
So it's excluded from -Xcomp again. Maybe it would be better to add it to
ProblemList-Xcomp.txt instead? That would allow it to link to one of the JSB
issue on this issue.
-
PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2449153774
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825115214
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825127591
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825121520
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825112326
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825110254
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1817692430
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
On Tue, 22 Oct 2024 15:23:50 GMT, Andrew Haley wrote: > This last sentence has interesting consequences for user-defined schedulers. > Would it make sense to throw an exception if a carrier thread is holding a > monitor while mounting a virtual thread? Doing that would also have the > advantage of making some kinds of deadlock impossible. There's nothing exposed today to allow custom schedulers. The experiments/explorations going on right now have to be careful to not hold any locks. Throwing if holding a monitor is an option but only it would need to be backed by spec and would also shine light on the issue of j.u.concurrent locks as a carrier might independently hold a lock there too. - PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2431600434
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v30]
On Wed, 6 Nov 2024 00:01:21 GMT, Patricio Chilano Mateo wrote: >>> Is this posted after the VirtualThreadMount extension event posted? >>> >> It's posted before. We post the mount event at the end of thaw only if we >> are able to acquire the monitor: >> https://github.com/openjdk/jdk/blob/124efa0a6b8d05909e10005f47f06357b2a73949/src/hotspot/share/runtime/continuationFreezeThaw.cpp#L1620 > >> The JvmtiExport::post_monitor_waited() is called at the line 1801. >> Does it post the MonitorWaited event for this virtual thread as well? >> > That's the path a virtual thread will take if pinned. This case is when we > were able to unmount the vthread. It is the equivalent, where the vthread > finished the wait part (notified, interrupted or timed-out case) and it's > going to retry acquiring the monitor. Just to add that the 2 extension events (VirtualThreadMount and VirtualThreadUnmount) are not part of any supported/documented interface. They are a left over from the exploration phase of virtual threads when we assumed the debugger agent would need to track the transitions. So at some point I think we need to figure out how to make them go away as they are an attractive nuisance (esp. if the event callback were to upcall and execute Java code). - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1830657204
Re: RFR: 8310996: Add JFR event for connect operations [v4]
On Wed, 6 Nov 2024 00:15:44 GMT, Tim Prinzing wrote: >> Adds a JFR event for socket connect operations. >> >> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also >> check for connect events. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > suggested changes The update in ce9d39e2 has the changes that I discussed with Tim yesterday. Specifically, it fixes several issues with the non-blocking case, only records an event if the connect method actually attempts to establish a connection, and fixes the socket adaptor. So I think this to NioSocketImpl and SocketChannelImpl are good now. - PR Comment: https://git.openjdk.org/jdk/pull/21528#issuecomment-2458949631
Re: RFR: 8310996: Add JFR event for connect operations [v4]
On Wed, 6 Nov 2024 00:15:44 GMT, Tim Prinzing wrote:
>> Adds a JFR event for socket connect operations.
>>
>> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also
>> check for connect events.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> suggested changes
src/jdk.jfr/share/classes/jdk/jfr/internal/JDKEvents.java line 2:
> 1: /*
> 2: * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights
> reserved.
I assume you didn't mean to change that.
src/jdk.jfr/share/conf/jfr/profile.jfc line 741:
> 739: true
> 740: 10
> ms
> 741:
In default.jfr the threshold for the socket events is 20ms, but 10ms in
profile.jfc. Is that intentional?
test/jdk/jdk/jfr/event/io/TestSocketChannelEvents.java line 106:
> 104:
> 105: try (SocketChannel sc =
> SocketChannel.open(ssc.getLocalAddress())) {
> 106:
> addExpectedEvent(IOEvent.createSocketConnectEvent(sc.socket()));
This is SocketChannel in blocking mode where the connect succeeds. There is
also the non-blocking and where connect fails. In addition the connection can
established with the socket adaptor. So 6 possible cases for SocketChannel if
the test is expanded.
test/jdk/jdk/jfr/event/io/TestSocketEvents.java line 108:
> 106: try (Socket s = new Socket()) {
> 107: s.connect(ss.getLocalSocketAddress());
> 108:
> addExpectedEvent(IOEvent.createSocketConnectEvent(s));
This is Socket.connect success case, I assume we'll need a test for fail case
too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1830512546
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1830516492
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1830545551
PR Review Comment: https://git.openjdk.org/jdk/pull/21528#discussion_r1830537634
Re: RFR: 8343064: ClassFormatError: Illegal class name from InnerClassLambdaMetafactory
On Tue, 5 Nov 2024 18:29:52 GMT, Chen Liang wrote: > After the ClassFile API migration, when serializable lambdas are requested > for hidden class callers, illegal class name is generated for the > serialization methods, which previously had legal yet unusable class names, > as hidden classes cannot be described by a `CONSTANT_Class_info`. > > This patch restores a similar older behavior of using legal yet unusable > class names. Previously, the invalid `.` in hidden class names was replaced > by a `/` as if a package separator; this patch changes it to a `_` like that > in the generated lambda's class name. > > The bug report shows some unintended and erroneous usages of > `LambdaMetafactory`. To clarify and to persuade against misuse, I added a > paragraph of API notes to `LambdaMetafactory`, describing the impact of this > API being designed to support Java language features. In particular, I used > the scenario where people assumed LMf generates weak hidden classes, which > happened in this issue report and in #12493, that misuse can lead to resource > leaks. Is it possible to add a test for this? - PR Comment: https://git.openjdk.org/jdk/pull/21912#issuecomment-2457907971
Re: RFR: 8310996: Add JFR event for connect operations [v3]
On Tue, 5 Nov 2024 16:48:14 GMT, Tim Prinzing wrote: >> Adds a JFR event for socket connect operations. >> >> Existing tests TestSocketEvents and TestSocketChannelEvents modified to also >> check for connect events. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > improved exception names and handling Discussed with Tim as there are a number of issues that will need attention. A SocketConnectEvent should only be "offered" for connect events, not cases such as "already connected" or "socket closed". For SocketChannel there is also a socket adaptor (blockingConnect method) that will need to be updated. The non-blocking connect/finishConnect is complicated and there are several issues there. Finally, remoteAddress requires stateLock. - PR Comment: https://git.openjdk.org/jdk/pull/21528#issuecomment-2457759513
RFR: 8342380: Implement JEP draft: Warn about use of Memory-Access Methods in `sun.misc.Unsafe`
Change default value of the command line option --sun-misc-unsafe-memory-access from "allow" to "warn". - Commit messages: - Update test - Merge branch 'master' into JDK-8342380 - Update usage/man page - Initial commit Changes: https://git.openjdk.org/jdk/pull/21762/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21762&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342380 Stats: 29 lines in 4 files changed: 10 ins; 5 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/21762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21762/head:pull/21762 PR: https://git.openjdk.org/jdk/pull/21762
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v30]
On Tue, 5 Nov 2024 07:48:40 GMT, Erik Gahlin wrote: >> Patricio Chilano Mateo has updated the pull request incrementally with five >> additional commits since the last revision: >> >> - Add oopDesc::has_klass_gap() check >> - Rename waitTimeout/set_waitTimeout accessors >> - Revert suggestion to ThawBase::new_stack_frame >> - Improve JFR pinned reason in event >> - Use freeze_result consistently > > src/hotspot/share/jfr/metadata/metadata.xml line 160: > >> 158: >> 159: >> 160: > > Previously, the event was in the "Java Application" category. I think that > was a better fit because it meant it was visualized in the same lane in a > thread graph. See here for more information about the category: > > https://docs.oracle.com/en/java/javase/21/docs/api/jdk.jfr/jdk/jfr/Category.html > > (Note: The fact that the event is now written in the JVM doesn't determine > the category.) Thanks for spotting this, it wasn't intended to change the category. I think it's that Event element was copied from another event when adding it to metadata.xml and value that was in the `@Catalog` wasn't carried over. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1828915229
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v17]
On Mon, 4 Nov 2024 09:21:52 GMT, Thomas Stuefe wrote: > Can we get rid of `JNICALL` too, please? > > Or would that change be too big? There's >1000 in java.base, lots more elsewhere, so it would be a lot of files and would hide the core changes. So maybe for a follow-up PR that does the one thing. - PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2454201348
Re: RFR: 8317542: Specjvm::xml have scalability issue for high vCPU numbers
On Thu, 31 Oct 2024 21:33:11 GMT, Vladimir Ivanov wrote: > The synchronization block may be substituted by the 'volatile' variable > smaller synchronization block. > It reduce the total blocking time for the specjvm2008::xml.validation > workload and improve the reported score. > Scores for the 112vCPU on the with 28GB heap increased from 17915.83 to > 22943.2. > Unit tests was not affected: > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jaxp 516 516 0 0 >jtreg:test/jdk/javax/xml 7070 0 0 > == > TEST SUCCESS > > The tier1 is OK too. Could this code be converted to use a j.u.c.Semaphore with a single permit? It has the tryAcquire/release that appears to be needed here. - PR Comment: https://git.openjdk.org/jdk/pull/21815#issuecomment-2454046752
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v28]
On Mon, 4 Nov 2024 07:21:19 GMT, Axel Boldt-Christmas wrote: >> Patricio Chilano Mateo has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use lazySubmitRunContinuation when blocking > > src/java.base/share/classes/jdk/internal/vm/Continuation.java line 62: > >> 60: NATIVE(2, "Native frame or on stack"), >> 61: MONITOR(3, "Monitor held"), >> 62: CRITICAL_SECTION(4, "In critical section"); > > Is there a reason that the `reasonCode` values does not match the > `freeze_result` reason values used in `pinnedReason(int reason)` to create > one of these? > > I cannot see that it is used either. Only seem to be read for JFR > VirtualThreadPinned Event which only uses the string. That's a good question as they should match. Not noticed as it's not currently used. As it happens, this has been reverted in the loom repo as part of improving this code and fixing another issue. Related is the freeze_result enum has new members, e.g. freeze_unsupported for LM_LEGACY, that don't have a mapping to a Pinned, need to check if we could trip over that. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827316145
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v28]
On Mon, 4 Nov 2024 02:12:40 GMT, David Holmes wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use lazySubmitRunContinuation when blocking
>
> src/hotspot/share/classfile/javaClasses.cpp line 2107:
>
>> 2105:
>> 2106: jlong java_lang_VirtualThread::waitTimeout(oop vthread) {
>> 2107: return vthread->long_field(_timeout_offset);
>
> Not sure what motivated the name change but it seems odd to have the method
> named differently to the field it accesses. ??
It was initially parkTimeout and waitTimeout but it doesn't require two fields
as you can't be waiting in Object.wait(timeout) and LockSupport.parkNanos at
the same time. So the field was renamed, the accessors here should probably be
renamed too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827219720
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v6]
On Sun, 3 Nov 2024 11:25:13 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/java/lang/System.java line 1364:
>>
>>> 1362: *
>>> 1363: * It is the responsibility of the provider of
>>> 1364: * the concrete {@code LoggerFinder} implementation to ensure that
>>
>> This is still a part of the paragraph related to the security manager.
>
> Right - this paragraph - lines 1620-1625 (old file) / 1362-1367 (new file) is
> no longer relevant and should be removed too. Thanks for spotting that.
Removed in jep486 branch in sandbox so will get picked up when PR is refreshed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1826972198
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v6]
On Sat, 2 Nov 2024 22:18:09 GMT, ExE Boss wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 200 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Modify three RMI tests to work without the security manager: >> - test/jdk/java/rmi/registry/classPathCodebase/ClassPathCodebase.java >> - test/jdk/java/rmi/registry/readTest/CodebaseTest.java >> - >> test/jdk/java/rmi/server/RMIClassLoader/useCodebaseOnly/UseCodebaseOnly.java >>Also remove them from the problem list. >> - Remove two obsolete RMI tests: >> - test/jdk/java/rmi/server/RMIClassLoader/spi/ContextInsulation.java >> - >> test/jdk/sun/rmi/transport/tcp/disableMultiplexing/DisableMultiplexing.java >>Adjust two tests to run without the Security Manager: >> - >> test/jdk/java/rmi/server/RMIClassLoader/loadProxyClasses/LoadProxyClasses.java >> - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java >>Remove all of these tests from the problem list. >> - In staticPermissionsOnly(), change "current policy binding" to "current >> policy" so wording is consistent with the API note that follows. >> - Added API Notes to ProtectionDomain clarifying that the current policy >> always >>grants no permissions. A few other small changes to Policy and PD. >> - Merge branch 'master' into jep486 >> - JAXP tests: organize imports of a few tests >> - Improve description of Executors.privilegedThreadFactory >> - rename TestAppletLoggerContext.java as suggested in util test review >> - clientlibs: Javadoc cleanup >> - ... and 190 more: https://git.openjdk.org/jdk/compare/158ae51b...7958ee2b > > src/java.base/share/classes/java/lang/System.java line 2338: > >> 2336: * Invoked by VM. Phase 3 is the final system initialization: >> 2337: * 1. eagerly initialize bootstrap method factories that might >> interact >> 2338: *negatively with custom security managers and custom class >> loaders > > They might still interact negatively with custom class loaders though. The context here is custom a security manager doing string concatenation, this has required StringConcatFactory be eagerly initialized or security managers set on the command line to have been compiled with -XDstringConcat=inline. I think your question is about overriding the system class loader and whether its initialisation can use string concatenation reliably. That's a good thing to add a test for. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1826916876
Re: RFR: 8335991: Implement Simple Source Files and Instance Main Methods (Fourth Preview)
On Sat, 2 Nov 2024 10:41:23 GMT, Jaikiran Pai wrote: >> This is a partial implementation of JEP 495 - adjustments of the JEP >> metadata in `PreviewFeature`. There are no language changes associated with >> this JEP. Changes to the `java.io.IO` class are covered by >> https://github.com/openjdk/jdk/pull/21693. > > src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 72: > >> 70: //--- >> 71: @JEP(number=495, title="Simple Source Files and Instance Main >> Methods", status="Fourth Preview") >> 72: IMPLICIT_CLASSES, > > Hello Jan, although it's just an internal enum name, do you think we should > rename it to be closer to the JEP's title or is it not worth it? The title also shows up in list of preview APIs in the javadoc so changing seems correct. - PR Review Comment: https://git.openjdk.org/jdk/pull/21787#discussion_r1826585785
Re: RFR: 8341903: Implementation of Scoped Values (Fourth Preview) [v7]
On Fri, 1 Nov 2024 16:23:03 GMT, Andrew Haley wrote: >> The fourth preview of scoped values. > > Andrew Haley 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 10 additional > commits since the last revision: > > - Merge https://github.com/openjdk/jdk into JDK-8341903 > - Typo in comment. (Thanks, Roland!) > - API change > - Merge branch 'clean' into JDK-8341903 > - Scoped values > - Scoped values > - Fix javadoc > - Scoped Values API changes > - Scoped Values API changes > - Scoped Values API changes Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21456#pullrequestreview-2410408387
Re: RFR: 8343250: ArrayBlockingQueue serialization not thread safe
On Wed, 30 Oct 2024 08:54:55 GMT, kabutz wrote: > The ArrayBlockingQueue has had a readObject() method since Java 7, which > checks invariants of the deserialized object. However, it does not have a > writeObject() method. This means that the ArrayBlockingQueue could be > modified whilst it is being written, resulting in broken invariants. The > readObject() method's invariant checking is not exhaustive, which means that > it is possible to end up with ArrayBlockingQueue instances that contain null > values, leading to a difference between "size()" and how many objects would > be returned with "poll()". > > The ABQ should get a writeObject() method that is locking on the same locks > as the rest of the class. A good find. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21783#pullrequestreview-2410378934
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v22]
On Thu, 31 Oct 2024 20:12:06 GMT, Dean Long wrote: >> Patricio Chilano Mateo has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix typos in comments > > test/hotspot/gtest/nmt/test_vmatree.cpp line 34: > >> 32: >> 33: using Tree = VMATree; >> 34: using TNode = Tree::TreapNode; > > Why is this needed? We had to rename the alias to avoid a compiling with the Node in compile.hpp. Just lucky not to run into this in main-line. I think Johan had planned to change this in main line but it may have got forgotten. - PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825121520
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v22]
On Thu, 31 Oct 2024 19:59:00 GMT, Dean Long wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix typos in comments
>
> src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java line
> 57:
>
>> 55: static {
>> 56: try {
>> 57:
>> MethodHandles.lookup().ensureInitialized(AnchorCertificates.class);
>
> Why is this needed? A comment would help.
That's probably a good idea. It’s caused by pinning due to the
sun.security.util.AnchorCertificates’s class initializer, some of the http
client tests are running into this. Once monitors are out of the way then class
initializers, both executing, and waiting for, will be a priority.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825127591
Re: RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v22]
On Thu, 31 Oct 2024 20:13:31 GMT, Dean Long wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix typos in comments
>
> src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 108:
>
>> 106: processDeregisterQueue();
>> 107:
>> 108: if (Thread.currentThread().isVirtual()) {
>
> It looks like we have two implementations, depending on if the current thread
> is virtual or not. The two implementations differ in the way they signal
> interrupted. Can we unify the two somehow?
When executed on a platform thread is will block in epoll_wait or kqueue so it
has to handle EINTR. It doesn't block in sys call when executed in a virtual
thread. So very different implementations.
> test/hotspot/jtreg/compiler/codecache/stress/OverloadCompileQueueTest.java
> line 42:
>
>> 40: *
>> -XX:CompileCommand=exclude,java.lang.Thread::beforeSleep
>> 41: *
>> -XX:CompileCommand=exclude,java.lang.Thread::afterSleep
>> 42: *
>> -XX:CompileCommand=exclude,java.util.concurrent.TimeUnit::toNanos
>
> I'm guessing these changes have something to do with JDK-8279653?
It should have been added when Thread.sleep was changed but we got lucky.
> test/hotspot/jtreg/serviceability/jvmti/events/MonitorContendedEnter/mcontenter01/libmcontenter01.cpp
> line 73:
>
>> 71: /*
>> == */
>> 72:
>> 73: static int prepare(JNIEnv* jni) {
>
> Is this a bug fix?
Testing ran into a couple of bugs in JVMTI tests. One of was tests that was
stashing the JNIEnv into a static.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825115214
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825112326
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825110254
Re: RFR: 8335989: Implement Module Import Declarations (Second Preview) [v5]
On Wed, 30 Oct 2024 15:25:24 GMT, Jan Lahoda wrote: >> This is a current patch for module imports declarations, second preview. At >> least the JEP number and preview revision will need to be updated in >> `jdk.internal.javac.PreviewFeature.Feature`, but otherwise I believe this is >> ready to receive feedback. >> >> The main changes are: >> - `requires transitive java.base;` is permitted, under the preview flag. >> Both javac and the runtime module system are updated to accept this >> directive when preview is enabled. >> - the `java.se` module is using `requires transitive java.base;`, and is >> deemed to be participating in preview, so its classfile version is not >> tainted. Runtime is updated to access `requires transitive java.base;` in >> any `java.*`, considering all of them to be participating in preview. Can be >> tighten up to only `java.se` is desired. >> - the types imported through module imports can be shadowed using on-demand >> imports. So, for example, having: >> >> import module java.base; >> import module java.desktop; >> ... >> List l;//ambigous >> >> but: >> >> import module java.base; >> import module java.desktop; >> import java.util.*; >> ... >> List l;//not ambigous, reference to java.util.List > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Moving operators to the beginning of line, as suggested. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21431#pullrequestreview-2407660920
