8264322: Generate CDS archive when creating custom JDK image #5174

calvinccheung · 2021-08-18T21:21:06Z

Please review this change for adding a jlink command line option --generate-cds-archive for generating CDS archives as a post processing step during the creation of a custom JDK image.

Details can be found in the corresponding CSR JDK-8269178.

Testing:

tiers 1,2 (including the new test)

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8264322: Generate CDS archive when creating custom JDK image

Reviewers

Mandy Chung (@mlchung - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5174/head:pull/5174
$ git checkout pull/5174

Update a local copy of the PR:
$ git checkout pull/5174
$ git pull https://git.openjdk.java.net/jdk pull/5174/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5174

View PR using the GUI difftool:
$ git pr show -t 5174

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5174.diff

bridgekeeper · 2021-08-18T21:22:20Z

👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

calvinccheung · 2021-08-18T21:22:20Z

/label remove hotspot
/label add core-libs
/label add hotspot-runtime

openjdk · 2021-08-18T21:23:47Z

@calvinccheung The hotspot label was not set.

openjdk · 2021-08-18T21:24:08Z

@calvinccheung
The core-libs label was successfully added.

openjdk · 2021-08-18T21:25:08Z

@calvinccheung
The hotspot-runtime label was successfully added.

mlbridge · 2021-08-18T21:32:49Z

Webrevs

01: Full - Incremental (b7a5ec68)
00: Full (918d5066)

mlchung · 2021-08-18T23:55:25Z

src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java

@@ -86,6 +86,7 @@
        private final Path home;
        private final List<String> args;
        private final Set<String> modules;
+        private Platform platform;


Can DefaultExecutableImage constructor take an additional platform argument and make this platform field final?

When the DefaultExecutableImage is constructed, it already has the target platform information.

In the constructor, it can check if the platform parameter must not be UNKNOWN; otherwise throw IAE.

I've added the platform argument and made the platform field final.
However, as we've discussed offline, there's a code path via the --post-process-path option where the platform may not be available. So we can't throw IAE on an UNKNOWN platform in the constructor.

Since --post-process-path is a hidden option, it's fine not to support it.

mlchung · 2021-08-18T23:58:40Z

src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java

@@ -127,13 +129,29 @@ public void storeLaunchArgs(List<String> args) {
                throw new UncheckedIOException(ex);
            }
        }
