Skip to content

8311302: Implement JEP 493: Linking Run-Time Images without JMODs #14787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 188 commits into from

Conversation

jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Jul 6, 2023

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.jmod            java.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.jmod        java.prefs.jmod          java.transaction.xa.jmod  jdk.dynalink.jmod          jdk.internal.vm.ci.jmod                   jdk.jdwp.agent.jmod  jdk.management.jfr.jmod    jdk.security.jgss.jmod
java.datatransfer.jmod    java.rmi.jmod            java.xml.crypto.jmod      jdk.editpad.jmod           jdk.internal.vm.compiler.jmod             jdk.jfr.jmod         jdk.management.jmod        jdk.unsupported.desktop.jmod
java.desktop.jmod         java.scripting.jmod      java.xml.jmod             jdk.hotspot.agent.jmod     jdk.internal.vm.compiler.management.jmod  jdk.jlink.jmod       jdk.naming.dns.jmod        jdk.unsupported.jmod
java.instrument.jmod      java.security.jgss.jmod  jdk.accessibility.jmod    jdk.httpserver.jmod        jdk.jartool.jmod                          jdk.jpackage.jmod    jdk.naming.rmi.jmod        jdk.xml.dom.jmod
java.logging.jmod         java.security.sasl.jmod  jdk.attach.jmod           jdk.incubator.vector.jmod  jdk.javadoc.jmod                          jdk.jshell.jmod      jdk.net.jmod               jdk.zipfs.jmod
java.management.jmod      java.se.jmod             jdk.charsets.jmod         jdk.internal.ed.jmod       jdk.jcmd.jmod                             jdk.jsobject.jmod    jdk.nio.mapmode.jmod
java.management.rmi.jmod  java.smartcardio.jmod    jdk.compiler.jmod         jdk.internal.jvmstat.jmod  jdk.jconsole.jmod                         jdk.jstatd.jmod      jdk.random.jmod
java.naming.jmod          java.sql.jmod            jdk.crypto.cryptoki.jmod  jdk.internal.le.jmod       jdk.jdeps.jmod                            jdk.localedata.jmod  jdk.sctp.jmod
$ ls jmods
ls: cannot access 'jmods': No such file or directory
$ ./bin/jlink --version
22-internal
$ ./bin/jlink --add-modules java.se --output ../java.se-runtime --verbose
java.base jrt:/java.base (run-image)
java.compiler jrt:/java.compiler (run-image)
java.datatransfer jrt:/java.datatransfer (run-image)
java.desktop jrt:/java.desktop (run-image)
java.instrument jrt:/java.instrument (run-image)
java.logging jrt:/java.logging (run-image)
java.management jrt:/java.management (run-image)
java.management.rmi jrt:/java.management.rmi (run-image)
java.naming jrt:/java.naming (run-image)
java.net.http jrt:/java.net.http (run-image)
java.prefs jrt:/java.prefs (run-image)
java.rmi jrt:/java.rmi (run-image)
java.scripting jrt:/java.scripting (run-image)
java.se jrt:/java.se (run-image)
java.security.jgss jrt:/java.security.jgss (run-image)
java.security.sasl jrt:/java.security.sasl (run-image)
java.sql jrt:/java.sql (run-image)
java.sql.rowset jrt:/java.sql.rowset (run-image)
java.transaction.xa jrt:/java.transaction.xa (run-image)
java.xml jrt:/java.xml (run-image)
java.xml.crypto jrt:/java.xml.crypto (run-image)

Providers:
  java.desktop provides java.net.ContentHandlerFactory used by java.base
  java.base provides java.nio.file.spi.FileSystemProvider used by java.base
  java.naming provides java.security.Provider used by java.base
  java.security.jgss provides java.security.Provider used by java.base
  java.security.sasl provides java.security.Provider used by java.base
  java.xml.crypto provides java.security.Provider used by java.base
  java.base provides java.util.random.RandomGenerator used by java.base
  java.management.rmi provides javax.management.remote.JMXConnectorProvider used by java.management
  java.management.rmi provides javax.management.remote.JMXConnectorServerProvider used by java.management
  java.desktop provides javax.print.PrintServiceLookup used by java.desktop
  java.desktop provides javax.print.StreamPrintServiceFactory used by java.desktop
  java.management provides javax.security.auth.spi.LoginModule used by java.base
  java.desktop provides javax.sound.midi.spi.MidiDeviceProvider used by java.desktop
  java.desktop provides javax.sound.midi.spi.MidiFileReader used by java.desktop
  java.desktop provides javax.sound.midi.spi.MidiFileWriter used by java.desktop
  java.desktop provides javax.sound.midi.spi.SoundbankReader used by java.desktop
  java.desktop provides javax.sound.sampled.spi.AudioFileReader used by java.desktop
  java.desktop provides javax.sound.sampled.spi.AudioFileWriter used by java.desktop
  java.desktop provides javax.sound.sampled.spi.FormatConversionProvider used by java.desktop
  java.desktop provides javax.sound.sampled.spi.MixerProvider used by java.desktop
  java.logging provides jdk.internal.logger.DefaultLoggerFinder used by java.base
  java.desktop provides sun.datatransfer.DesktopDatatransferService used by java.datatransfer

