Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Wed, 5 Jun 2024 17:48:25 GMT, Naoto Sato wrote: >> This test intends to verify the behavior of JdkConsole for the java.base >> module, wrt restoring the echo. The test assumes the internal methods that >> sets/gets the echo status of the platform. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unnecessary add-opens Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19315#pullrequestreview-2100486648
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
On Wed, 5 Jun 2024 17:31:44 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: > > - Move JImageHelper > - Update wording on multi-hop. > - Remove printStackTrace() > - Fix comment. Stream.toList() > - Use pattern matching for instanceof in JRTArchive::equals > - Rename JLINK_PRODUCE_RUNTIME_LINK_JDK to JLINK_PRODUCE_LINKABLE_RUNTIME Build changes look good. Note that having a dynamic value for a DEFAULT makes it render the help text a bit weirdly. You may want to consider spelling out how the default changes in the help text. We have some of these issues with other configure options already though so no big deal I think. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2100313329
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed.
On Fri, 24 Feb 2023 22:15:18 GMT, Leonid Mesnik wrote: > The solution proposed by Stefan K > > The startProcess() might wait forever for the expected line if the process > exits (failed to start). It makes sense to just fail earlier in such cases. > > The fix also move > 'output = new OutputAnalyzer(this.process);' > in method xrun() to be able to try to print them in waitFor is > failed/interrupted. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/12751#issuecomment-1444608371
Re: RFR: 8319457: Update jpackage to support WiX v4 and v5 on Windows
On Tue, 4 Jun 2024 12:37:03 GMT, Alexey Semenyuk wrote: > > I assume WiX5 will just work if installed instead of WiX4? > > Correct. > > > Do you think it will make sense to introduce Wix5 ToolsetType in case if we > > need to do something different between 4 and 5? > > They claim WiX5 is backward compatible with WiX4, i.e. WiX4 code should work > with WiX5 toolkit. So I don't see a reason to introduce Wix5 ToolsetType now. jpackage allows override of main WiX source file (main.wxs), do you know what will happen if user will add main.wxs with format features available only in WiX 5 and will have WiX 5 toolkit installed? Do you know if there any benefits to use any features available in WiX5 if WiX5 toolkit is installed, instead of using only WiX4 features with WiX5 toolkit? - PR Comment: https://git.openjdk.org/jdk/pull/19318#issuecomment-2150809380
RFR: 8325984: 4 jcstress tests are failing in Tier6 4 times each
These 4 tests were failing due to an incompatibility with jcstress. They were problemlisted in past (https://bugs.openjdk.org/browse/JDK-8326062). Now that jcstress has been updated (https://github.com/openjdk/jdk/pull/19332) with the relevant fix (https://github.com/openjdk/jcstress/pull/147), we can re-enable these tests. Testing: I've verified that all 4 tests now pass on Linux-x64 - Commit messages: - reenable jcstress tests Changes: https://git.openjdk.org/jdk/pull/19565/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19565=00 Issue: https://bugs.openjdk.org/browse/JDK-8325984 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19565.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19565/head:pull/19565 PR: https://git.openjdk.org/jdk/pull/19565
Re: RFR: 8319457: Update jpackage to support WiX v4 and v5 on Windows [v4]
On Wed, 5 Jun 2024 14:53:09 GMT, Alexey Semenyuk wrote: >> src/jdk.jpackage/windows/classes/jdk/jpackage/internal/resources/WinResources.properties >> line 44: >> >>> 42: resource.installdirnotemptydlg-wix-file=Not empty install directory >>> dialog WiX project file >>> 43: resource.launcher-as-service-wix-file=Service installer WiX project file >>> 44: resource.wix-src-conv=XSLT stylesheet converting WiX sources from WiX >>> v3 to WiX v4 format >> >> WiX v4 -> WiX v4/v5 > > The converter does exactly as described, it converts WiX v3 sources into WiX > v4 **format** that can be used with WiX v4 and WiX v5. So I believe the > description should remain unchanged. Makes sense, but for `error.no-wix-tools` I think we should add v5, in case if users sees this message when only v5 is installed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19318#discussion_r1628308316
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed unnecessary add-opens - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/1c0ab8eb..c583c633 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=08 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=07-08 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8294960: Convert java.base/java.lang.invoke package to use the Classfile API to generate lambdas and method handles [v14]
> java.base java.lang.invoke package heavily uses ASM to generate lambdas and > method handles. > > This patch converts ASM calls to Classfile API. > > This PR is continuation of https://github.com/openjdk/jdk/pull/12945 > > Any comments and suggestions are welcome. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Merge branch 'master' into JDK-8294960-invoke # Conflicts: # src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java - Merge branch 'master' into JDK-8294960-invoke # Conflicts: #src/java.base/share/classes/java/lang/classfile/Attributes.java - fixed CodeBuilder use in j.l.invoke - Merge branch 'master' into JDK-8294960-invoke - Merge pull request #4 from cl4es/boxunbox_holder Only create box/unbox MethodRefEntries on request - Only create box/unbox MethodRefEntries on request - Merge pull request #3 from cl4es/minor_init_improvements Reduce init overhead of InvokeBytecodeGenerator and StackMapGenerator - Remove stray MRE_LF_interpretWithArguments - Reduce init overhead of InvokeBytecodeGenerator and StackMapGenerator - Deferred initialization of attributes map by moving into a holder class Co-authored-by: Claes Redestad - ... and 14 more: https://git.openjdk.org/jdk/compare/f73922b2...9360b0eb - Changes: https://git.openjdk.org/jdk/pull/17108/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17108=13 Stats: 2113 lines in 10 files changed: 422 ins; 861 del; 830 mod Patch: https://git.openjdk.org/jdk/pull/17108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17108/head:pull/17108 PR: https://git.openjdk.org/jdk/pull/17108
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Tue, 4 Jun 2024 09:04:30 GMT, Magnus Ihse Bursie wrote: >>> Does that proposal sound good? >> >> That table is useful, I think it's right. No change to default behavior. If >> someone configures with --enable-runtime-image then they get a JDK run-time >> image that supports jlink (with some limitations) but is significantly small >> as it doesn't include the packaged modules. >> >> I've read through most of the changes now. Overall I think it's looking >> good, just a few terminology and minor points that I'll add as comments. > >> Does that proposal sound good? > > What you basically is saying is that the default value of `packaged-modules` > should be `! runtime-link-image` (i.e. the inverse)? @magicus @erikj79 Do you mind re-reviewing the build changes? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2150590534
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
> 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 b eing 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.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with six additional commits since the last revision: - Move JImageHelper - Update wording on multi-hop. - Remove printStackTrace() - Fix comment. Stream.toList() - Use pattern matching for instanceof in JRTArchive::equals - Rename JLINK_PRODUCE_RUNTIME_LINK_JDK to JLINK_PRODUCE_LINKABLE_RUNTIME - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/0eb1e48d..b72648ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14787=30 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=29-30 Stats: 28 lines in 10 files changed: 4 ins; 7 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:21:20 GMT, Alan Bateman wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 113 commits:
>>
>> - Mark some tests with requiring packaged modules
>> - Correctly set packaged modules default
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Fix new line in jlink.properties
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Simplify JLINK_JDK_EXTRA_OPTS var handling
>> - Only add runtime track files for linkable runtimes
>> - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
>
>> [5fef297](https://github.com/openjdk/jdk/commit/5fef297ba63d60452f098193d2cba33a941b)
>> implements this.
>
> I think it was surprising that --enable-runtime-link-image still included the
> packaged modules so thanks for taking the feedback on this point.
Thanks for the review, @AlanBateman! Updates pushed.
> test/jdk/tools/lib/tests/JImageHelper.java line 38:
>
>> 36: * JDK Modular image iterator
>> 37: */
>> 38: public class JImageHelper {
>
> This is only usable in tests that use `@modules` to open
> jdk.internal.jimage.*. It might be better co-locate this with the jlink
> tests for now. It may be that we do have test infra structure for jimage in
> the future but starting out with the supporting test lib in the jlink test
> tree should be okay.
Sure.
-
PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2150589379
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628156069
Integrated: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
On Thu, 30 May 2024 13:50:33 GMT, Jan Lahoda wrote:
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
This pull request has now been integrated.
Changeset: f7dbb98f
Author:Jan Lahoda
URL:
https://git.openjdk.org/jdk/commit/f7dbb98fe69eb98f8544577d81550b4fd817864b
Stats: 226 lines in 2 files changed: 213 ins; 1 del; 12 mod
8333086: Using Console.println is unnecessarily slow due to JLine initalization
Reviewed-by: asotona, naoto
-
PR: https://git.openjdk.org/jdk/pull/19479
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:46:59 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 113 commits: >> >> - Mark some tests with requiring packaged modules >> - Correctly set packaged modules default >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Fix new line in jlink.properties >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Simplify JLINK_JDK_EXTRA_OPTS var handling >> - Only add runtime track files for linkable runtimes >> - Generate the runtime image link diff at jlink time >> >>But only do that once the plugin-pipeline has run. The generation is >>enabled with the hidden option '--generate-linkable-runtime' and >>the resource pools available at jlink time are being used to generate >>the diff. >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d > > src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line > 119: > >> 117: >> 118: err.runtime.link.not.linkable.runtime=The current run-time image does >> not support run-time linking. >> 119: err.runtime.link.jdk.jlink.prohibited=Including jdk.jlink module for >> run-time image based links is not allowed. > > The phrase "run-time image based links" in this error message (and in the > value of err.runtime.link.packaged.mods) is a bit unusual. As developers will > see this message then maybe it could say that this jlink in the current > run-time image doesn't contain packaged modules and cannot be used to create > another run-time image that include the jdk.jlink module. I've used alternative phrasing. Hopefully better now. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628146154
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization [v3]
On Wed, 5 Jun 2024 12:36:33 GMT, Jan Lahoda wrote:
>> Consider these two programs:
>>
>>
>> public class SystemPrint {
>> public static void main(String... args) {
>> System.err.println("Hello!");
>> }
>> }
>>
>> and:
>>
>> public class IOPrint {
>> public static void main(String... args) {
>> java.io.IO.println("Hello!");
>> }
>> }
>>
>>
>> They do the same conceptual thing - write a text to the output. But,
>> `IO.println` delegates to `Console.println`, which then delegates to a
>> `Console` backend, and the default backend is currently based on JLine.
>>
>> The issues is that JLine takes a quite a long time to initialize, and in a
>> program like this, JLine is not really needed - it is used to provide better
>> editing experience when reading input, but there's no reading in these
>> programs.
>>
>> For example, on my computer:
>>
>> $ time java -classpath /tmp SystemPrint
>> Hello!
>>
>> real0m0,035s
>> user0m0,019s
>> sys 0m0,019s
>>
>> $ time java -classpath /tmp --enable-preview IOPrint
>> Hello!
>>
>> real0m0,165s
>> user0m0,324s
>> sys 0m0,042s
>>
>>
>> The proposal herein is to delegate to the simpler `Console` backend from
>> `java.base` as long as the user only uses methods that print to output, and
>> switch to the JLine delegate when other methods (typically input) is used.
>> Note that while technically `writer()` is a method doing output, it will
>> force JLine initialization to avoid possible problems if the client caches
>> the writer and uses it after switching the delegates.
>>
>> With this patch, I can get timing like this:
>>
>> $ time java --enable-preview -classpath /tmp/ IOPrint
>> Hello!
>>
>> real0m0,051s
>> user0m0,038s
>> sys 0m0,020s
>>
>>
>> which seems much more acceptable.
>>
>> There is also #19467, which may reduce the time further.
>>
>> A future work might focus on making JLine initialize faster, but avoiding
>> JLine initialization in case where we don't need it seems like a good step
>> to me in any case.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Correctly reflecting review feedback
LGTM. Thanks for the changes.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19479#pullrequestreview-2099801902
Re: RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix [v2]
On Tue, 4 Jun 2024 20:43:09 GMT, Justin Lu wrote:
>> Please review this PR which handles incorrect CompactNumberFormat integer
>> only parsing when there is no suffix.
>>
>> See the following snippet,
>>
>>
>> var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
>> NumberFormat.Style.SHORT)
>> fmt.setParseIntegerOnly(true)
>> fmt.parse("5K") // returns 5000
>> fmt.parse("50.00") // returns 50
>> fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
>> fmt.parse("5 ") // returns 5
>>
>>
>> Within the `parse` method, there is code that advances `position` past the
>> fractional portion to find the suffix when `parseIntegerOnly()` is true.
>> However, if a valid string input is given with no suffix,
>> `DecimalFormat.subparseNumber()` will fully iterate through the string and
>> `position` will be equal to the length of the string, throwing
>> StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
>>
>> We should check to make sure position does not exceed the string length
>> before deciding to check for a decimal symbol.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> move test to existing test file
LGTM. Thanks for the changes.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19533#pullrequestreview-2099786378
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:54:07 GMT, Alan Bateman wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 113 commits:
>>
>> - Mark some tests with requiring packaged modules
>> - Correctly set packaged modules default
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Fix new line in jlink.properties
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Simplify JLINK_JDK_EXTRA_OPTS var handling
>> - Only add runtime track files for linkable runtimes
>> - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
> line 215:
>
>> 213: }
>> 214: } catch (IOException e) {
>> 215: e.printStackTrace();
>
> Is this IOException swallowed by jlink when not running with the debugging
> option? I'm wondering about the printStackTrace here.
I think this is a left-over of some devevelopment work. Removed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628115979
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Wed, 5 Jun 2024 13:52:43 GMT, Alan Bateman wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 113 commits:
>>
>> - Mark some tests with requiring packaged modules
>> - Correctly set packaged modules default
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Fix new line in jlink.properties
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - Simplify JLINK_JDK_EXTRA_OPTS var handling
>> - Only add runtime track files for linkable runtimes
>> - Generate the runtime image link diff at jlink time
>>
>>But only do that once the plugin-pipeline has run. The generation is
>>enabled with the hidden option '--generate-linkable-runtime' and
>>the resource pools available at jlink time are being used to generate
>>the diff.
>> - Merge branch 'master' into jdk-8311302-jmodless-link
>> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
> line 39:
>
>> 37:
>> 38: /**
>> 39: * Class representing a difference between a jimage resource. For all
>> intents
>
> A difference between a jimage resource and ??? Someone might wonder about
> that.
I've updated the comment a bit to make it clearer.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java
> line 33:
>
>> 31: import jdk.tools.jlink.plugin.ResourcePool;
>> 32:
>> 33: @SuppressWarnings("try")
>
> Is this needed?
Yes. Otherwise this warning is produced:
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java:32:
warning: [try] auto-closeable resource ResourcePoolReader has a member method
close() that could throw InterruptedException
public class ResourcePoolReader implements ImageResource {
^
error: warnings found and -Werror specified
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628112307
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1628111856
Integrated: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
On Mon, 3 Jun 2024 05:06:00 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. This pull request has now been integrated. Changeset: d7d1afb0 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d7d1afb0a84e771870e9f43e08c4a63c8fdccdd9 Stats: 21 lines in 2 files changed: 11 ins; 2 del; 8 mod 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE Reviewed-by: lancea, alanb - PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
On Wed, 5 Jun 2024 12:14:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the API specification >> of `java.util.zip.InflaterInputStream.skip()` method to match its current >> implementation? >> >> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes >> a `long` value `n` representing the number of bytes to skip. When a value >> greater than `Integer.MAX_VALUE` is passed to this >> `InflaterInputStream.skip()` method, the current implementation in that >> method only skips at most `Integer.MAX_VALUE` bytes. >> `DeflaterInputStream.skip()` too has the same behaviour. However, in the >> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that >> method's API documentation. `InflaterInputStream.skip()` currently doesn't >> specify this behaviour. >> >> The commit in this PR updates the `InflaterInputStream.skip()` to specify >> the current implementation. >> >> I'll open a CSR for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > clarify blocking semantic Thank you all for the reviews and the inputs to the specification text. The CSR has been approved, so I'll integrate this now. - PR Comment: https://git.openjdk.org/jdk/pull/19515#issuecomment-2150413847
Re: RFR: 8319457: Update jpackage to support WiX v4 and v5 on Windows [v4]
On Wed, 5 Jun 2024 03:54:46 GMT, Alexander Matveev wrote:
>> Alexey Semenyuk has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - WixSourceConverter#applyTo should do what
>> OverridableResource#saveToFile() does: create parent directory and replace
>> output file if it exists.
>> - Fix issue with overwriting custom l10n file in the resource directory.
>> All WiX source files from the resource directory should be copied to
>> jpackage work directory before running wxi tools. jpackage should not change
>> files in the resource directory.
>
> src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixAppImageFragmentBuilder.java
> line 157:
>
>> 155:
>> 156: @Override
>> 157: List getLoggableWixFeatures() {
>
> Maybe I am missing something, but is it used? I only see call to base class
> `WixFragmentBuilder::getLoggableWixFeatures`.
`WixFragmentBuilder::getLoggableWixFeatures` is equivalent to:
new Function>() {
public List apply(WixFragmentBuilder obj) {
return obj.getLoggableWixFeatures();
}
}
An overridden WixAppImageFragmentBuilder#getLoggableWixFeatures() method will
be called when WixAppImageFragmentBuilder instance is given.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19318#discussion_r1628020809
Re: RFR: 8330182: Start of release updates for JDK 24 [v12]
On Wed, 5 Jun 2024 15:35:18 GMT, Joe Darcy wrote: >> Get JDK 24 underway. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 23 commits: > > - Update copyright. > - Updated problem list after bug fix. > - Merge branch 'master' into JDK-8330188 > - Merge branch 'master' into JDK-8330188 > - Temporarily problem list java.lang.instrument tests until jar generation > is fixed. > - Merge branch 'master' into JDK-8330188 > - Update symbol files for JDK 23 build 25. > - Correct release year. > - Merge branch 'master' into JDK-8330188 > - Add symbol files current with JDK 23 build 24. > - ... and 13 more: https://git.openjdk.org/jdk/compare/4369856c...f4a5026b Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18787#pullrequestreview-2099585754
Re: RFR: 8319457: Update jpackage to support WiX v4 and v5 on Windows [v4]
On Tue, 4 Jun 2024 01:20:15 GMT, Alexander Matveev wrote:
> Do we need to handle default case for unknown wix type? In same cases you
> have default -> throw new IllegalArgumentException(); and in some you do not
> have.
Good catch. I'll update the code to make `switch(wixVersion) ...` expressions
consistent as follows:
switch (wixVersion) {
case Wix3 -> {}
case WiX4 -> {}
default -> throw new IllegalArgumentException();
}
-
PR Comment: https://git.openjdk.org/jdk/pull/19318#issuecomment-2150382290
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
On Wed, 5 Jun 2024 12:14:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the API specification >> of `java.util.zip.InflaterInputStream.skip()` method to match its current >> implementation? >> >> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes >> a `long` value `n` representing the number of bytes to skip. When a value >> greater than `Integer.MAX_VALUE` is passed to this >> `InflaterInputStream.skip()` method, the current implementation in that >> method only skips at most `Integer.MAX_VALUE` bytes. >> `DeflaterInputStream.skip()` too has the same behaviour. However, in the >> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that >> method's API documentation. `InflaterInputStream.skip()` currently doesn't >> specify this behaviour. >> >> The commit in this PR updates the `InflaterInputStream.skip()` to specify >> the current implementation. >> >> I'll open a CSR for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > clarify blocking semantic latest update looks good. Thank you and Alan for the additional wordsmithing - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19515#pullrequestreview-2099574770
Integrated: 8332457: Examine startup overheads from JDK-8294961
On Mon, 27 May 2024 09:01:36 GMT, Adam Sotona wrote: > [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > The generated proxy is migrated from static initialization to CONDY bootstrap. > > Please review. > > Thank you, > Adam This pull request has now been integrated. Changeset: d85b0ca5 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/d85b0ca5cdc1820a886c46bf555b2051fed7f167 Stats: 500 lines in 9 files changed: 174 ins; 197 del; 129 mod 8332457: Examine startup overheads from JDK-8294961 8229959: Convert proxy class to use constant dynamic Reviewed-by: liach, redestad - PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8330182: Start of release updates for JDK 24 [v12]
> Get JDK 24 underway. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - Update copyright. - Updated problem list after bug fix. - Merge branch 'master' into JDK-8330188 - Merge branch 'master' into JDK-8330188 - Temporarily problem list java.lang.instrument tests until jar generation is fixed. - Merge branch 'master' into JDK-8330188 - Update symbol files for JDK 23 build 25. - Correct release year. - Merge branch 'master' into JDK-8330188 - Add symbol files current with JDK 23 build 24. - ... and 13 more: https://git.openjdk.org/jdk/compare/4369856c...f4a5026b - Changes: https://git.openjdk.org/jdk/pull/18787/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18787=11 Stats: 2083 lines in 50 files changed: 2019 ins; 7 del; 57 mod Patch: https://git.openjdk.org/jdk/pull/18787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18787/head:pull/18787 PR: https://git.openjdk.org/jdk/pull/18787
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v2]
On Tue, 4 Jun 2024 19:58:41 GMT, jengebr wrote: >> Agreed. You might as well be consistent with this so that no one will need >> to re-address if some application does zillions of clones of empty lists. > > Sorry, I crossed wires with @DougLea 's comment, will add `clone`. `clone()` performs a shallow copy, so there is currently no `Object[]` allocated and therefore nothing to optimize. I don't think there's any work to do here. @DougLea @viktorklang-ora can you confirm? - PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1627947686
Re: RFR: 8319457: Update jpackage to support WiX v4 and v5 on Windows [v4]
On Wed, 5 Jun 2024 04:20:17 GMT, Alexander Matveev wrote: >> Alexey Semenyuk has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - WixSourceConverter#applyTo should do what >> OverridableResource#saveToFile() does: create parent directory and replace >> output file if it exists. >> - Fix issue with overwriting custom l10n file in the resource directory. >> All WiX source files from the resource directory should be copied to >> jpackage work directory before running wxi tools. jpackage should not change >> files in the resource directory. > > src/jdk.jpackage/windows/classes/jdk/jpackage/internal/resources/WinResources.properties > line 44: > >> 42: resource.installdirnotemptydlg-wix-file=Not empty install directory >> dialog WiX project file >> 43: resource.launcher-as-service-wix-file=Service installer WiX project file >> 44: resource.wix-src-conv=XSLT stylesheet converting WiX sources from WiX v3 >> to WiX v4 format > > WiX v4 -> WiX v4/v5 The converter does exactly as described, it converts WiX v3 sources into WiX v4 **format** that can be used with WiX v4 and WiX v5. So I believe the description should remain unchanged. - PR Review Comment: https://git.openjdk.org/jdk/pull/19318#discussion_r1627942141
Integrated: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang wrote: > Primarily offering this PR for discussion, as Throwables throwing exceptions > on toString(), getLocalizedMessage(), or getMessage() seems like a rather > unreasonable thing to do. > > Nevertheless, there is some things we can do, as witnessed in this PR. This pull request has now been integrated. Changeset: 326dbb1b Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/326dbb1b139dd1ec1b8605339b91697cdf49da9a Stats: 63 lines in 2 files changed: 52 ins; 2 del; 9 mod 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/18988
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang wrote: > Primarily offering this PR for discussion, as Throwables throwing exceptions > on toString(), getLocalizedMessage(), or getMessage() seems like a rather > unreasonable thing to do. > > Nevertheless, there is some things we can do, as witnessed in this PR. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18988#pullrequestreview-2099417935
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 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 with a new target base due to a
> merge or a rebase. The pull request now contains 113 commits:
>
> - Mark some tests with requiring packaged modules
> - Correctly set packaged modules default
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Fix new line in jlink.properties
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Simplify JLINK_JDK_EXTRA_OPTS var handling
> - Only add runtime track files for linkable runtimes
> - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
line 39:
> 37:
> 38: /**
> 39: * Class representing a difference between a jimage resource. For all
> intents
A difference between a jimage resource and ??? Someone might wonder about that.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
line 215:
> 213: }
> 214: } catch (IOException e) {
> 215: e.printStackTrace();
Is this IOException swallowed by jlink when not running with the debugging
option? I'm wondering about the printStackTrace here.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java
line 33:
> 31: import jdk.tools.jlink.plugin.ResourcePool;
> 32:
> 33: @SuppressWarnings("try")
Is this needed?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java
line 49:
> 47: @Override
> 48: public List getEntries() {
> 49: return pool.entries().map(a ->
> a.path()).collect(Collectors.toList());
I think you can use toList() here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627830148
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627833211
PR Review Comment:
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 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 with a new target base due to a > merge or a rebase. The pull request now contains 113 commits: > > - Mark some tests with requiring packaged modules > - Correctly set packaged modules default > - Merge branch 'master' into jdk-8311302-jmodless-link > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 119: > 117: > 118: err.runtime.link.not.linkable.runtime=The current run-time image does > not support run-time linking. > 119: err.runtime.link.jdk.jlink.prohibited=Including jdk.jlink module for > run-time image based links is not allowed. The phrase "run-time image based links" in this error message (and in the value of err.runtime.link.packaged.mods) is a bit unusual. As developers will see this message then maybe it could say that this jlink in the current run-time image doesn't contain packaged modules and cannot be used to create another run-time image that include the jdk.jlink module. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627820124
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 5 Jun 2024 13:41:10 GMT, Per Minborg wrote: >> In order to be eligible for constant folding, the benchmark must declare the >> `Method hashCodeMethod;` as `static final`. >> >> It is hard for me to understand why a `@Stable` annotation should have a >> detrimental performance impact on an instance field. >> >> ~~Can we see a benchmark on Arm Neoverse N1 with the field declared >> `@Stable` compared to not declared `@Stable`? ~~ Also, if the field is >> `static final`, how would it look like? > > As a note, If we would have access to the contemplated `StableValue` and > `hash` was declared even as an _instance variable_ `StableValue` in > a regular class, then it would be trusted and would be eligible for constant > folding due to the VM will have special rules for fields of StableValue type. Ahh. There was a benchmark in the initial message. Sorry. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1627818122
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 5 Jun 2024 13:37:15 GMT, Per Minborg wrote: >> Interesting, don't know about hotspot internals so I can't diagnose the >> exact cause of this regression. > > In order to be eligible for constant folding, the benchmark must declare the > `Method hashCodeMethod;` as `static final`. > > It is hard for me to understand why a `@Stable` annotation should have a > detrimental performance impact on an instance field. > > Can we see a benchmark on Arm Neoverse N1 with the field declared `@Stable` > compared to not declared `@Stable`? Also, if the field is `static final`, how > would it look like? As a note, If we would have access to the contemplated `StableValue` and `hash` was declared even as an _instance variable_ `StableValue` in a regular class, then it would be trusted and would be eligible for constant folding due to the VM will have special rules for fields of StableValue type. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1627810415
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 29 May 2024 15:20:15 GMT, Chen Liang wrote: >> This was something that I tried and I observed a performance regression on >> the ARM platform that I was testing. From my understanding, `@Stable` tells >> the compiler to trust the contents of this field which is only given when >> the field holder is a known constant. This is generally true for cases like >> `Enums` but not usually for `Method`. >> >> >> Benchmark Mode Cnt Score Error Units >> # Intel Skylake >> MethodHashCode.benchmarkHashCode avgt5 1.113 ± 1.146 ns/op >> # Arm Neoverse N1 >> MethodHashCode.benchmarkHashCode avgt5 3.204 ± 0.001 ns/op > > Interesting, don't know about hotspot internals so I can't diagnose the exact > cause of this regression. In order to be eligible for constant folding, the benchmark must declare the `Method hashCodeMethod;` as `static final`. It is hard for me to understand why a `@Stable` annotation should have a detrimental performance impact on an instance field. Can we see a benchmark on Arm Neoverse N1 with the field declared `@Stable` compared to not declared `@Stable`? Also, if the field is `static final`, how would it look like? - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1627803876
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 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 with a new target base due to a
> merge or a rebase. The pull request now contains 113 commits:
>
> - Mark some tests with requiring packaged modules
> - Correctly set packaged modules default
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Fix new line in jlink.properties
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Simplify JLINK_JDK_EXTRA_OPTS var handling
> - Only add runtime track files for linkable runtimes
> - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 131:
> 129: public boolean equals(Object obj) {
> 130: if (obj instanceof JRTArchive) {
> 131: JRTArchive other = (JRTArchive)obj;
Cleanup for later, you can use pattern matching here and avoid the cast.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627798949
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 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 with a new target base due to a > merge or a rebase. The pull request now contains 113 commits: > > - Mark some tests with requiring packaged modules > - Correctly set packaged modules default > - Merge branch 'master' into jdk-8311302-jmodless-link > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d make/Images.gmk line 101: > 99: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) > 100: JLINK_JDK_EXTRA_OPTS += --generate-linkable-runtime > 101: endif In passing, I suppose this could be JLINK_PRODUCE_LINKABLE_RUNTIME to be consistent with the configure option. No big deal of course, just noticed a few places where the words are transposed. make/autoconf/jdk-options.m4 line 594: > 592: DEFAULT_PACKAGED_MODULES=false > 593: else > 594: DEFAULT_PACKAGED_MODULES=true Good! - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627791600 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627791805
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 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 with a new target base due to a
> merge or a rebase. The pull request now contains 113 commits:
>
> - Mark some tests with requiring packaged modules
> - Correctly set packaged modules default
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Fix new line in jlink.properties
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Simplify JLINK_JDK_EXTRA_OPTS var handling
> - Only add runtime track files for linkable runtimes
> - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
test/jdk/tools/lib/tests/JImageHelper.java line 38:
> 36: * JDK Modular image iterator
> 37: */
> 38: public class JImageHelper {
This is only usable in tests that use `@modules` to open jdk.internal.jimage.*.
It might be better co-locate this with the jlink tests for now. It may be that
we do have test infra structure for jimage in the future but starting out with
the supporting test lib in the jlink test tree should be okay.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627780757
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang wrote: > Primarily offering this PR for discussion, as Throwables throwing exceptions > on toString(), getLocalizedMessage(), or getMessage() seems like a rather > unreasonable thing to do. > > Nevertheless, there is some things we can do, as witnessed in this PR. @AlanBateman Do you have a minute to review this one? - PR Comment: https://git.openjdk.org/jdk/pull/18988#issuecomment-2149910528
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Tue, 4 Jun 2024 14:39:23 GMT, Severin Gehwolf wrote: > I've added a couple of `@requires jlink.packagedModules` (new with this > patch) so that jlink tests don't start to fail with it. Good, the `@requires jlink.packagedModules` make sense. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-214941
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 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 with a new target base due to a > merge or a rebase. The pull request now contains 113 commits: > > - Mark some tests with requiring packaged modules > - Correctly set packaged modules default > - Merge branch 'master' into jdk-8311302-jmodless-link > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d > [5fef297](https://github.com/openjdk/jdk/commit/5fef297ba63d60452f098193d2cba33a941b) > implements this. I think it was surprising that --enable-runtime-link-image still included the packaged modules so thanks for taking the feedback on this point. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2149894976
Integrated: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles wrote: > Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia This pull request has now been integrated. Changeset: cbb6747e Author:Sonia Zaldana Calles Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/cbb6747e6b9ce7e2b9e0ffb0a1f9499f7e0e13b0 Stats: 351 lines in 4 files changed: 309 ins; 10 del; 32 mod 8329581: Java launcher no longer prints a stack trace 8329420: Java 22 (and 23) launcher calls default constructor although main() is static 8330864: No error message when ExceptionInInitializerError thrown in static initializer Reviewed-by: stuefe - PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v20]
On Wed, 5 Jun 2024 12:50:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/micro/org/openjdk/bench/java/lang/reflect/ProxyGenBench.java > > Co-authored-by: Chen Liang Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2099091898
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman > @SoniaZaldana CI testing of this PR along with latest master commits passed > without any failures. So you can now integrate this. Thanks @jaikiran! - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2149833060
Integrated: 8322732: ForkJoinPool may underutilize cores in async mode
On Tue, 7 May 2024 22:50:18 GMT, Doug Lea wrote: > This set of changes address causes of poor utilization with small numbers of > cores due to overly aggressive contention avoidance. A number of further > adjustments were needed to still avoid most contention effects in deployments > with large numbers of cores This pull request has now been integrated. Changeset: 789f704d Author:Doug Lea URL: https://git.openjdk.org/jdk/commit/789f704d9ab5aaf87193f508859c4c9a528d7779 Stats: 735 lines in 2 files changed: 250 ins; 184 del; 301 mod 8322732: ForkJoinPool may underutilize cores in async mode 8327854: Test java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpStatefulTest.java failed with RuntimeException Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/19131
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v20]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > The generated proxy is migrated from static initialization to CONDY bootstrap. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update test/micro/org/openjdk/bench/java/lang/reflect/ProxyGenBench.java Co-authored-by: Chen Liang - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/54376fe8..a0822718 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=19 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=18-19 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
On Wed, 5 Jun 2024 12:00:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > An assortment of potential improvements > > Co-authored-by: Claes Redestad src/java.base/share/classes/java/lang/constant/ConstantDescs.java line 356: > 354: ClassDesc[] fullParamTypes = new ClassDesc[paramTypes.length + > prefixLen]; > 355: System.arraycopy(INDY_BOOTSTRAP_ARGS, 0, fullParamTypes, 0, > prefixLen); > 356: System.arraycopy(paramTypes, 0, fullParamTypes, prefixLen, > paramTypes.length); I'm thinking about creating a basic MethodTypeDesc like `INDY_BOOTSTRAP_TYPE = MethodTypeDesc.ofValidated(CD_Object, INDY_BOOTSTRAP_ARGS)`, and we derive bsm with INDY_BOOTSTRAP_TYPE .insertParameterTypes(INDY_BOOTSTRAP_TYPE.parameterCount(), paramTypes) .changeReturnType(returnType) which creates two MTD wrappers but they share the same parameter array test/micro/org/openjdk/bench/java/lang/reflect/ProxyGenBench.java line 2: > 1: /* > 2: * Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1627683294 PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1627685503
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization [v3]
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Correctly reflecting review feedback
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19479/files
- new: https://git.openjdk.org/jdk/pull/19479/files/7a0c448f..f3259793
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19479=02
- incr: https://webrevs.openjdk.org/?repo=jdk=19479=01-02
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19479/head:pull/19479
PR: https://git.openjdk.org/jdk/pull/19479
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v17]
On Wed, 5 Jun 2024 11:45:10 GMT, Alan Bateman wrote: >> Doug Lea has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reconcile changes > > Viktor (mostly) and I have been testing and reviewing these changes at each > iteration. I think we are happy with where things are at, significantly very > positive in some of the streams/gatherer tests. The Starvation test is the > canary to catch a scenario that has been observed along the way, but not with > the current version. So I think this is good to integrate. Seconding @AlanBateman, I've reviewed and provided feedback on this PR multiple times, and have had numerous runs of parallel streams / gatherers and I think this is good to go. If I was a reviewer my vote would matter, but I'm adding this comment here as an "enthusiastic supporter". :) - PR Comment: https://git.openjdk.org/jdk/pull/19131#issuecomment-2149714806
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
On Wed, 5 Jun 2024 12:00:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > An assortment of potential improvements > > Co-authored-by: Claes Redestad Looks good. Generation of proxy classes gets a nice boost this way. The condy bsm calls that may happen later takes a small hit which we can improve in a follow-up. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2098990024
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
On Wed, 5 Jun 2024 12:00:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > An assortment of potential improvements > > Co-authored-by: Claes Redestad Benchmark Mode Cnt Score Error Units ProxyGenBench.generate100Proxies(master)ss 10 15.295 ? 5.435 ms/op ProxyGenBench.generate100Proxies(#19410)ss 10 11.761 ? 3.323 ms/op Finished running test 'micro:.+ProxyGenBench.+' - PR Comment: https://git.openjdk.org/jdk/pull/19410#issuecomment-2149694078
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
On Wed, 5 Jun 2024 12:14:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the API specification >> of `java.util.zip.InflaterInputStream.skip()` method to match its current >> implementation? >> >> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes >> a `long` value `n` representing the number of bytes to skip. When a value >> greater than `Integer.MAX_VALUE` is passed to this >> `InflaterInputStream.skip()` method, the current implementation in that >> method only skips at most `Integer.MAX_VALUE` bytes. >> `DeflaterInputStream.skip()` too has the same behaviour. However, in the >> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that >> method's API documentation. `InflaterInputStream.skip()` currently doesn't >> specify this behaviour. >> >> The commit in this PR updates the `InflaterInputStream.skip()` to specify >> the current implementation. >> >> I'll open a CSR for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > clarify blocking semantic This went through a few iterations in the CSR issue, looking good now. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19515#pullrequestreview-2098975614
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
> Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: clarify blocking semantic - Changes: - all: https://git.openjdk.org/jdk/pull/19515/files - new: https://git.openjdk.org/jdk/pull/19515/files/a43e96de..b7b870d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19515=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=02-03 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
On Wed, 5 Jun 2024 12:00:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > An assortment of potential improvements > > Co-authored-by: Claes Redestad Name Cnt Base Error Test Error Unit Change Perfstartup-JMH 2040.000 ± 0.000 40.000 ± 0.000 ms/op 1.00x (p = 0.000 ) :.cycles 236365197.300 ± 3630668.331 224263724.700 ± 4160144.635 cycles 0.95x (p = 0.000*) :.instructions 602001346.250 ± 3134647.039 574894340.900 ± 2297441.310 instructions 0.95x (p = 0.000*) :.taskclock 60.000 ± 0.000 58.500 ± 3.181 ms 0.98x (p = 0.083 ) * = significant - PR Comment: https://git.openjdk.org/jdk/pull/19410#issuecomment-2149681885
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use > classfile API for reflection proxy-generation. Actual implementation of > `ProxyGenerator` is focused on performance, however it causes JDK bootstrap > regressions. `ProxyGenerator.TEMPLATE` class model is statically created and > each proxy class is transformed from the template. > > This patch is intended to examine plain proxy generation impact on > performance and JDK bootstrap (vs proxy transformation from template). > > The generated proxy is migrated from static initialization to CONDY bootstrap. > > Please review. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: An assortment of potential improvements Co-authored-by: Claes Redestad - Changes: - all: https://git.openjdk.org/jdk/pull/19410/files - new: https://git.openjdk.org/jdk/pull/19410/files/6ca164bc..54376fe8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19410=18 - incr: https://webrevs.openjdk.org/?repo=jdk=19410=17-18 Stats: 25 lines in 5 files changed: 15 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/19410.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19410/head:pull/19410 PR: https://git.openjdk.org/jdk/pull/19410
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v17]
On Fri, 31 May 2024 13:18:33 GMT, Doug Lea wrote: >> This set of changes address causes of poor utilization with small numbers of >> cores due to overly aggressive contention avoidance. A number of further >> adjustments were needed to still avoid most contention effects in >> deployments with large numbers of cores > > Doug Lea has updated the pull request incrementally with one additional > commit since the last revision: > > Reconcile changes Viktor (mostly) and I have been testing and reviewing these changes at each iteration. I think we are happy with where things are at, significantly very positive in some of the streams/gatherer tests. The Starvation test is the canary to catch a scenario that has been observed along the way, but not with the current version. So I think this is good to integrate. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19131#pullrequestreview-2098894992
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v3]
> Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: review feedback updates from the CSR - Changes: - all: https://git.openjdk.org/jdk/pull/19515/files - new: https://git.openjdk.org/jdk/pull/19515/files/e13e65d2..a43e96de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19515=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=01-02 Stats: 12 lines in 2 files changed: 2 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v12]
On Wed, 22 May 2024 14:19:36 GMT, Volodymyr Paprotski wrote: >> Volodymyr Paprotski 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 17 >> additional commits since the last revision: >> >> - Merge remote-tracking branch 'origin/master' into ecc-montgomery >> - shenandoah verifier >> - comments from Sandhya >> - whitespace >> - add message back >> - whitespace >> - Use AffinePoint to exit Montgomery domain >> >>Style notes: >>Affine.equals() >>- Mismatched fields only appear to be used from testing, perhaps >> should be moved there instead >>Affine.getX(boolean)|getY(boolean) >>- "Passing flag is bad design" - cleanest/performant alternative to >> several instanceof checks >>- needed to convert Affine to Projective (need to stay in montgomery >> domain) >>ECOperations.PointMultiplier >> - changes could probably be restored to original (since >> ProjectivePoint handling no longer required) >> - consider these changes an improvement? (fewer nested classes) >> - was an inner-class but not using inner-class features (i.e. ecOps >> variable should be converted) >> - whitespace >> - Comments from Tony and Jatin >> - Comments from Jatin and Tony >> - ... and 7 more: https://git.openjdk.org/jdk/compare/1adfff34...b1a33004 > > Thanks Tobi! Unfortunately, this caused a performance regression, see [JDK-8333583](https://bugs.openjdk.org/browse/JDK-8333583). @vpaprotsk, please have a look. - PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2149576062
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization [v2]
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review feedback - explicitly selecting the jdk.internal.le
provider in the test.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19479/files
- new: https://git.openjdk.org/jdk/pull/19479/files/9886732e..7a0c448f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19479=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19479=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19479/head:pull/19479
PR: https://git.openjdk.org/jdk/pull/19479