+
+        @Override
+        public void setTargetPlatform(Platform p) {


This setter is not needed if it's passed to the constructor.

mlchung · 2021-08-18T23:59:40Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java

+     *
+     * @param p
+     */
+    public void setTargetPlatform(Platform p);


This method should not be needed. The platform should be known when an executable image is created.

mlchung · 2021-08-19T00:01:14Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java

+     *
+     * @return boolean
+     */
+    public boolean is64Bit();


This is64Bit method should belong to Platform class

Done. I've moved the method to the Platform class.

mlchung · 2021-08-19T00:01:39Z

src/jdk.jlink/share/classes/jdk/tools/jlink/builder/ImageBuilder.java

+    /**
+     * Gets the target image platform.
+     *
+     * @return Platform


Suggested change

* @return Platform

* @return {@code Platform} object representing the platform of the runtime image

mlchung · 2021-08-19T00:18:54Z

src/jdk.jlink/share/classes/jdk/tools/jlink/resources/plugins.properties

+generate-cds-archive.argument=
+
+generate-cds-archive.description=\
+CDS plugin: generate cds archives (classes.jsa, classes_nocoops.jsa).\n\


Is it necessary to show classes.jsa, classes_nocoops.jsa the archive file name? Simply drop them?

Removed the file names.

mlchung · 2021-08-19T00:21:19Z

src/jdk.jlink/share/classes/jdk/tools/jlink/resources/plugins.properties

+
+generate-cds-archive.usage=\
+\  --generate-cds-archive    Generate CDS archives (classes.jsa, classes_nocoops.jsa).\n\
+\                            Applicable to JDK images built with the CDS feature.


Suggested change

\ Applicable to JDK images built with the CDS feature.

\ --generate-cds-archive Generate CDS archives if the runtime image supports CDS feature.\n\

Does this match what you want to say?

mlchung · 2021-08-19T00:26:09Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CDSPlugin.java

+            ex.printStackTrace();
+        }
+        if (status == 0) {
+            System.out.println("Created " + archiveMsg + " archive successfully");


Is it significant to print two lines on 64-bit platform? Users may not have any idea what NOCOOPS means?

Created CDS archive successfully Created NOCOOPS CDS archive successfully

It seems adequate to me as a user to get one output:

Created CDS archive successfully

I've made the change; the plugin will print only one successful message.

mlchung · 2021-08-19T00:32:00Z

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CDSPlugin.java

+            Process p = builder.inheritIO().start();
+            status = p.waitFor();
+        } catch (Exception ex) {
+            ex.printStackTrace();


This plugin should fail with any exception. jlink will handle PluginException and UncheckedIOException and print the error message. You can simply wrap IOException with UncheckedIOException

try { Process p = builder.inheritIO().start(); status = p.waitFor(); } catch (IOException e) { throw new UncheckedIOException(e); }

I've changed it to the following since we also need to catch the InterruptedException:

try { Process p = builder.inheritIO().start(); status = p.waitFor(); } catch (InterruptedException | IOException e) { throw new PluginException(e); }

mlchung · 2021-08-19T00:33:21Z

src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties

@@ -87,6 +87,9 @@ main.opt.ignore-signing-information=\
 \                                        signed modular JARs are not copied to\n\
 \                                        the runtime image.

+main.opt.generate-cds-archive=\


This should not be needed. Leftover from an early version.

mlchung

Thanks for the change. Looks okay.

mlchung

Thanks for creating a JBS issue. I'm okay to follow up the test improvement as a separate issue.

openjdk · 2021-08-24T02:17:32Z

@calvinccheung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264322: Generate CDS archive when creating custom JDK image

Reviewed-by: mchung, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 37 new commits pushed to the master branch:

7f80683: 8272783: Epsilon: Refactor tests to improve performance
22ef4f0: 5015261: NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame
9bc0232: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux
2ff4c01: 8271600: C2: CheckCastPP which should closely follow Allocate is sunk of a loop
ad92033: 8272736: [JVMCI] Add API for reading and writing JVMCI thread locals
709b591: 8272553: several hotspot runtime/CommandLine tests don't check exit code
1884072: 8265253: javac -Xdoclint:all gives "no comment" warning for code that can't be commented
594e516: 8272778: Consolidate is_instance and is_instance_inlined in java_lang_String
d542745: 8267894: Skip work for empty regions in G1 Full GC
741f58c: 8272417: ZGC: fastdebug build crashes when printing ClassLoaderData
... and 27 more: https://git.openjdk.java.net/jdk/compare/73da66ffb707abf6dc38368a12e337d52597de25...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

AlanBateman

This looks okay, and I see a follow-on issue has been created for the test improvements.

calvinccheung · 2021-08-24T15:29:35Z

@mlchung, @AlanBateman Thanks for the review and discussions.

calvinccheung · 2021-08-24T15:30:31Z

/integrate

openjdk · 2021-08-24T15:31:31Z

Going to push as commit f608e81.
Since your change was applied there have been 44 commits pushed to the master branch:

f681d65: 8272806: [macOS] "Apple AWT Internal Exception" when input method is changed
6e0328f: 8272725: G1: add documentation on needs_remset_update_t vs bool
2309b7d: 8253178: Replace LinkedList Impl in net.http.FilterFactory
94f5e44: 8271258: @param with non-ascii variable names produces incorrect results
7454306: 8272526: Cleanup ThreadStateTransition class
0597cde: 8221360: Eliminate Shared_DirtyCardQ_lock
928b972: 8271930: Simplify end_card calculation in G1BlockOffsetTablePart::verify
7f80683: 8272783: Epsilon: Refactor tests to improve performance
22ef4f0: 5015261: NPE may be thrown if JDesktopIcon is set to null on a JInternalFrame
9bc0232: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux
... and 34 more: https://git.openjdk.java.net/jdk/compare/73da66ffb707abf6dc38368a12e337d52597de25...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-24T15:31:44Z

@calvinccheung Pushed as commit f608e81.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8264322: Generate CDS archive when creating custom JDK image

17087e9

openjdk bot added the core-libs core-libs-dev@openjdk.org label Aug 18, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 18, 2021

fix trailing whitespace

918d506

calvinccheung marked this pull request as ready for review August 18, 2021 21:27

openjdk bot added the rfr Pull request is ready for review label Aug 18, 2021

mlchung reviewed Aug 19, 2021

View reviewed changes

@mlchung comments

b7a5ec6

mlchung reviewed Aug 20, 2021

View reviewed changes

mlchung approved these changes Aug 24, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 24, 2021

AlanBateman approved these changes Aug 24, 2021

View reviewed changes

openjdk bot closed this Aug 24, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 24, 2021

calvinccheung deleted the 8264322-jlink-cds-archive branch August 24, 2021 15:46

	* @return Platform
	* @return {@code Platform} object representing the platform of the runtime image

	\ Applicable to JDK images built with the CDS feature.
	\ --generate-cds-archive Generate CDS archives if the runtime image supports CDS feature.\n\

8264322: Generate CDS archive when creating custom JDK image #5174

8264322: Generate CDS archive when creating custom JDK image #5174

Conversation

calvinccheung commented Aug 18, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 18, 2021

calvinccheung commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

mlbridge bot commented Aug 18, 2021 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 24, 2021 • edited Loading

AlanBateman left a comment

Choose a reason for hiding this comment

calvinccheung commented Aug 24, 2021

calvinccheung commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

openjdk bot commented Aug 24, 2021

calvinccheung commented Aug 18, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 18, 2021 •

edited

Loading

openjdk bot commented Aug 24, 2021 •

edited

Loading