One nice property of this patch is that it can produce an identical (as in binary identical) java.se JDK image in run-time image based link mode as compared to a regular jlink with packaged modules present. This has been asserted in newly added tests. In order to prevent accidental copy of modified files in the base JDK a checksum mechanism is in place to fail the link if a runtime image file has been modified. This is also asserted in tests.

One limitation of this mode is that there is no way to use it for cross-linking (at least currently). That is, a run-time image based jlink needs to happen on the same runtime platform the resulting image is intended to get deployed on.

Testing:

  • GHA (including jdk/tools/jlink tests and JDKs built with --enable-linkable-runtime). See for example the latest run and here.
  • GHA (including jdk/tools/jlink tests and JDKs built with --enable-linkable-runtime --enable-keep-packaged-modules). See for example the latest run and here.
  • GHA, including jdk/tools/jlink tests for the default build (--disable-linkable-runtime), which also runs the tests in runtimeImage sub-folder here by first creating a run-time image link capable JDK including jdk.jlink module.
  • Some tests on our internal infra. It didn't show any regressions.
  • Added tests as part of the patch. All pass on the major platforms.

Thoughts?


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires a JEP request to be targeted
  • Change requires CSR request JDK-8317420 to be approved
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

  • JDK-8311302: Implement JEP 493: Linking Run-Time Images without JMODs (Enhancement - P4)
  • JDK-8333799: JEP 493: Linking Run-Time Images without JMODs (JEP)
  • JDK-8317420: Implement JEP 493: Linking Run-Time Images without JMODs (CSR)

Reviewers

Contributors

  • Mandy Chung <mchung@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787
$ git checkout pull/14787

Update a local copy of the PR:
$ git checkout pull/14787
$ git pull https://git.openjdk.org/jdk.git pull/14787/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14787

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14787.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 6, 2023

👋 Welcome back sgehwolf! 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.

@openjdk
Copy link

openjdk bot commented Jul 6, 2023

@jerboaa The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 6, 2023
@jerboaa jerboaa marked this pull request as ready for review July 7, 2023 08:34
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 7, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Aug 4, 2023

@jerboaa This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@jerboaa
Copy link
Contributor Author

jerboaa commented Aug 7, 2023

Please keep open, bot.

@jerboaa jerboaa force-pushed the jdk-8311302-jmodless-link branch from a001d8c to baeaaf5 Compare August 8, 2023 16:29
@openjdk
Copy link

openjdk bot commented Aug 8, 2023

@jerboaa Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

This is configurable so add tests for both scenarios.
This uses a stamp file in 'lib/modules' jimage in order to recognize
multi-hop links. However, this has the caveat of no longer producing
reproducible builds as compared to a packaged module-based link.

Add an option to allow for multi-hop, which disables the stamp file
addition and makes it reproducible again.
@jerboaa
Copy link
Contributor Author

jerboaa commented Aug 10, 2023

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Aug 10, 2023
@openjdk
Copy link

openjdk bot commented Aug 10, 2023

@jerboaa has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jerboaa please create a CSR request for issue JDK-8311302 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@jerboaa
Copy link
Contributor Author

jerboaa commented Aug 10, 2023

@AlanBateman Hi! Moving the discusion to this PR now. I've updated this patch to do single-hops only by default now. Looks like this:

$ bin/jlink --add-modules java.base --output ../testme-java-base/ --verbose
java.base jrt:/java.base (run-image)

Providers:
  java.base provides java.nio.file.spi.FileSystemProvider used by java.base
  java.base provides java.util.random.RandomGenerator used by java.base
Error: Run image links only allow single-hop.

I'm not particularly fond of that, though, as there is a need to know what is single hop is and what not. My approach is adding a 0-sized stamp file to the lib/modules jimage, but that has the issue of now breaking comparison between a packaged module jlink vs. a runtime image using one. The same would be true for adding a file or appending the jmod_resources file with some info to know it's multi-hop. Some state needs to be there unless I'm missing something for this. For now an option suppresses creating that file so as to be able to keep asserting packaged vs run-image link and other properties in tests. I'd argue most of the time multi-hop would be OK, but default off and a switch to turn it on seems reasonable to me. Up for discussion...

Also, if a file is modified the link fails unless it's turned off with an option. Looks like this:

$ ./bin/jlink --add-modules java.base --output ../abort-on-mod --verbose
java.base jrt:/java.base (run-image)

Providers:
  java.base provides java.nio.file.spi.FileSystemProvider used by java.base
  java.base provides java.util.random.RandomGenerator used by java.base
Error: java.lang.IllegalArgumentException: /disk/openjdk/upstream-sources/git/jdk-jdk/build/jmodless-copy-jmods-removed/conf/net.properties has been modified. Please double check!
$ echo $?
1

Both things will have CLI options to a) make the exit on mod a warning only and b) allow multi-hop - some tests needed that. Hence, I've marked this PR as needing a CSR for now.

More thoughts?

@jerboaa
Copy link
Contributor Author

jerboaa commented Aug 29, 2023

@AlanBateman Gentle ping.

@AlanBateman
Copy link
Contributor

@AlanBateman Gentle ping.

On my list, it's a lot to get through and a number of aspects to this that I think will require refinement and discussion.

@jerboaa
Copy link
Contributor Author

jerboaa commented Aug 30, 2023

@AlanBateman Gentle ping.

On my list, it's a lot to get through and a number of aspects to this that I think will require refinement and discussion.

Thanks for the heads-up! Your input is much appreciated.

@AlanBateman
Copy link
Contributor

AlanBateman commented Sep 19, 2023

I did a pass over this to see where this proposal is currently at. At a high-level I think good progress since the discussion on leyden-dev some time ago. A few comments this from this pass:

If I read it correctly, the default behavior is now to fail if jlink is executed from a run-time image that has been modified from the original and the packaged modules are not observable on the module path (this includes the default module path effectively appended by jlink). That seems the right policy as it avoids modifications by plugins or conf changes silently propagating.

If I read the code correctly, the error when a hash doesn't match is a very terse "Run image links only allow single-hop" so that probably needs to be looked to make sure the message says that the run-time image has been modified and maybe include one of the files/resources that has changed so there is something real to go with the error.

The command line options to override and change the error to a warning or silently ignore seems to be "-run-time-ignore-single-hop" and "--run-image-only-warnings". Maybe it should be reduced to just one option and maybe it should contain the word "clone" to something that makes it more obvious that it's just copying or cloning the contents of the modules in the run-time image (I suspect "single hop" won't be understood).

Adding a new jdk.tools.jlink.internal.Archive implementation where the resources come from the run-time image seems a sensible approach. I don't particularly like the name "JmodLessArchive" because the original module may not have been packaged as a JMOD file, it may have been a modular JAR. Same comment on the BOM file "jmod_resources" added to the top-level directory of each module. I think it's clever to add this to each module in the container image but part of me wonders if it should be hidden in some way to avoid tools depending on it. I don't have a concrete suggestion except to wonder if jrtfs should filter it out, same thing in the SystemModuleReader code as ModuleReader.find("jmod_resources") will find the resource, could be an attractive nuisance.

@jerboaa
Copy link
Contributor Author

jerboaa commented Sep 29, 2023

I did a pass over this to see where this proposal is currently at. At a high-level I think good progress since the discussion on leyden-dev some time ago. A few comments this from this pass:

Thanks, Alan!

If I read it correctly, the default behavior is now to fail if jlink is executed from a run-time image that has been modified from the original and the packaged modules are not observable on the module path (this includes the default module path effectively appended by jlink). That seems the right policy as it avoids modifications by plugins or conf changes silently propagating.

Correct.

If I read the code correctly, the error when a hash doesn't match is a very terse "Run image links only allow single-hop" so that probably needs to be looked to make sure the message says that the run-time image has been modified and maybe include one of the files/resources that has changed so there is something real to go with the error.

Not quite. See the example I've added in the previous comment. It already includes that info. It looks like this:

Error: java.lang.IllegalArgumentException: /disk/openjdk/upstream-sources/git/jdk-jdk/build/jmodless-copy-jmods-removed/conf/net.properties has been modified. Please double check!

The message can of course be modified what we think is best.

The command line options to override and change the error to a warning or silently ignore seems to be "-run-time-ignore-single-hop" and "--run-image-only-warnings". Maybe it should be reduced to just one option and maybe it should contain the word "clone" to something that makes it more obvious that it's just copying or cloning the contents of the modules in the run-time image (I suspect "single hop" won't be understood).

