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

8278260: JavaFX shared libraries not stripped on Linux or macOS #695

Closed
wants to merge 2 commits into from

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 15, 2021

Build change to strip the non-global symbols from native shared libraries on Linux and macOS by running strip -x, unless doing a -PCONF=DebugNative build.

Here is a before / after size comparison. All sizes in KBytes:

Linux

Native Library Current Stripped
libavplugin-54.so 52 48
libavplugin-56.so 52 48
libavplugin-57.so 52 48
libavplugin-ffmpeg-56.so 52 48
libavplugin-ffmpeg-57.so 52 48
libavplugin-ffmpeg-58.so 52 48
libdecora_sse.so 76 72
libfxplugins.so 56 52
libglass.so 16 12
libglassgtk2.so 932 324
libglassgtk3.so 932 324
libgstreamer-lite.so 2,280 2,100
libjavafx_font.so 20 16
libjavafx_font_freetype.so 28 28
libjavafx_font_pango.so 28 24
libjavafx_iio.so 148 140
libjfxmedia.so 2,048 516
libjfxwebkit.so 106,696 88,428
libprism_common.so 8 8
libprism_es2.so 64 64
libprism_sw.so 68 64

macOS

Native Library Current Stripped
libdecora_sse.dylib 88 88
libfxplugins.dylib 88 84
libglass.dylib 360 324
libglib-lite.dylib 1,192 1,148
libgstreamer-lite.dylib 1,708 1584
libjavafx_font.dylib 64 64
libjavafx_iio.dylib 308 300
libjfxmedia.dylib 212 200
libjfxmedia_avf.dylib 100 88
libjfxwebkit.dylib 85,896 71,636
libprism_common.dylib 20 20
libprism_es2.dylib 64 64
libprism_sw.dylib 88 88

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8278260: JavaFX shared libraries not stripped on Linux or macOS

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/695/head:pull/695
$ git checkout pull/695

Update a local copy of the PR:
$ git checkout pull/695
$ git pull https://git.openjdk.java.net/jfx pull/695/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 695

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/695.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 15, 2021

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

@kevinrushforth
Copy link
Member Author

/reviewers 2

@openjdk openjdk bot added the rfr Ready for review label Dec 15, 2021
@openjdk
Copy link

openjdk bot commented Dec 15, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@mlbridge
Copy link

mlbridge bot commented Dec 15, 2021

Webrevs

@johanvos
Copy link
Collaborator

This looks good and useful. I'll do builds and tests on Linux and Mac.
The only question I have for now is if it would make sense to have a new configuration instead of binding this to DEBUG_NATIVE. The impact of building with DEBUG_NATIVE is very big.
For most debugging and crash resolutions, DEBUG_NATIVE is not required, but symbols make it easier/faster to debug.
But it's doable with stripped libs as well of course -- and I agree stripped libs should be the default for distributions.

@kevinrushforth
Copy link
Member Author

The only question I have for now is if it would make sense to have a new configuration instead of binding this to DEBUG_NATIVE.

I had initially thought of only stripping the binaries for production builds, e.g., when -PCONF=Release is specified. That seems preferable to creating a new configuration. What do you think?

@mstr2
Copy link
Collaborator

mstr2 commented Dec 16, 2021

I don't think that stripping unused symbols is necessary for debug builds, and it's okay to only enable such optimizations for release builds.

@mstr2
Copy link
Collaborator

mstr2 commented Dec 16, 2021

Here are my results when building with -PCONF=Release on macOS 11.6.1:

Native Library current kevinrushforth mstr2
libdecora_sse.dylib 88 88 87
libfxplugins.dylib 88 84 83
libglass.dylib 360 324 331
libglib-lite.dylib 1,192 1,148 1,171
libgstreamer-lite.dylib 1,708 1584 1,615
libjavafx_font.dylib 64 64 64
libjavafx_iio.dylib 308 300 306
libjfxmedia.dylib 212 200 204
libjfxmedia_avf.dylib 100 88 89
libjfxwebkit.dylib 85,896 71,636 72,905
libprism_common.dylib 20 20 16
libprism_es2.dylib 64 64 61
libprism_sw.dylib 88 88 87

@johanvos
Copy link
Collaborator

The only question I have for now is if it would make sense to have a new configuration instead of binding this to DEBUG_NATIVE.

I had initially thought of only stripping the binaries for production builds, e.g., when -PCONF=Release is specified. That seems preferable to creating a new configuration. What do you think?

That sounds perfect indeed, and I see you committed that change already -- thanks.

@kevinrushforth
Copy link
Member Author

Here are my results when building with -PCONF=Release on macOS 11.6.1:

My guess is that the differences are due to either your macOS version or the Xcode you used to build, and that your "current" numbers would also be different from mine. FWIW, mine were generated on macOS 10.15.7 using Xcode 12.4.

Copy link
Collaborator

@johanvos johanvos left a comment

Choose a reason for hiding this comment

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

Tested on Linux and Mac

@openjdk
Copy link

openjdk bot commented Dec 16, 2021

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

8278260: JavaFX shared libraries not stripped on Linux or macOS

Reviewed-by: jvos, aghaisas

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 1 new commit pushed to the master branch:

  • 27f2810: 8273089: Deprecate JavaFX GTK 2 library for removal

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Ready to be integrated label Dec 16, 2021
@kevinrushforth
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Dec 16, 2021

Going to push as commit 1ebf7a2.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 27f2810: 8273089: Deprecate JavaFX GTK 2 library for removal

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 16, 2021
@openjdk openjdk bot closed this Dec 16, 2021
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 16, 2021
@openjdk
Copy link

openjdk bot commented Dec 16, 2021

@kevinrushforth Pushed as commit 1ebf7a2.

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

@kevinrushforth kevinrushforth deleted the 8278260-strip branch December 16, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants