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

Bundle binary footprint optimization and symbol visibility. #442

Closed
PengZheng opened this issue Sep 1, 2022 · 4 comments · Fixed by #544 or #584
Closed

Bundle binary footprint optimization and symbol visibility. #442

PengZheng opened this issue Sep 1, 2022 · 4 comments · Fixed by #544 or #584
Assignees
Labels
kind/improvement Categorizes issue or PR as related to improvements.
Milestone

Comments

@PengZheng
Copy link
Contributor

PengZheng commented Sep 1, 2022

I've implemented a C++ bundle named json_config, which depends on https://github.com/nlohmann/json and https://github.com/pboettch/json-schema-validator. It turns out that building using common settings will produce a 2.4M binary:

CFLAGS=-fdata-sections -ffunction-sections -Os
CXXFLAGS=-fdata-sections -ffunction-sections -Os
LDFLAGS=-Wl,--gc-sections

Further inspection reveals that a lot of C++ template instantiation unused is built into the final binary. Linking using the following linker script eliminated all these unused code from the binary, resulting in 300K binary, which is a huge win:

{
global:
    celix_bundleActivator_create;
    celix_bundleActivator_start;
    celix_bundleActivator_stop;
    celix_bundleActivator_destroy;
local: *;
};

I noticed that the original API design took symbol visibility into account, but the new celix API did not:

FRAMEWORK_EXPORT celix_status_t framework_create(celix_framework_t **framework, celix_properties_t *config);

Shall we add symbol visibility control back to the whole Celix API? This is important especially for C++ bundle.

@pnoltes
Copy link
Contributor

pnoltes commented Apr 10, 2023

I think it would be good to reintroduce FRAMEWORK_EXPORT using new set of marcros and some documentation.

For marcro, I was thinking about:

//celix_export.h (part of celix_utils lib)
#define CELIX_EXPORT __attribute__((visibility ("default")))
#define CELIX_LOCAL __attribute__((visibility ("hidden")))

And applying the marcros to all C/C++ utils, dfi and framework headers.

To be honest I am not sure how this needs to be applied to bundle libraries.
With the linking script example, only the bundle activator symbols are global and I think this is default what we want for bundles.

@PengZheng Does this means that all C++ - note header only - methods needs to be declared as "CELIX_LOCAL" , with an exception for the C++ exceptions?

Is an other option to let the add_celix_bundle cmake function default produce a linker script, so that only the bundle activator functions are global?

@PengZheng
Copy link
Contributor Author

PengZheng commented Apr 11, 2023

To be honest I am not sure how this needs to be applied to bundle libraries.

Bundles are also shared libraries, so we only have to deal with shared objects.
As you have noted, bundles are simpler, since they export a fixed set of symbols, i.e. activator symbols.

For shared library (and bundle) target, we set C_VISIBILITY_PRESET and CXX_VISIBILITY_PRESET to be hidden and VISIBILITY_INLINES_HIDDEN to be ON. Then for a given shared library, we add CELIX_EXPORT to its public API.
For bundles, we can set these target properties in add_celix_bundle, and add CELIX_EXPORT to celix_bundle_activator.h, and then we're done.

I think CELIX_LOCAL is not really necessary.

Does this means that all C++ - note header only - methods needs to be declared as "CELIX_LOCAL" , with an exception for the C++ exceptions?

No, these C++ headers does NOT form public API of any shared object. Users can use it to write static/shared library, executable, or whatever they want. They should take care of the symbol visibility themselves. With the help of CMake (GenerateExportHeader), it should not be burdensome.

C++ ABI is notoriously hard to maintain, I'm happy we don't have it (please correct me if I were wrong). But CppMicroServices does have C++ API, and they control their class's visibility using similar approach.

As for exception, since we only have C API (bundle activator) and they are nothrow by nature, any C++ exception should be catched and translated into error code before returning to the C framework. Currently we don't do it that way, a C++ exception will lead to crash.

Is an other option to let the add_celix_bundle cmake function default produce a linker script, so that only the bundle activator functions are global?

I use version script (-Wl,--version-script=) as a last resort. It works like charm, especially when I can not modify the upstream. For example, when doing footprint optimization of OpenSSL, I use it to only export the used symbols (approximately 300) instead of 3000+. But it leads to less optimized code, see Section 2.2 of Ulrich Drepper's definitive How To Write Shared Libraries .

@pnoltes
Copy link
Contributor

pnoltes commented Apr 11, 2023

@PengZheng I would like to try and tackle this.
I do not have a lot of experience with symbol visibility, but that is also a good reason to dive into this.

@PengZheng PengZheng assigned PengZheng and pnoltes and unassigned PengZheng Apr 14, 2023
@pnoltes
Copy link
Contributor

pnoltes commented Apr 17, 2023

I will try to solve this issue in 4 steps:

  • Apply export header configuration and export macro usage to the libraries in the libs dir.
  • Add way to configure bundle symbol visibility and apply this or configured bundle symbol visibility using export header.
  • Add Apache Celix developer and user documentation for symbol visiblity.
  • Apply export header configuration for libs in the bundles directories if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/improvement Categorizes issue or PR as related to improvements.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants