Skip to content
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

8293532: Use lighter jmod compression levels in build config #10214

Closed

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Sep 8, 2022

As follow-up to JDK-8293499, JDK build can stick to lighter jmod compression levels to gain build performance.

# ----- Baseline
$ time CONF=linux-x86_64-server-release make clean-images images
real	0m10.762s
user	1m5.157s
sys	0m12.370s

$ du -sk build/linux-x86_64-server-release/images/jdk/jmods/
81480	build/linux-x86_64-server-release/images/jdk/jmods/

$ time CONF=linux-x86_64-server-release make clean-images images
real	0m9.574s
user	1m2.644s
sys	0m12.143s

$ du -sk build/linux-x86_64-server-release/images/jdk/jmods/
85744	build/linux-x86_64-server-release/images/jdk/jmods/

This gets even more substantial if JVM is configured with --with-native-debug-symbols=internal, in which case java.base includes a much larger libjvm.so:

# Baseline
$ time CONF=linux-x86_64-server-release make clean-images images
real	0m36.617s
user	1m30.216s
sys	0m11.692s

$ du -sk build/linux-x86_64-server-release/images/jdk/jmods/
295428	build/linux-x86_64-server-release/images/jdk/jmods/

# Patched
$ time CONF=linux-x86_64-server-release make clean-images images
real	0m22.183s
user	1m13.841s
sys	0m11.657s

$ du -sk build/linux-x86_64-server-release/images/jdk/jmods/
316856	build/linux-x86_64-server-release/images/jdk/jmods/

Users can use --with-jmod-compress=zip-6 to get the original JMOD compression level back. Or, they can give up on compression altogether using --with-jmod-compress=zip-0, and then reap even more time benefits:

$ time CONF=linux-x86_64-server-release make clean-images images
real	0m6.411s
user	0m56.145s
sys	0m12.278s

$ du -sk build/linux-x86_64-server-release/images/jdk/jmods/
183752	build/linux-x86_64-server-release/images/jdk/jmods/

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8293532: Use lighter jmod compression levels in build config

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10214

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 8, 2022

👋 Welcome back shade! A progress list of the required criteria for merging this PR into pr/10213 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 Sep 8, 2022

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

  • build

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 build build-dev@openjdk.org label Sep 8, 2022
@shipilev shipilev force-pushed the JDK-8293532-jmod-build-lighter branch from cfe88a6 to 923c6d5 Compare September 12, 2022 18:40
@openjdk-notifier openjdk-notifier bot changed the base branch from pr/10213 to master September 19, 2022 06:22
@openjdk-notifier
Copy link

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8293532-jmod-build-lighter
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

openjdk bot commented Sep 19, 2022

@shipilev this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8293532-jmod-build-lighter
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 19, 2022
@shipilev shipilev force-pushed the JDK-8293532-jmod-build-lighter branch from 923c6d5 to 4f29501 Compare September 19, 2022 08:48
@shipilev shipilev marked this pull request as ready for review September 19, 2022 08:49
@openjdk openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Sep 19, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 19, 2022

Webrevs

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of being able to change the jmod compression is good. I'm torn about what a good default is. Maybe keeping a higher compression on release builds would make sense? I'm not sure so would like to hear if there are more opinions on it. There is little point spending time compressing jmods on debug builds at least.

make/autoconf/jdk-options.m4 Outdated Show resolved Hide resolved
@magicus
Copy link
Member

magicus commented Sep 19, 2022

I fully agree with Erik. The idea is good, but what is a suitable default? The patch, as it stands, requires all distributors to update their build scripts to get the same output as they did before the patch. So if we set the default to 6 instead (iiuc), we will get the same default behavior, but we can get a speedup by passing an argument to configure.

Normally, my stance would be that configure with few arguments should setup a good developer experience, and that CI systems can afford to add extra flags. In principle I think that should apply here too, but I'm slightly more worried that we might not be able to reach out to everyone affected.

Also, if we do end up keeping the default as currently suggested in this PR, we need at least to update the jib-profiles.conf file.

@AlanBateman
Copy link
Contributor

I think the only mildly controversial part is the default used when creating the packaged modules for the release build. Adding a configure option, creating the interim JMODs with --compress zip-0, and changing the default for debug releases might be okay.

@magicus
Copy link
Member

magicus commented Sep 20, 2022

Alans suggestion seems the safest route for now.

We can always come back later and change the default. If you go down this route, please add the new configure option to the GHA files to speed up GHA release builds, as these are not distributed.