I believe keeping them both makes sense. Or at least make it so that it takes an argument to be able to distinguish between the two. The reason why I think this is useful is in the container case, where say - the JDK got installed by an installer - which didn't include packaged modules, but is otherwise a full JDK (with all modules). The installer already does integrity checking of the files and it might be desirable to do multi-hop evolutions, while still wanting to get warnings/errors on modified configuration files, for example. Happy to change the name of that flag/flags to --clone-run-image=ignore-multi,warn-only or something similar.

Adding a new jdk.tools.jlink.internal.Archive implementation where the resources come from the run-time image seems a sensible approach. I don't particularly like the name "JmodLessArchive" because the original module may not have been packaged as a JMOD file, it may have been a modular JAR. Same comment on the BOM file "jmod_resources" added to the top-level directory of each module.

OK. How about RunimageArchive and runimage_resources or clone_resources?

I think it's clever to add this to each module in the container image but part of me wonders if it should be hidden in some way to avoid tools depending on it. I don't have a concrete suggestion except to wonder if jrtfs should filter it out, same thing in the SystemModuleReader code as ModuleReader.find("jmod_resources") will find the resource, could be an attractive nuisance.

That seems fine. I can add code to filter those resources out. On the other hand, no special handling is being made for jdk/internal/vm/options. Perhaps that should be handled uniformly?

The test doesn't need the default module path and default jmods
generated from the helper. Using this approach makes it work for
linkable run-time images as well.
@AlanBateman
Copy link
Contributor

Therefore, paging @AlanBateman @mlchung @mbreinhold @magicus for their final thoughts. Thank you!

ACK. I'll do another pass on the changes in the PR.

Similar to GenerateJLIClassesPluginTest.java, the default module path
doesn't need to be explicitly specified.
@jerboaa
Copy link
Contributor Author

jerboaa commented Nov 7, 2024

For the test changes, I believe multiple @test tags to verify a JDK of linkable run-time image and another JDK with packaged modules are not necessary. Each test execution can only verify one JDK. The tests can keep one single @test and the main method has to determine the test JDK configuration for test verification.

Done in the latest update. Thanks again for the review!

@jerboaa
Copy link
Contributor Author

jerboaa commented Nov 7, 2024

GHA failure on aarch64 MacOSX is infra related.

@jerboaa
Copy link
Contributor Author

jerboaa commented Nov 8, 2024

GHA failure of MacOSX on aarch64 is still infra related.

@AlanBateman
Copy link
Contributor

I'm worked through the make + src changes and don't have any issues. I skimmed tests some didn't study closely. You will need to bump the copyright header of several jlink src files before integrating.

@jerboaa
Copy link
Contributor Author

jerboaa commented Nov 8, 2024

Thanks for the review!

You will need to bump the copyright header of several jlink src files before integrating.

OK. Let me work on that.

@mlchung
Copy link
Member

mlchung commented Nov 9, 2024

Thanks for the update and skimmed through the tests. Looks good.

I also agree that better to fail early when detecting patched modules as described in JDK-8343839.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 9, 2024
@jerboaa
Copy link
Contributor Author

jerboaa commented Nov 11, 2024

Thanks for the reviews! I'll integrate this later today.

@jerboaa
Copy link
Contributor Author

jerboaa commented Nov 11, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Nov 11, 2024

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

  • f3ba767: 8343535: IGV: Colorize nodes on demand
  • 5016132: 8343838: Test EmptyDomainNotificationTest.java fails with ListenerNotFoundException
  • 36e1295: 8343929: Remove PreservedMarksSet::createTask() after JDK-8305895
  • b1a9491: 8343321: Bad verify in LockStack::oops_do()
  • cbe8448: 8268895: Do not filter out man pages from build
  • ec13364: 8343067: C2: revisit constant-offset AddP chains after successful input idealizations
  • 5ca6698: 8341176: Permit access to diagnostics for transient snippets
  • a93bd9d: 8343810: [s390x] is_uimm* methods should take unsigned arguments
  • f12c370: 8343118: [TESTBUG] java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.java fails with rror. Can't find HTML file PrintCheckboxManualTest.html
  • ae6bb3c: 8343824: Remove unused InstructionFlags in C1
  • ... and 60 more: https://git.openjdk.org/jdk/compare/83f3d42d6bcefac80449987f4d951f8280eeee3a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 11, 2024
@openjdk openjdk bot closed this Nov 11, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 11, 2024
@openjdk
Copy link

openjdk bot commented Nov 11, 2024

@jerboaa Pushed as commit 2ec3580.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

7 participants