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

[Mandy Chung]

On Wed, 18 Aug 2021 21:21:06 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

> 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](https://bugs.openjdk.java.net/browse/JDK-8269178).
> 
> Testing:
> 
> - [x] tiers 1,2 (including the new test)

src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java line 89:

> 87:         private final List<String> args;
> 88:         private final Set<String> modules;
> 89:         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.

src/jdk.jlink/share/classes/jdk/tools/jlink/builder/DefaultImageBuilder.java line 134:

> 132: 
> 133:         @Override
> 134:         public void setTargetPlatform(Platform p) {

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

src/jdk.jlink/share/classes/jdk/tools/jlink/builder/ImageBuilder.java line 82:

> 80:      * Gets the target image platform.
> 81:      *
> 82:      * @return Platform

Suggestion:

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

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 70:

> 68:      * @param p
> 69:      */
> 70:     public void setTargetPlatform(Platform p);

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

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 75:

> 73:      * The Platform of the image.
> 74:      *
> 75:      * @return Platform

Suggestion:

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

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 82:

> 80:      * Checks if the image is 64-bit.
> 81:      *
> 82:      * @return boolean

Suggestion:

     * @return true if it's a 64-bit platform

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ExecutableImage.java line 84:

> 82:      * @return boolean
> 83:      */
> 84:     public boolean is64Bit();

This `is64Bit` method should belong to `Platform` class

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImagePluginStack.java line 195:

> 193:     public void operate(ImageProvider provider) throws Exception {
> 194:         ExecutableImage img = provider.retrieve(this);
> 195:         img.setTargetPlatform(imageBuilder.getTargetPlatform());

See above comment - the ExecutableImage should know the platform when it's constructed.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java line 55:

> 53:      * Returns the {@code Platform} based on the platformString of the form <operating system>-<arch>.
> 54:      */
> 55:     public static Platform parseTargetPlatform(String platformString) {

This method can be renamed back to `parsePlatform` (an earlier version).    My suggestion to name this method as `parseTargetPlatform` no longer apply in this version.

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

> 78:             status = p.waitFor();
> 79:         } catch (Exception ex) {
> 80:             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);
 }

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

> 81:         }
> 82:         if (status == 0) {
> 83:             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

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

> 119:     @Override
> 120:     public ResourcePool transform(ResourcePool in, ResourcePoolBuilder out) {
> 121:         in.transformAndCopy((file) -> {

This method should not be called in a post-processor.    Plugin API needs some re-thinking to support post-processor plugin.   As `Plugin::transform` is abstract method, for now this method should simply throw `UnsupportedOperationException`.

src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 90:

> 88: \                                        the runtime image.
> 89: 
> 90: main.opt.generate-cds-archive=\

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

src/jdk.jlink/share/classes/jdk/tools/jlink/resources/plugins.properties line 143:

> 141: 
> 142: generate-cds-archive.description=\
> 143: 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?

src/jdk.jlink/share/classes/jdk/tools/jlink/resources/plugins.properties line 148:

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

Suggestion:

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


Does this match what you want to say?

test/jdk/tools/jlink/plugins/CDSPluginTest.java line 66:

> 64:         String sep = File.separator;
> 65:         String osName = System.getProperty("os.name");
> 66:         if (System.getProperty("os.name").startsWith("Windows")) {

Since this test already depends on `jdk.tools.jlink.internal`, maybe it can use `Platform::runtime` to get the runtime platform.

test/jdk/tools/jlink/plugins/CDSPluginTest.java line 80:

> 78:            String jlinkPath = JDKToolFinder.getJDKTool("jlink");
> 79:            String[] cmd = {jlinkPath, "--add-modules", "java.base,java.logging",
> 80:                            "-J-Dos.name=windows", "--generate-cds-archive",

Is there a better way of setting `os.name` system property on the command line?

Maybe override `ModuleTarget` attribute in module-info.class of java.base to be a different platform?

-------------

PR: https://git.openjdk.java.net/jdk/pull/5174