@openjdk-notifier
Copy link

@shipilev Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@shipilev
Copy link
Member Author

shipilev commented Sep 20, 2022

All right then. See new commits:

  • release builds with zip-6 (default) -- still improves a bit, because interim jmods are not compressed
  • fastdebug builds with zip-1 -- favoring speed over disk size
  • GHA release/fastdebug build with zip-1 -- to gain build perf without incurring too much network/storage overhead;

Sample improvements for make clean-images images:

# Linux x86_64 release

# Baseline
real	0m10.583s
user	1m3.106s
sys	0m11.806s

# Patched
real	0m10.430s
user	1m2.314s
sys	0m11.678s


# Linux x86_64 fastdebug

# Baseline
real	0m17.388s
user	2m47.504s
sys	0m17.616s

# Patched
real	0m15.713s
user	2m41.490s
sys	0m20.843s

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it! :)

@openjdk
Copy link

openjdk bot commented Sep 20, 2022

@shipilev 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:

8293532: Use lighter jmod compression levels in build config

Reviewed-by: ihse

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 30 new commits pushed to the master branch:

  • cddd6de: 8279941: sun/security/pkcs11/Signature/TestDSAKeyLength.java fails when NSS version detection fails
  • 21008ca: 8285383: vmTestbase/nsk/jvmti/scenarios/hotswap/HS204/hs204t001/hs204t001.java failed with "exit code: 96"
  • 3b438a6: 8294067: [macOS] javax/swing/JComboBox/6559152/bug6559152.java Cannot select an item from popup with the ENTER key.
  • caae53f: 8289508: Improve test coverage for XPath Axes: ancestor, ancestor-or-self, preceding, and preceding-sibling
  • cb5771d: 8294006: Avoid hardcoding object file suffixes in make
  • 5002eaa: 8293828: JFR: jfr/event/oldobject/TestClassLoaderLeak.java still fails when GC cycles are not happening
  • e3358e7: 8294008: Grapheme implementation of setText() throws IndexOutOfBoundsException
  • df8ec09: 8294046: Newly added test test/jdk/javax/swing/JTabbedPane/TestNPEStateChgListener.java fails in macos
  • 09af637: 8294012: RISC-V: get/put_native_u8 missing the case when address&7 is 6
  • 584de68: 8294058: Early use of lambda introduced in JDK-8285263 cause startup regressions in 20-b02
  • ... and 20 more: https://git.openjdk.org/jdk/compare/36c9034ff1274f37969550a3f9239f1bb16a0b25...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 20, 2022
@shipilev
Copy link
Member Author

Ship it! :)

Aye, Capt'n!

Any more opinions on this, or we are done?

@shipilev
Copy link
Member Author

Anyone else?

@magicus
Copy link
Member

magicus commented Sep 23, 2022

I think you're fine. The only controversial part was the default, which is now restored to what is was before.

@shipilev
Copy link
Member Author

All right then!

/integrate

@openjdk
Copy link

openjdk bot commented Sep 23, 2022

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

  • eec992c: 8292602: ZGC: C2 late barrier analysis uses invalid dominator information
  • f6d78cd: 8293657: sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 failed with "SSLHandshakeException: Remote host terminated the handshake"
  • a4dc035: 8290910: Wrong memory state is picked in SuperWord::co_locate_pack()
  • f3ba332: 8294183: AArch64: Wrong macro check in SharedRuntime::generate_deopt_blob
  • df53fa7: 8292328: AccessibleActionsTest.java test instruction for show popup on JLabel did not specify shift key
  • 5285035: 8294075: gtest/AsyncLogGtest crashes with SEGV
  • 696287d: 8294037: Using alias template to unify hashtables in AsyncLogWriter
  • 48cc156: 8293331: Refactor FileDispatcherImpl into operating system-specific components
  • f751e60: 8294197: Zero: JVM_handle_linux_signal should not assume deopt NOPs
  • 4a6060b: 8294190: Incorrect check messages in SharedRuntime::generate_uncommon_trap_blob
  • ... and 63 more: https://git.openjdk.org/jdk/compare/36c9034ff1274f37969550a3f9239f1bb16a0b25...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Sep 23, 2022

@shipilev Pushed as commit 05c8cab.

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

@shipilev shipilev deleted the JDK-8293532-jmod-build-lighter branch October 21, 2022 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org integrated Pull request has been integrated
4 participants