8373388: Reenable LTO for libsplashscreen

[MBaesken]

JDK-8372554 disabled LTO for libsplashscreen to fix an issue with the comparison build. It turns out the failure was caused by extra files that LTO produced and which were included in the image. Now that JDK-8373255 has fixed that we should be able to reenable LTO again.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8373388: Reenable LTO for libsplashscreen (Bug - P4)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)
• David Holmes (@dholmes-ora - Reviewer)
• Sergey Bylokhov (@mrserb - Reviewer)
• Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28746

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28746.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]

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

8373388: Reenable LTO for libsplashscreen

Reviewed-by: erikj, dholmes, serb, prr

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

• b6319f5: 8369595: HttpClient: HttpHeaders.firstValueAsLong failures should be converted to ProtocolException
• 6e2ab84: 8068378: [TEST_BUG]The java/awt/Modal/PrintDialogsTest/PrintDialogsTest.java instruction need to update
• 9b12c0b: 7067310: 3 tests from closed/javax/sound/sampled caused BSOD on win 7 x86
• ... and 30 more: https://git.openjdk.org/jdk/compare/b58e3b600bb14bf7133eda0c37a4be4c82919d79...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]

@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.

[mlbridge]

Webrevs

• 00: Full (0b3dfdfe)

[mrserb]

Don’t we need to wait until #28676 is fixed?

[prrace]

Why was it enabled in the first place ?

[prrace]

/reviewers 2 reviewer

[prrace]

Do not integrate this without my review/approval.

[openjdk]

@prrace
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[MBaesken]

Don’t we need to wait until #28676 is fixed?

We can wait for 8372759, no problem. ( But this issue showed up only in some special environments anyway. )

[MBaesken]

Why was it enabled in the first place ?

At least we could identify and fix 2 bugs with enabling this (and those 2 bugs were in for 5+ years) .
If you prefer to have some 'opt-in' switch via configure for lto in the same way we have it for libjvm , we can of course discuss this and introduce it.
If your question is about 'why for libsplashscreen' , because we see some good lib size - benefits for this lib (there were some measurements by @mserb for this #27976 (comment) ) .

[prrace]

Why was it enabled in the first place ?
If your question is about 'why for libsplashscreen' , because we see some good lib size - benefits for this lib (there were some measurements by @mserb for this #27976 (comment) ) .

Yes, my question was why splash screen out of all the libraries in the JDK.
I guess a size reduction is good, but I've no idea if it helps performance.
Note that on Linux, IIRC, the system gif/png/jpeg libs may be used by the build, and in such a case
I doubt there's much "size" left to reduce.

I've re-tested the comparison build and it passed. So I'll approve, just for this one library.

[MBaesken]

Thanks for the reviews !

/integrate

[openjdk]

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

• b6319f5: 8369595: HttpClient: HttpHeaders.firstValueAsLong failures should be converted to ProtocolException
• 6e2ab84: 8068378: [TEST_BUG]The java/awt/Modal/PrintDialogsTest/PrintDialogsTest.java instruction need to update
• 9b12c0b: 7067310: 3 tests from closed/javax/sound/sampled caused BSOD on win 7 x86
• ... and 30 more: https://git.openjdk.org/jdk/compare/b58e3b600bb14bf7133eda0c37a4be4c82919d79...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit 4e9525e.

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