8370438: Offer link time optimization support on library level

[MBaesken]

We currently have support for LTO (link time optimization) for Hotspot/libjvm, that can be enabled as a JVM feature.
But for other JDK native libs, we do not have support for this feature.
LTO and sometimes lead to faster and also in some cases smaller binaries, so support for this might be interesting also for other libs and not only libjvm.

---

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-8370438: Offer link time optimization support on library level (Enhancement - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27976

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mbaesken! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@MBaesken The following labels will be automatically applied to this pull request:

• build
• client

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

[erikj79]

I notice that the compiler args for GCC and Clang are different, but the linker args are the same. Just want to make sure that's intentional.

[MBaesken]

Yes they are different, this is 'borrowed' from Hotspot flags, see

jdk/make/hotspot/lib/JvmFeatures.gmk

Line 178 in 9625993

ifeq ($(call isCompiler, gcc), true)

where we supported LTO for some time.

[TheShermanTanker]

It might be better if we offered this as an option to turn off and on through configure (My personal fork of the JDK has something along the lines of --enable-linktime-opt, can't remember the exact name) rather than making some native code have to use LTO. Additionally I also think this is better done similar to how HotSpot does it instead of adding LTO options as an exported Makefile variable and adding a new parameter to SetupNativeCompilation.

[MBaesken]

It might be better if we offered this as an option to turn off and on through configure (My personal fork of the JDK has something along the lines of --enable-linktime-opt, can't remember the exact name) rather than making some native code have to use LTO. Additionally I also think this is better done similar to how HotSpot does it instead of adding LTO options as an exported Makefile variable and adding a new parameter to SetupNativeCompilation.

The PR gives developers working on some lib , where LTO looks promising, the ability to test the feature on the specific lib.
Enabling it for ALL jdk libs is of course possible too (I did this for experiments locally as well); the lib idea was discussed here
#22464 (comment)

[MBaesken]

the lib idea was discussed here
#22464 (comment)

@mrserb is this what you had in mind ?

[MBaesken]

I want to remove the changes from make/modules/java.desktop/lib/ClientLibraries.gmk because this change is not about changing the settings for single libs, just for offering the LTO support to the lib developers/maintainers.

[mlbridge]

Webrevs

• 08: Full - Incremental (bea8d07a)
• 07: Full - Incremental (a5e266e0)
• 06: Full - Incremental (e68bc711)
• 05: Full - Incremental (75f83437)
• 04: Full - Incremental (72ba9ed1)
• 03: Full - Incremental (6adc512d)
• 02: Full - Incremental (de4dbf2b)
• 01: Full - Incremental (b54cbed6)
• 00: Full (ab71b582)

[MBaesken]

For libfontmanager the lib sizes decrease quite a lot on most platforms if LTO is enabled in the build of the lib.
libfontmanager.so / dylib / dll size without/with LTO enabled

38M	 / 17M   aix_ppc64
1.8M / 1.1M  linux_aarch64
2.0M / 1.3M  linux_alpine_x86_64
2.3M / 1.4M  linux_ppc64le
1.8M / 1.2M  linux_x86_64

1.4M / 900K   macos aarch64
1.4M / 952K   macos x86_64

932K / 916K   windows x86_64

(however the freetype lib does not show this decrease in lib size when enabling lto)

[erikj79]

This change on its own doesn't do anything. Do you plan on following up immediately with a set of suggested libs to enable this on? Have you noticed any impact on build performance when enabling this on any JDK libs? I very much doubt any component owner is going to test and apply this on their own. Without that, this is a dead feature and I wouldn't want it to go in.

I wonder if it would be a good idea to add a global on/off switch (configure arg) for this whole thing, I'm really not sure. It kind of depends on what the impact is. If the impact is unknown, then a global on/off switch, default off, would let every distributor evaluate this on their own. That would of course also require that at least some libs have this enabled.

[TheShermanTanker]

This change on its own doesn't do anything. Do you plan on following up immediately with a set of suggested libs to enable this on? Have you noticed any impact on build performance when enabling this on any JDK libs? I very much doubt any component owner is going to test and apply this on their own. Without that, this is a dead feature and I wouldn't want it to go in.

I wonder if it would be a good idea to add a global on/off switch (configure arg) for this whole thing, I'm really not sure. It kind of depends on what the impact is. If the impact is unknown, then a global on/off switch, default off, would let every distributor evaluate this on their own. That would of course also require that at least some libs have this enabled.

I was suggesting a configure switch too, in my fork I have a UTIL_ARG_ENABLE for the configure option and then in LibCommon.gmk and LauncherCommon.gmk I implemented the options as such:

https://github.com/TheShermanTanker/jdk/blob/e3d33aee0270d2ce674367f787bdea77a7ad0c58/make/common/modules/LibCommon.gmk#L33

[MBaesken]

This change on its own doesn't do anything. Do you plan on following up immediately with a set of suggested libs to enable this on? Have you noticed any impact on build performance when enabling this on any JDK libs? I very much doubt any component owner is going to test and apply this on their own. Without that, this is a dead feature and I wouldn't want it to go in.

I wonder if it would be a good idea to add a global on/off switch (configure arg) for this whole thing, I'm really not sure. It kind of depends on what the impact is. If the impact is unknown, then a global on/off switch, default off, would let every distributor evaluate this on their own. That would of course also require that at least some libs have this enabled.

There was some interest on the lib-level solution here #22464 (comment) . 'Provide an option for library owners to opt-in, which can be enabled per-library, per-platform and per-compiler after appropriate testing for performance, functionality, and footprint.'

[MBaesken]

Have you noticed any impact on build performance when enabling this on any JDK libs?

I see not much difference when looking at the build times of our builds with and without this patch. But this is the time for a whole scratch-build , not for single libs. For single libs with LTO enabled the time might be slower, but because this is just a small part of the (parallel) whole JDK build, it does not matter so much.

[mrserb]

@mrserb is this what you had in mind ?

Yes, this looks fine. It might be useful to update the documentation to mention which toolchains are supported (or at least have been tested once). Do we already have this documented somewhere for hotspot?

[MBaesken]

@mrserb is this what you had in mind ?

Yes, this looks fine. It might be useful to update the documentation to mention which toolchains are supported (or at least have been tested once). Do we already have this documented somewhere for hotspot?

We have support for all important toolchains (gcc, clang, MSVC). (when testing the PR on libfontmanager and freetype I had this enabled on all our SAP supported platforms AIX, various Linux, macOS, Windows x86_64).
However it might not work for all versions of these compilers.
And when building HS with lto support I remember a few jtreg tests failed (so it might need some more work to look into all details).

[erikj79]

However it might not work for all versions of these compilers.

When introducing new compiler flags, you need to do one of:

1. Verify that the flags are supported on all versions of the compiler we support. There is a minimum version defined for each. Reading the documentation to find out when the compiler feature was introduced is enough, you don't need to actually test every version yourself.
2. If 1 couldn't be proved, then only add the flag conditionally with a check in configure.

[MBaesken]

I wonder if it would be a good idea to add a global on/off switch (configure arg) for this whole thing, I'm really not sure. It kind of depends on what the impact is. If the impact is unknown, then a global on/off switch, default off, would let every distributor evaluate this on their own. That would of course also require that at least some libs have this enabled.

I can find a few other libs besides libfontmanager where we see some size reduction (performance improvement will be hard to prove without good benchmarks on lib level).
For those libs where we set LTO = true, we can add a global on/off configure arg to enable this. This way, we do not cause issues for distributors because still the configure flag must be set; but this provides an easy way to enable it and use lto on JDK lib level (and it will be easy to add it for more libs if it is seen as beneficial).

[MBaesken]

However it might not work for all versions of these compilers.

When introducing new compiler flags, you need to do one of:

1. Verify that the flags are supported on all versions of the compiler we support. There is a minimum version defined for each. Reading the documentation to find out when the compiler feature was introduced is enough, you don't need to actually test every version yourself.
2. If 1 couldn't be proved, then only add the flag conditionally with a check in configure.

The compiler flags are not really 'new' but borrowed from Hotspot LTO.

MSVC ( -GL -LTCG:INCREMENTAL ): available for a long time (~20 years), all our supported compilers

AIX : we only support XLC 17.X, there it is available

gcc : -flto=auto -fuse-linker-plugin -fno-strict-aliasing -fno-fat-lto-objects
since 4.8 (-flto=auto is documented since gcc-10 but older compilers seem to understand it too)

clang: I can find -flto in the clang13 manual (https://releases.llvm.org/13.0.0/tools/clang/docs/UsersManual.html) but cannot find -flto=auto , so maybe we should just just -flto in the link flags for clang ?

[prrace]

@azvegint please review

[MBaesken]

For those libs where we set LTO = true, we can add a global on/off configure arg to enable this. This way, we do not cause issues for distributors because still the configure flag must be set

For Hotspot / libjvm we have currently --enable-jvm-feature-link-time-opt ; maybe we should introduce something similar named for the JDK libs where we choose lto?

[mrserb]

When introducing new compiler flags, you need to do one of:

Just an idea to think about: would it be possible to share the same variable between the JDK and HotSpot to store these parameters? Even in this discussion, it would highlight that these parameters are not new. Or is it not worth the effort?

[MBaesken]

When introducing new compiler flags, you need to do one of:

Just an idea to think about: would it be possible to share the same variable between the JDK and HotSpot to store these parameters? Even in this discussion, it would highlight that these parameters are not new. Or is it not worth the effort?

For the HS build, we set them directly in the makefile and not in the configure m4 files, see

jdk/make/hotspot/lib/JvmFeatures.gmk

Line 179 in e4aed95

JVM_CFLAGS_FEATURES += -flto=auto -fuse-linker-plugin -fno-strict-aliasing \

Might indeed make sense to refactor this.

[TheShermanTanker]

When introducing new compiler flags, you need to do one of:

Just an idea to think about: would it be possible to share the same variable between the JDK and HotSpot to store these parameters? Even in this discussion, it would highlight that these parameters are not new. Or is it not worth the effort?

For the HS build, we set them directly in the makefile and not in the configure m4 files, see

jdk/make/hotspot/lib/JvmFeatures.gmk

Line 179 in e4aed95

JVM_CFLAGS_FEATURES += -flto=auto -fuse-linker-plugin -fno-strict-aliasing \

Might indeed make sense to refactor this.

It would be a little awkward to refactor only the JVM LTO flags into configure flags while leaving the rest of the JVM feature flags set by the Makefile, just my 2 cents.

[MBaesken]

It would be a little awkward to refactor only the JVM LTO flags into configure flags while leaving the rest of the JVM feature flags set by the Makefile, just my 2 cents.

The part in the Makefile would probably stay, but could reuse common flags from configure, e.g. something like this

    JVM_LDFLAGS_FEATURES += $(CXX_O_FLAG_HIGHEST_JVM) -flto=auto \
        -fuse-linker-plugin -fno-strict-aliasing
=>
    JVM_LDFLAGS_FEATURES += $(CXX_O_FLAG_HIGHEST_JVM) $(LDFLAGS_LTO)

[erikj79]

It would be a little awkward to refactor only the JVM LTO flags into configure flags while leaving the rest of the JVM feature flags set by the Makefile, just my 2 cents.

The part in the Makefile would probably stay, but could reuse common flags from configure, e.g. something like this

    JVM_LDFLAGS_FEATURES += $(CXX_O_FLAG_HIGHEST_JVM) -flto=auto \
        -fuse-linker-plugin -fno-strict-aliasing
=>
    JVM_LDFLAGS_FEATURES += $(CXX_O_FLAG_HIGHEST_JVM) $(LDFLAGS_LTO)

Yes, please only define the values once and use the variables in the JVM makefile.

[MBaesken]

Here are the lib sizes of libsplashscreen without / with lto for our toolchains at SAP (gcc 13.2.0, VS2022, Xcode 15.4) ; so the lib might be a good example to use lto (maybe with an additional configure flag to enable/disable it).

jdk opt / opt+LTO
1.5M / 1.5M  aix_ppc64/images/jdk/lib/libsplashscreen.so
304K / 220K  linux_aarch64/images/jdk/lib/libsplashscreen.so
292K / 180K  linux_alpine_x86_64/images/jdk/lib/libsplashscreen.so
376K / 228K  linux_ppc64le/images/jdk/lib/libsplashscreen.so
288K / 176K  linux_x86_64/images/jdk/lib/libsplashscreen.so
388K / 240K  darwinaarch64/images/jdk/lib/libsplashscreen.dylib
304K / 196K  darwinintel64/images/jdk/lib/libsplashscreen.dylib
204K / 204K  windows x86_64/images/jdk/bin/splashscreen.dll

pdb / debuginfo size :
jdk opt / opt+LTO
1.4M / 984K  linux_aarch64/images/jdk/lib/libsplashscreen.debuginfo
1.4M / 924K  linux_alpine_x86_64/images/jdk/lib/libsplashscreen.debuginfo
1.4M / 976K  linux_ppc64le/images/jdk/lib/libsplashscreen.debuginfo
1.4M / 924K  linux_x86_64/images/jdk/lib/libsplashscreen.debuginfo
352K / 344K  windows x86_64/images/jdk/bin/splashscreen.dll.pdb

[MBaesken]

Yes, please only define the values once and use the variables in the JVM makefile.

I adjusted the build flag settings in JvmFeatures.gmk to use the flags set by configure.

[erikj79]

I don't think adding the LTO flags to the OPT flags is the right move, but if we are going with that, then this addition of LTO flags should only be done if $$($1_OPTIMIZATION) is set, as otherwise, those flags are already added through $$($2_OPT_CFLAGS). That means, this block should be moved into the else block above, before the endif on line 64.

However, the OPT flags are treated separately through SetupCompilerFlags and SetupCompileFileFlags because it should be possible to override the optimization level on a per file basis. The LTO flags on the other hand, are not possible to override on a per file basis, so we should not be tinkering with those in the SetupCompileFileFlags macro at all. So mixing in the LTO flags with the OPT flags is the wrong move.

[MBaesken]

I don't think adding the LTO flags to the OPT flags is the right move

So where should I move it? After all it is link time optimization so it is an optimization related tool feature.
But should I remove it completely from SetupCompileFileFlags because having it in SetupCompilerFlags is sufficient for out task ?

[erikj79]

Again, the optimization flags aren't handled as a separate set of variables because they are of the category "optimization". It's because optimization flags are handled on a higher level with an abstraction of "low", "medium", "high" etc, and that abstraction allows for specific file overrides. That's what the *_OPT_C*FLAGS are there to handle. Since the LTO flags are not related to this abstraction, they should not be mixed into these variables, that is just confusing and adding unnecessary complexity.

The LTO flags should just be stacked on to the $1_EXTRA_* flags like any other.

[erikj79]

Again, the optimization flags aren't handled as a separate set of variables because they are of the category "optimization". It's because optimization flags are handled on a higher level with an abstraction of "low", "medium", "high" etc, and that abstraction allows for specific file overrides. That's what the *_OPT_C*FLAGS are there to handle. Since the LTO flags are not related to this abstraction, they should not be mixed into these variables, that is just confusing and adding unnecessary complexity.

The LTO flags should just be stacked on to the $1_EXTRA_* flags like any other.