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

[Scons] Added the ability to change the visibility of symbols #1313

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

DmitriySalnikov
Copy link
Contributor

@DmitriySalnikov DmitriySalnikov commented Nov 20, 2023

Added an argument to change -fvisibility.
This greatly reduces the size of the libraries and does not affect the functionality in any way, since only the entry point is used.
This argument mainly affects the size of shared libraries, .a files retain approximately the same size, but without this argument in godot-cpp itself, the size of the library does not change much.

In my case, I used my library for testing

scons platform=web target=template_release (need to apply the patch first)
Before this PR:
2.44 MB Export[10781] (via wasm-objdump -x)

After this PR:
512.64 KB Export[10]

-fvisibility=hidden only in my code:
2.13 MB Export[8936]

Initially, I wanted to try to hide symbols only for the Web to reduce the size of the dummy library, because 2.5MB is too much.
But it turned out that by hiding useless symbols for all platforms, I managed to reduce the total size of the libraries from 45MB to 27MB1.

In my understanding, disabling visibility by default will not affect the absolute majority of users, since usually the interaction of the library and godot occurs through a single entry point, which is already explicitly exported via GDE_EXPORT.
For those who really need it, they can use symbols_visibility=visible/symbols_visibility=auto (does not change the compiler arguments in any way) or explicitly specify GDE_EXPORT.

Also in godot itself on ios and android, -fvisibility=hidden is used by default.

-fvisibility=hidden can be replaced with --exclude-libs in user's scons, but it is only supported on linux and android. macOS, wasm and probably others either do not have such an argument, or use a different approach.

Perhaps the name of the new argument and its enumeration may not be quite correct, it may be worth changing them.

Footnotes

  1. There, in my repository, the use of Android NDK is also fixed, as GitHub has removed NDK r23 support from its images. It does not affect the size.

@DmitriySalnikov DmitriySalnikov requested a review from a team as a code owner November 20, 2023 19:36
@Calinou Calinou added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Nov 21, 2023
@Calinou Calinou added this to the 4.x milestone Nov 21, 2023
@Calinou
Copy link
Member

Calinou commented Nov 21, 2023

Also in godot itself on ios and android, -fvisibility=hidden is used by default.

Would this be worth doing on desktop platforms too for Godot itself, at least as an opt-in option?

@DmitriySalnikov
Copy link
Contributor Author

DmitriySalnikov commented Nov 21, 2023

I tried to view the exports of the godot executable for linux downloaded from the official website, but there were no unnecessary exports in it. Maybe I need to know more about exports in Godot 🤷‍♂️


As far as I understand -fvisibility=hidden mainly affects shared libraries

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 21, 2023

Also in godot itself on ios and android, -fvisibility=hidden is used by default.

Would this be worth doing on desktop platforms too for Godot itself, at least as an opt-in option?

I've been meaning to try using -fvisibility=hidden with the Godot executeable on desktop Linux, to see if could be an alternative solution to issue godotengine/godot#82812. We fixed that in a different way, but it has some caveats.

If this would also perhaps make the Godot executeable smaller, that would be another interesting benefit :-)

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 21, 2023

Overall, this looks good to me! I think -fvisibility=hidden is a good default

@DmitriySalnikov Can you rebase on the latest master now that PR #1314 is merged? That'll hopefully show the CI passing for Android builds

@DmitriySalnikov
Copy link
Contributor Author

DmitriySalnikov commented Nov 21, 2023

If this would also perhaps make the Godot executeable smaller, that would be another interesting benefit :-)

Most likely it will not affect in any way. The official GCC documentation mentions only shared objects.

It can be useful in godotengine/godot#72883

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 21, 2023

Most likely it will not affect in any way. The official GCC documentation mentions only shared objects.

Possibly. However, if you run nm -g --defined-only GODOT_EXECUTEABLE it generates a huge list, which leads me to believe that the table of exported symbols is filled up with all that data, whereas using -fvisibility=hidden could perhaps leave that table blank, which should theoretically save some space. It's worth testing to see what happens :-)

@DmitriySalnikov
Copy link
Contributor Author

However, if you run nm -g --defined-only GODOT_EXECUTEABLE it generates a huge list

Official 4.1.3 and 4.2.rc1
image
Maybe this is relevant for debug builds or macOS?

Anyway, for now I want to reduce the size of GDExtension libraries and continue updating DD3D.

@bruvzg
Copy link
Member

bruvzg commented Nov 21, 2023

Maybe this is relevant for debug builds or macOS?

It's relevant for official macOS builds (debug builds have more exposed symbols).

The official GCC documentation mentions only shared objects.

macOS builds always use clang, which might have different defaults. But in most cases, there's almost no difference between shared object and executable.

Also, adding -fvisibility=hidden seems to make macOS executable about 4 MB larger.

@DmitriySalnikov
Copy link
Contributor Author

DmitriySalnikov commented Nov 21, 2023

seems to make macOS executable about 4 MB larger.

It's strange but macOS libraries have become ~2.5 times smaller for me.
web ~4.8 times smaller.
browser_gb2b4K75dA

I checked the Web and Linux builds (Linux also passed tests here), but I can't check macOS yet.

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 21, 2023

Official 4.1.3 and 4.2.rc1
image
Maybe this is relevant for debug builds or macOS?

Oh, that's interesting! I can confirm what you're seeing with the official builds. For whatever reason, the builds I do locally are packed with exported symbols. Anyway, this is a bit off-topic :-)

Android is now building fine on CI and this PR looks good to me!

@dsnopek dsnopek merged commit 588d869 into godotengine:master Nov 24, 2023
12 checks passed
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 24, 2023

Thanks!

@dsnopek
Copy link
Collaborator

dsnopek commented Jun 14, 2024

Cherry-picked for 4.1 in PR #1491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants